Re: [PATCH v10 08/19] qspinlock: Make a new qnode structure to support virtualization
On Fri, May 09, 2014 at 09:08:56PM -0400, Waiman Long wrote:
On 05/08/2014 03:04 PM, Peter Zijlstra wrote:
On Wed, May 07, 2014 at 11:01:36AM -0400, Waiman Long wrote:
/*
+ * To have additional features for better virtualization support, it is
+ * necessary to store additional data in the queue node structure. So
+ * a new queue node structure will have to be defined and used here.
+ */
+struct qnode {
+ struct mcs_spinlock mcs;
+};
You can ditch this entire patch; its pointless, just add a new
DEFINE_PER_CPU for the para-virt muck.
Yes, I can certainly merge it to the next one in the series. I break it out
to make each individual patch smaller, more single-purpose and easier to
review.
No, don't merge it, _drop_ it. Wrapping things in a struct generates a
ton of pointless change.
Put the new data in a new DEFINE_PER_CPU and leave the existing code as
is.
pgp44pHRGZ7w8.pgp
Description: PGP signature
Re: [PATCH v10 08/19] qspinlock: Make a new qnode structure to support virtualization
On Sat, May 10, 2014 at 04:14:17PM +0200, Peter Zijlstra wrote:
On Fri, May 09, 2014 at 09:08:56PM -0400, Waiman Long wrote:
On 05/08/2014 03:04 PM, Peter Zijlstra wrote:
On Wed, May 07, 2014 at 11:01:36AM -0400, Waiman Long wrote:
/*
+ * To have additional features for better virtualization support, it is
+ * necessary to store additional data in the queue node structure. So
+ * a new queue node structure will have to be defined and used here.
+ */
+struct qnode {
+ struct mcs_spinlock mcs;
+};
You can ditch this entire patch; its pointless, just add a new
DEFINE_PER_CPU for the para-virt muck.
Yes, I can certainly merge it to the next one in the series. I break it out
to make each individual patch smaller, more single-purpose and easier to
review.
No, don't merge it, _drop_ it. Wrapping things in a struct generates a
ton of pointless change.
Put the new data in a new DEFINE_PER_CPU and leave the existing code as
is.
So I had a look at the resulting code:
struct qnode {
struct mcs_spinlock mcs;
#ifdef CONFIG_PARAVIRT_UNFAIR_LOCKS
int lsteal_mask;/* Lock stealing frequency mask */
u32 prev_tail; /* Tail code of previous node */
#ifndef CONFIG_PARAVIRT_SPINLOCKS
struct qnode *qprev; /* Previous queue node addr */
#endif
#endif
struct pv_qvars pv; /* For para-virtualization */
};
With all the bells and whistles on (say an enterprise distro), that
single node will now fill an entire cacheline on its own.
That means that the normal case for normal people who stay the heck away
from virt shit will very often hit _3_ cachelines for their spin_lock().
1 - the cacheline that has the spinlock_t in,
2 - the cacheline that has node[0].count in to find which node to use
3 - the cacheline that has the actual right node in
That's of course complete and utter crap.
Not to mention that the final result of those 19 patches is going to
take me days to untangle :-(
Days I don't really have because I get to go hunt bugs in existing code
before thinking about adding shiny new stuff.
pgp3fLZxYHOe1.pgp
Description: PGP signature
Re: [PATCH v10 08/19] qspinlock: Make a new qnode structure to support virtualization
On 05/08/2014 03:04 PM, Peter Zijlstra wrote:
On Wed, May 07, 2014 at 11:01:36AM -0400, Waiman Long wrote:
/*
+ * To have additional features for better virtualization support, it is
+ * necessary to store additional data in the queue node structure. So
+ * a new queue node structure will have to be defined and used here.
+ */
+struct qnode {
+ struct mcs_spinlock mcs;
+};
You can ditch this entire patch; its pointless, just add a new
DEFINE_PER_CPU for the para-virt muck.
Yes, I can certainly merge it to the next one in the series. I break it
out to make each individual patch smaller, more single-purpose and
easier to review.
-Longman
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 08/19] qspinlock: Make a new qnode structure to support virtualization
In order to support additional virtualization features like unfair lock
and para-virtualized spinlock, it is necessary to store additional
CPU specific data into the queue node structure. As a result, a new
qnode structure is created and the mcs_spinlock structure is now part
of the new structure.
It is also necessary to expand arch_mcs_spin_lock_contended() to the
underlying while loop as additional code will need to be inserted
into the loop.
Signed-off-by: Waiman Long waiman.l...@hp.com
---
kernel/locking/qspinlock.c | 36 +++-
1 files changed, 23 insertions(+), 13 deletions(-)
diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
index 0ee1a23..e98d7d4 100644
--- a/kernel/locking/qspinlock.c
+++ b/kernel/locking/qspinlock.c
@@ -57,12 +57,21 @@
#include mcs_spinlock.h
/*
+ * To have additional features for better virtualization support, it is
+ * necessary to store additional data in the queue node structure. So
+ * a new queue node structure will have to be defined and used here.
+ */
+struct qnode {
+ struct mcs_spinlock mcs;
+};
+
+/*
* Per-CPU queue node structures; we can never have more than 4 nested
* contexts: task, softirq, hardirq, nmi.
*
* Exactly fits one cacheline.
*/
-static DEFINE_PER_CPU_ALIGNED(struct mcs_spinlock, mcs_nodes[4]);
+static DEFINE_PER_CPU_ALIGNED(struct qnode, qnodes[4]);
/*
* We must be able to distinguish between no-tail and the tail at 0:0,
@@ -79,12 +88,12 @@ static inline u32 encode_tail(int cpu, int idx)
return tail;
}
-static inline struct mcs_spinlock *decode_tail(u32 tail)
+static inline struct qnode *decode_tail(u32 tail)
{
int cpu = (tail _Q_TAIL_CPU_OFFSET) - 1;
int idx = (tail _Q_TAIL_IDX_MASK) _Q_TAIL_IDX_OFFSET;
- return per_cpu_ptr(mcs_nodes[idx], cpu);
+ return per_cpu_ptr(qnodes[idx], cpu);
}
#define _Q_LOCKED_PENDING_MASK (_Q_LOCKED_MASK | _Q_PENDING_MASK)
@@ -342,7 +351,7 @@ static inline int trylock_pending(struct qspinlock *lock,
u32 *pval)
*/
void queue_spin_lock_slowpath(struct qspinlock *lock, u32 val)
{
- struct mcs_spinlock *prev, *next, *node;
+ struct qnode *prev, *next, *node;
u32 old, tail;
int idx;
@@ -351,13 +360,13 @@ void queue_spin_lock_slowpath(struct qspinlock *lock, u32
val)
if (trylock_pending(lock, val))
return; /* Lock acquired */
- node = this_cpu_ptr(mcs_nodes[0]);
- idx = node-count++;
+ node = this_cpu_ptr(qnodes[0]);
+ idx = node-mcs.count++;
tail = encode_tail(smp_processor_id(), idx);
node += idx;
- node-locked = 0;
- node-next = NULL;
+ node-mcs.locked = 0;
+ node-mcs.next = NULL;
/*
* We touched a (possibly) cold cacheline in the per-cpu queue node;
@@ -380,9 +389,10 @@ void queue_spin_lock_slowpath(struct qspinlock *lock, u32
val)
*/
if (old _Q_TAIL_MASK) {
prev = decode_tail(old);
- ACCESS_ONCE(prev-next) = node;
+ ACCESS_ONCE(prev-mcs.next) = (struct mcs_spinlock *)node;
- arch_mcs_spin_lock_contended(node-locked);
+ while (!smp_load_acquire(node-mcs.locked))
+ arch_mutex_cpu_relax();
}
/*
@@ -422,15 +432,15 @@ void queue_spin_lock_slowpath(struct qspinlock *lock, u32
val)
/*
* contended path; wait for next, release.
*/
- while (!(next = ACCESS_ONCE(node-next)))
+ while (!(next = (struct qnode *)ACCESS_ONCE(node-mcs.next)))
arch_mutex_cpu_relax();
- arch_mcs_spin_unlock_contended(next-locked);
+ arch_mcs_spin_unlock_contended(next-mcs.locked);
release:
/*
* release the node
*/
- this_cpu_dec(mcs_nodes[0].count);
+ this_cpu_dec(qnodes[0].mcs.count);
}
EXPORT_SYMBOL(queue_spin_lock_slowpath);
--
1.7.1
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
