On 03/19/2015 08:25 AM, Peter Zijlstra wrote:
> On Thu, Mar 19, 2015 at 11:12:42AM +0100, Peter Zijlstra wrote:
>> So I was now thinking of hashing the lock pointer; let me go and quickly
>> put something together.
> A little something like so; ideally we'd allocate the hashtable since
> NR_CPUS is kinda bloated, but it shows the idea I think.
>
> And while this has loops in (the rehashing thing) their fwd progress
> does not depend on other CPUs.
>
> And I suspect that for the typical lock contention scenarios its
> unlikely we ever really get into long rehashing chains.
>
> ---
> include/linux/lfsr.h | 49 ++++++++++++
> kernel/locking/qspinlock_paravirt.h | 143 ++++++++++++++++++++++++++++++++----
> 2 files changed, 178 insertions(+), 14 deletions(-)
>
> --- /dev/null
>
> +
> +static int pv_hash_find(struct qspinlock *lock)
> +{
> + u64 hash = hash_ptr(lock, PV_LOCK_HASH_BITS);
> + struct pv_hash_bucket *hb, *end;
> + int cpu = -1;
> +
> + if (!hash)
> + hash = 1;
> +
> + hb =&__pv_lock_hash[hash_align(hash)];
> + for (;;) {
> + for (end = hb + PV_HB_PER_LINE; hb< end; hb++) {
> + struct qspinlock *l = READ_ONCE(hb->lock);
> +
> + /*
> + * If we hit an unused bucket, there is no match.
> + */
> + if (!l)
> + goto done;
After more careful reading, I think the assumption that the presence of
an unused bucket means there is no match is not true. Consider the scenario:
1. cpu 0 puts lock1 into hb[0]
2. cpu 1 puts lock2 into hb[1]
3. cpu 2 clears hb[0]
4. cpu 3 looks for lock2 and doesn't find it
I was thinking about putting some USED flag in the buckets, but then we
will eventually fill them all up as used. If we put the entries into a
hashed linked list, we have to deal with the complicated synchronization
issues with link list update.
At this point, I am thinking using back your previous idea of passing
the queue head information down the queue. I am now convinced that the
unlock call site patching should work. So I will incorporate that in my
next update.
Please let me know if you think my reasoning is not correct.
Thanks,
Longman
On Wed, Apr 01, 2015 at 12:20:30PM -0400, Waiman Long wrote:
> After more careful reading, I think the assumption that the presence of an
> unused bucket means there is no match is not true. Consider the scenario:
>
> 1. cpu 0 puts lock1 into hb[0]
> 2. cpu 1 puts lock2 into hb[1]
> 3. cpu 2 clears hb[0]
> 4. cpu 3 looks for lock2 and doesn't find it
Hmm, yes. The only way I can see that being true is if we assume entries
are never taken out again.
The wikipedia page could use some clarification here, this is not clear.
> At this point, I am thinking using back your previous idea of passing the
> queue head information down the queue.
Having to scan the entire array for a lookup sure sucks, but the wait
loops involved in the other idea can get us in the exact predicament we
were trying to get out, because their forward progress depends on other
CPUs.
Hohumm.. time to think more I think ;-)
On Wed, Apr 01, 2015 at 07:12:23PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 01, 2015 at 12:20:30PM -0400, Waiman Long wrote:
> > After more careful reading, I think the assumption that the presence of an
> > unused bucket means there is no match is not true. Consider the scenario:
> >
> > 1. cpu 0 puts lock1 into hb[0]
> > 2. cpu 1 puts lock2 into hb[1]
> > 3. cpu 2 clears hb[0]
> > 4. cpu 3 looks for lock2 and doesn't find it
>
> Hmm, yes. The only way I can see that being true is if we assume entries
> are never taken out again.
>
> The wikipedia page could use some clarification here, this is not clear.
>
> > At this point, I am thinking using back your previous idea of passing the
> > queue head information down the queue.
>
> Having to scan the entire array for a lookup sure sucks, but the wait
> loops involved in the other idea can get us in the exact predicament we
> were trying to get out, because their forward progress depends on other
> CPUs.
>
> Hohumm.. time to think more I think ;-)
So bear with me, I've not really pondered this well so it could be full
of holes (again).
After the cmpxchg(&l->locked, _Q_LOCKED_VAL, _Q_SLOW_VAL) succeeds the
spin_unlock() must do the hash lookup, right? We can make the lookup
unhash.
If the cmpxchg() fails the unlock will not do the lookup and we must
unhash.
On Wed, Apr 01, 2015 at 07:42:39PM +0200, Peter Zijlstra wrote:
> > Hohumm.. time to think more I think ;-)
>
> So bear with me, I've not really pondered this well so it could be full
> of holes (again).
>
> After the cmpxchg(&l->locked, _Q_LOCKED_VAL, _Q_SLOW_VAL) succeeds the
> spin_unlock() must do the hash lookup, right? We can make the lookup
> unhash.
>
> If the cmpxchg() fails the unlock will not do the lookup and we must
> unhash.
The idea being that the result is that any lookup is guaranteed to find
an entry, which reduces our worst case lookup cost to whatever the worst
case insertion cost was.
On Wed, Apr 01, 2015 at 02:54:45PM -0400, Waiman Long wrote:
> On 04/01/2015 02:17 PM, Peter Zijlstra wrote:
> >On Wed, Apr 01, 2015 at 07:42:39PM +0200, Peter Zijlstra wrote:
> >>>Hohumm.. time to think more I think ;-)
> >>So bear with me, I've not really pondered this well so it could be full
> >>of holes (again).
> >>
> >>After the cmpxchg(&l->locked, _Q_LOCKED_VAL, _Q_SLOW_VAL) succeeds the
> >>spin_unlock() must do the hash lookup, right? We can make the lookup
> >>unhash.
> >>
> >>If the cmpxchg() fails the unlock will not do the lookup and we must
> >>unhash.
> >The idea being that the result is that any lookup is guaranteed to find
> >an entry, which reduces our worst case lookup cost to whatever the worst
> >case insertion cost was.
> >
>
> I think it doesn't matter who did the unhashing. Multiple independent locks
> can be hashed to the same value. Since they can be unhashed independently,
> there is no way to know whether you have checked all the possible buckets.
oh but the crux is that you guarantee a lookup will find an entry. it will
never need to iterate the entire array.
On 04/01/2015 02:17 PM, Peter Zijlstra wrote:
> On Wed, Apr 01, 2015 at 07:42:39PM +0200, Peter Zijlstra wrote:
>>> Hohumm.. time to think more I think ;-)
>> So bear with me, I've not really pondered this well so it could be full
>> of holes (again).
>>
>> After the cmpxchg(&l->locked, _Q_LOCKED_VAL, _Q_SLOW_VAL) succeeds the
>> spin_unlock() must do the hash lookup, right? We can make the lookup
>> unhash.
>>
>> If the cmpxchg() fails the unlock will not do the lookup and we must
>> unhash.
> The idea being that the result is that any lookup is guaranteed to find
> an entry, which reduces our worst case lookup cost to whatever the worst
> case insertion cost was.
>
I think it doesn't matter who did the unhashing. Multiple independent
locks can be hashed to the same value. Since they can be unhashed
independently, there is no way to know whether you have checked all the
possible buckets.
-Longman
On 04/01/2015 02:48 PM, Peter Zijlstra wrote:
> On Wed, Apr 01, 2015 at 02:54:45PM -0400, Waiman Long wrote:
>> On 04/01/2015 02:17 PM, Peter Zijlstra wrote:
>>> On Wed, Apr 01, 2015 at 07:42:39PM +0200, Peter Zijlstra wrote:
>>>>> Hohumm.. time to think more I think ;-)
>>>> So bear with me, I've not really pondered this well so it could be full
>>>> of holes (again).
>>>>
>>>> After the cmpxchg(&l->locked, _Q_LOCKED_VAL, _Q_SLOW_VAL) succeeds the
>>>> spin_unlock() must do the hash lookup, right? We can make the lookup
>>>> unhash.
>>>>
>>>> If the cmpxchg() fails the unlock will not do the lookup and we must
>>>> unhash.
>>> The idea being that the result is that any lookup is guaranteed to find
>>> an entry, which reduces our worst case lookup cost to whatever the worst
>>> case insertion cost was.
>>>
>> I think it doesn't matter who did the unhashing. Multiple independent locks
>> can be hashed to the same value. Since they can be unhashed independently,
>> there is no way to know whether you have checked all the possible buckets.
> oh but the crux is that you guarantee a lookup will find an entry. it will
> never need to iterate the entire array.
I am sorry that I don't quite get what you mean here. My point is that
in the hashing step, a cpu will need to scan an empty bucket to put the
lock in. In the interim, an previously used bucket before the empty one
may get freed. In the lookup step for that lock, the scanning will stop
because of an empty bucket in front of the target one.
-Longman
On 04/01/2015 01:12 PM, Peter Zijlstra wrote:
> On Wed, Apr 01, 2015 at 12:20:30PM -0400, Waiman Long wrote:
>> After more careful reading, I think the assumption that the presence of an
>> unused bucket means there is no match is not true. Consider the scenario:
>>
>> 1. cpu 0 puts lock1 into hb[0]
>> 2. cpu 1 puts lock2 into hb[1]
>> 3. cpu 2 clears hb[0]
>> 4. cpu 3 looks for lock2 and doesn't find it
> Hmm, yes. The only way I can see that being true is if we assume entries
> are never taken out again.
>
> The wikipedia page could use some clarification here, this is not clear.
>
>> At this point, I am thinking using back your previous idea of passing the
>> queue head information down the queue.
> Having to scan the entire array for a lookup sure sucks, but the wait
> loops involved in the other idea can get us in the exact predicament we
> were trying to get out, because their forward progress depends on other
> CPUs.
For the waiting loop, the worst case is when a new CPU get queued right
before we write the head value to the previous tail node. In the case,
the maximum number of retries is equal to the total number of CPUs - 2.
But that should rarely happen.
I do find a way to guarantee forward progress in a few steps. I will try
the normal way once. If that fails, I will insert the head node to the
tail once again after saving the next pointer. After modifying the
previous tail node, cmpxchg will be used to restore the previous tail.
If that fails, we just have to wait until the next pointer is updated
and write it out to the previous tail node. We can now restore the next
pointer and move forward.
Let me know if that looks reasonable to you.
-Longman
On Wed, Apr 01, 2015 at 03:58:58PM -0400, Waiman Long wrote:
> On 04/01/2015 02:48 PM, Peter Zijlstra wrote:
> I am sorry that I don't quite get what you mean here. My point is that in
> the hashing step, a cpu will need to scan an empty bucket to put the lock
> in. In the interim, an previously used bucket before the empty one may get
> freed. In the lookup step for that lock, the scanning will stop because of
> an empty bucket in front of the target one.
Right, that's broken. So we need to do something else to limit the
lookup, because without that break, a lookup that needs to iterate the
entire array in order to determine -ENOENT, which is expensive.
So my alternative proposal is that IFF we can guarantee that every
lookup will succeed -- the entry we're looking for is always there, we
don't need the break on empty but can probe until we find the entry.
This will be bound in cost to the same number if probes we required for
insertion and avoids the full array scan.
Now I think we can indeed do this, if as said earlier we do not clear
the bucket on insert if the cmpxchg succeeds, in that case the unlock
will observe _Q_SLOW_VAL and do the lookup, the lookup will then find
the entry. And we then need the unlock to clear the entry.
Does that explain this? Or should I try again with code?
On 04/01/2015 05:03 PM, Peter Zijlstra wrote:
> On Wed, Apr 01, 2015 at 03:58:58PM -0400, Waiman Long wrote:
>> On 04/01/2015 02:48 PM, Peter Zijlstra wrote:
>> I am sorry that I don't quite get what you mean here. My point is that in
>> the hashing step, a cpu will need to scan an empty bucket to put the lock
>> in. In the interim, an previously used bucket before the empty one may get
>> freed. In the lookup step for that lock, the scanning will stop because of
>> an empty bucket in front of the target one.
> Right, that's broken. So we need to do something else to limit the
> lookup, because without that break, a lookup that needs to iterate the
> entire array in order to determine -ENOENT, which is expensive.
>
> So my alternative proposal is that IFF we can guarantee that every
> lookup will succeed -- the entry we're looking for is always there, we
> don't need the break on empty but can probe until we find the entry.
> This will be bound in cost to the same number if probes we required for
> insertion and avoids the full array scan.
>
> Now I think we can indeed do this, if as said earlier we do not clear
> the bucket on insert if the cmpxchg succeeds, in that case the unlock
> will observe _Q_SLOW_VAL and do the lookup, the lookup will then find
> the entry. And we then need the unlock to clear the entry.
> _Q_SLOW_VAL
> Does that explain this? Or should I try again with code?
OK, I got your proposal now. However, there is still the issue that
setting the _Q_SLOW_VAL flag and the hash bucket are not atomic wrt each
other. It is possible a CPU has set the _Q_SLOW_VAL flag but not yet
filling in the hash bucket while another one is trying to look for it.
So we need to have some kind of synchronization mechanism to let the
lookup CPU know when is a good time to look up.
One possibility is to delay setting _Q_SLOW_VAL until the hash bucket is
set up. Maybe we can make that work.
Cheers,
Longman
On Thu, Apr 02, 2015 at 12:28:30PM -0400, Waiman Long wrote:
> On 04/01/2015 05:03 PM, Peter Zijlstra wrote:
> >On Wed, Apr 01, 2015 at 03:58:58PM -0400, Waiman Long wrote:
> >>On 04/01/2015 02:48 PM, Peter Zijlstra wrote:
> >>I am sorry that I don't quite get what you mean here. My point is that in
> >>the hashing step, a cpu will need to scan an empty bucket to put the lock
> >>in. In the interim, an previously used bucket before the empty one may get
> >>freed. In the lookup step for that lock, the scanning will stop because of
> >>an empty bucket in front of the target one.
> >Right, that's broken. So we need to do something else to limit the
> >lookup, because without that break, a lookup that needs to iterate the
> >entire array in order to determine -ENOENT, which is expensive.
> >
> >So my alternative proposal is that IFF we can guarantee that every
> >lookup will succeed -- the entry we're looking for is always there, we
> >don't need the break on empty but can probe until we find the entry.
> >This will be bound in cost to the same number if probes we required for
> >insertion and avoids the full array scan.
> >
> >Now I think we can indeed do this, if as said earlier we do not clear
> >the bucket on insert if the cmpxchg succeeds, in that case the unlock
> >will observe _Q_SLOW_VAL and do the lookup, the lookup will then find
> >the entry. And we then need the unlock to clear the entry.
> >_Q_SLOW_VAL
> >Does that explain this? Or should I try again with code?
>
> OK, I got your proposal now. However, there is still the issue that setting
> the _Q_SLOW_VAL flag and the hash bucket are not atomic wrt each other.
So? They're strictly ordered, that's sufficient. We first hash the lock,
then we set _Q_SLOW_VAL. There's a full memory barrier between them.
> It
> is possible a CPU has set the _Q_SLOW_VAL flag but not yet filling in the
> hash bucket while another one is trying to look for it.
Nope. The unlock side does an xchg() on the locked value first, xchg
also implies a full barrier, so that guarantees that if we observe
_Q_SLOW_VAL we must also observe the hash bucket with the lock value.
> So we need to have
> some kind of synchronization mechanism to let the lookup CPU know when is a
> good time to look up.
No, its all already ordered and working.
pv_wait_head():
pv_hash()
/* MB as per cmpxchg */
cmpxchg(&l->locked, _Q_LOCKED_VAL, _Q_SLOW_VAL);
VS
__pv_queue_spin_unlock():
if (xchg(&l->locked, 0) != _Q_SLOW_VAL)
return;
/* MB as per xchg */
pv_hash_find(lock);
On Thu, Apr 02, 2015 at 07:20:57PM +0200, Peter Zijlstra wrote:
> pv_wait_head():
>
> pv_hash()
> /* MB as per cmpxchg */
> cmpxchg(&l->locked, _Q_LOCKED_VAL, _Q_SLOW_VAL);
>
> VS
>
> __pv_queue_spin_unlock():
>
> if (xchg(&l->locked, 0) != _Q_SLOW_VAL)
> return;
>
> /* MB as per xchg */
> pv_hash_find(lock);
>
>
Something like so.. compile tested only.
I took out the LFSR because that was likely over engineering from my
side :-)
--- a/kernel/locking/qspinlock_paravirt.h
+++ b/kernel/locking/qspinlock_paravirt.h
@@ -2,6 +2,8 @@
#error "do not include this file"
#endif
+#include <linux/hash.h>
+
/*
* Implement paravirt qspinlocks; the general idea is to halt the vcpus instead
* of spinning them.
@@ -107,7 +109,84 @@ static void pv_kick_node(struct mcs_spin
pv_kick(pn->cpu);
}
-static DEFINE_PER_CPU(struct qspinlock *, __pv_lock_wait);
+/*
+ * Hash table using open addressing with an linear probe sequence.
+ *
+ * Since we should not be holding locks from NMI context (very rare indeed) the
+ * max load factor is 0.75, which is around the point where open adressing
+ * breaks down.
+ *
+ * Instead of probing just the immediate bucket we probe all buckets in the
+ * same cacheline.
+ *
+ * http://en.wikipedia.org/wiki/Hash_table#Open_addressing
+ *
+ */
+
+struct pv_hash_bucket {
+ struct qspinlock *lock;
+ int cpu;
+};
+
+/*
+ * XXX dynamic allocate using nr_cpu_ids instead...
+ */
+#define PV_LOCK_HASH_BITS (2 + NR_CPUS_BITS)
+
+#if PV_LOCK_HASH_BITS < 6
+#undef PV_LOCK_HASH_BITS
+#define PB_LOCK_HASH_BITS 6
+#endif
+
+#define PV_LOCK_HASH_SIZE (1 << PV_LOCK_HASH_BITS)
+
+static struct pv_hash_bucket __pv_lock_hash[PV_LOCK_HASH_SIZE] ____cacheline_aligned;
+
+#define PV_HB_PER_LINE (SMP_CACHE_BYTES / sizeof(struct pv_hash_bucket))
+
+static inline u32 hash_align(u32 hash)
+{
+ return hash & ~(PV_HB_PER_LINE - 1);
+}
+
+#define for_each_hash_bucket(hb, off, hash) \
+ for (hash = hash_align(hash), off = 0, hb = &__pv_lock_hash[hash + off];\
+ off < PV_LOCK_HASH_SIZE; \
+ off++, hb = &__pv_lock_hash[(hash + off) % PV_LOCK_HASH_SIZE])
+
+static struct pv_hash_bucket *pv_hash_insert(struct qspinlock *lock)
+{
+ u32 offset, hash = hash_ptr(lock, PV_LOCK_HASH_BITS);
+ struct pv_hash_bucket *hb;
+
+ for_each_hash_bucket(hb, offset, hash) {
+ if (!cmpxchg(&hb->lock, NULL, lock)) {
+ WRITE_ONCE(hb->cpu, smp_processor_id());
+ return hb;
+ }
+ }
+
+ /*
+ * Hard assumes there is an empty bucket somewhere.
+ */
+ BUG();
+}
+
+static struct pv_hash_bucket *pv_hash_find(struct qspinlock *lock)
+{
+ u32 offset, hash = hash_ptr(lock, PV_LOCK_HASH_BITS);
+ struct pv_hash_bucket *hb;
+
+ for_each_hash_bucket(hb, offset, hash) {
+ if (READ_ONCE(hb->lock) == lock)
+ return hb;
+ }
+
+ /*
+ * Hard assumes we _WILL_ find a match.
+ */
+ BUG();
+}
/*
* Wait for l->locked to become clear; halt the vcpu after a short spin.
@@ -116,7 +195,9 @@ static DEFINE_PER_CPU(struct qspinlock *
static void pv_wait_head(struct qspinlock *lock)
{
struct __qspinlock *l = (void *)lock;
+ struct pv_hash_bucket *hb = NULL;
int loop;
+ u8 o;
for (;;) {
for (loop = SPIN_THRESHOLD; loop; loop--) {
@@ -126,29 +207,47 @@ static void pv_wait_head(struct qspinloc
cpu_relax();
}
- this_cpu_write(__pv_lock_wait, lock);
- /*
- * __pv_lock_wait must be set before setting _Q_SLOW_VAL
- *
- * [S] __pv_lock_wait = lock [RmW] l = l->locked = 0
- * MB MB
- * [S] l->locked = _Q_SLOW_VAL [L] __pv_lock_wait
- *
- * Matches the xchg() in pv_queue_spin_unlock().
- */
- if (!cmpxchg(&l->locked, _Q_LOCKED_VAL, _Q_SLOW_VAL))
- goto done;
+ if (!hb) {
+ hb = pv_hash_insert(lock);
+ /*
+ * hb must be set before setting _Q_SLOW_VAL
+ *
+ * [S] hb <- lock [RmW] l = l->locked = 0
+ * MB MB
+ * [RmW] l->locked ?= _Q_SLOW_VAL [L] hb
+ *
+ * Matches the xchg() in pv_queue_spin_unlock().
+ */
+ o = cmpxchg(&l->locked, _Q_LOCKED_VAL, _Q_SLOW_VAL);
+ if (!o) {
+ /*
+ * The lock got unlocked before we could set
+ * _Q_SLOW_VAL, we must unhash ourselves.
+ */
+ WRITE_ONCE(hb->lock, NULL);
+ goto done;
+ }
+ BUG_ON(o != _Q_LOCKED_VAL);
+ /*
+ * At this point _Q_SLOW_VAL is visible and the unlock
+ * will do the lookup. The lookup hard relies on the
+ * entry being visible -- which it should be. Unlock
+ * will unhash for us.
+ */
+ }
pv_wait(&l->locked, _Q_SLOW_VAL);
+ /*
+ * We can get spurious wakeups from interrupts, cycle back.
+ */
}
done:
- this_cpu_write(__pv_lock_wait, NULL);
-
/*
* Lock is unlocked now; the caller will acquire it without waiting.
* As with pv_wait_node() we rely on the caller to do a load-acquire
* for us.
*/
+ return;
}
/*
@@ -158,20 +257,20 @@ static void pv_wait_head(struct qspinloc
void __pv_queue_spin_unlock(struct qspinlock *lock)
{
struct __qspinlock *l = (void *)lock;
- int cpu;
+ struct pv_hash_bucket *hb;
if (xchg(&l->locked, 0) != _Q_SLOW_VAL)
return;
/*
* At this point the memory pointed at by lock can be freed/reused,
- * however we can still use the pointer value to search in our cpu
- * array.
+ * however we can still use the pointer value to search in our hash
+ * table.
*
- * XXX: get rid of this loop
+ * Also, if we observe _Q_SLOW_VAL we _must_ now observe 'our' hash
+ * bucket. See pv_wait_head().
*/
- for_each_possible_cpu(cpu) {
- if (per_cpu(__pv_lock_wait, cpu) == lock)
- pv_kick(cpu);
- }
+ hb = pv_hash_find(lock);
+ pv_kick(hb->cpu);
+ WRITE_ONCE(hb->lock, NULL); /* unhash */
}
On 04/02/2015 03:48 PM, Peter Zijlstra wrote:
> On Thu, Apr 02, 2015 at 07:20:57PM +0200, Peter Zijlstra wrote:
>> pv_wait_head():
>>
>> pv_hash()
>> /* MB as per cmpxchg */
>> cmpxchg(&l->locked, _Q_LOCKED_VAL, _Q_SLOW_VAL);
>>
>> VS
>>
>> __pv_queue_spin_unlock():
>>
>> if (xchg(&l->locked, 0) != _Q_SLOW_VAL)
>> return;
>>
>> /* MB as per xchg */
>> pv_hash_find(lock);
>>
>>
>
> Something like so.. compile tested only.
>
> I took out the LFSR because that was likely over engineering from my
> side :-)
>
> --- a/kernel/locking/qspinlock_paravirt.h
> +++ b/kernel/locking/qspinlock_paravirt.h
> @@ -2,6 +2,8 @@
> #error "do not include this file"
> #endif
>
> +#include<linux/hash.h>
> +
> /*
> * Implement paravirt qspinlocks; the general idea is to halt the vcpus instead
> * of spinning them.
> @@ -107,7 +109,84 @@ static void pv_kick_node(struct mcs_spin
> pv_kick(pn->cpu);
> }
>
> -static DEFINE_PER_CPU(struct qspinlock *, __pv_lock_wait);
> +/*
> + * Hash table using open addressing with an linear probe sequence.
> + *
> + * Since we should not be holding locks from NMI context (very rare indeed) the
> + * max load factor is 0.75, which is around the point where open adressing
> + * breaks down.
> + *
> + * Instead of probing just the immediate bucket we probe all buckets in the
> + * same cacheline.
> + *
> + * http://en.wikipedia.org/wiki/Hash_table#Open_addressing
> + *
> + */
> +
> +struct pv_hash_bucket {
> + struct qspinlock *lock;
> + int cpu;
> +};
> +
> +/*
> + * XXX dynamic allocate using nr_cpu_ids instead...
> + */
> +#define PV_LOCK_HASH_BITS (2 + NR_CPUS_BITS)
> +
> +#if PV_LOCK_HASH_BITS< 6
> +#undef PV_LOCK_HASH_BITS
> +#define PB_LOCK_HASH_BITS 6
> +#endif
> +
> +#define PV_LOCK_HASH_SIZE (1<< PV_LOCK_HASH_BITS)
> +
> +static struct pv_hash_bucket __pv_lock_hash[PV_LOCK_HASH_SIZE] ____cacheline_aligned;
> +
> +#define PV_HB_PER_LINE (SMP_CACHE_BYTES / sizeof(struct pv_hash_bucket))
> +
> +static inline u32 hash_align(u32 hash)
> +{
> + return hash& ~(PV_HB_PER_LINE - 1);
> +}
> +
> +#define for_each_hash_bucket(hb, off, hash) \
> + for (hash = hash_align(hash), off = 0, hb =&__pv_lock_hash[hash + off];\
> + off< PV_LOCK_HASH_SIZE; \
> + off++, hb =&__pv_lock_hash[(hash + off) % PV_LOCK_HASH_SIZE])
> +
> +static struct pv_hash_bucket *pv_hash_insert(struct qspinlock *lock)
> +{
> + u32 offset, hash = hash_ptr(lock, PV_LOCK_HASH_BITS);
> + struct pv_hash_bucket *hb;
> +
> + for_each_hash_bucket(hb, offset, hash) {
> + if (!cmpxchg(&hb->lock, NULL, lock)) {
> + WRITE_ONCE(hb->cpu, smp_processor_id());
> + return hb;
> + }
> + }
> +
> + /*
> + * Hard assumes there is an empty bucket somewhere.
> + */
> + BUG();
> +}
> +
> +static struct pv_hash_bucket *pv_hash_find(struct qspinlock *lock)
> +{
> + u32 offset, hash = hash_ptr(lock, PV_LOCK_HASH_BITS);
> + struct pv_hash_bucket *hb;
> +
> + for_each_hash_bucket(hb, offset, hash) {
> + if (READ_ONCE(hb->lock) == lock)
> + return hb;
> + }
> +
> + /*
> + * Hard assumes we _WILL_ find a match.
> + */
> + BUG();
> +}
>
> /*
> * Wait for l->locked to become clear; halt the vcpu after a short spin.
> @@ -116,7 +195,9 @@ static DEFINE_PER_CPU(struct qspinlock *
> static void pv_wait_head(struct qspinlock *lock)
> {
> struct __qspinlock *l = (void *)lock;
> + struct pv_hash_bucket *hb = NULL;
> int loop;
> + u8 o;
>
> for (;;) {
> for (loop = SPIN_THRESHOLD; loop; loop--) {
> @@ -126,29 +207,47 @@ static void pv_wait_head(struct qspinloc
> cpu_relax();
> }
>
> - this_cpu_write(__pv_lock_wait, lock);
> - /*
> - * __pv_lock_wait must be set before setting _Q_SLOW_VAL
> - *
> - * [S] __pv_lock_wait = lock [RmW] l = l->locked = 0
> - * MB MB
> - * [S] l->locked = _Q_SLOW_VAL [L] __pv_lock_wait
> - *
> - * Matches the xchg() in pv_queue_spin_unlock().
> - */
> - if (!cmpxchg(&l->locked, _Q_LOCKED_VAL, _Q_SLOW_VAL))
> - goto done;
> + if (!hb) {
> + hb = pv_hash_insert(lock);
> + /*
> + * hb must be set before setting _Q_SLOW_VAL
> + *
> + * [S] hb<- lock [RmW] l = l->locked = 0
> + * MB MB
> + * [RmW] l->locked ?= _Q_SLOW_VAL [L] hb
> + *
> + * Matches the xchg() in pv_queue_spin_unlock().
> + */
> + o = cmpxchg(&l->locked, _Q_LOCKED_VAL, _Q_SLOW_VAL);
> + if (!o) {
> + /*
> + * The lock got unlocked before we could set
> + * _Q_SLOW_VAL, we must unhash ourselves.
> + */
> + WRITE_ONCE(hb->lock, NULL);
> + goto done;
> + }
> + BUG_ON(o != _Q_LOCKED_VAL);
> + /*
> + * At this point _Q_SLOW_VAL is visible and the unlock
> + * will do the lookup. The lookup hard relies on the
> + * entry being visible -- which it should be. Unlock
> + * will unhash for us.
> + */
> + }
>
> pv_wait(&l->locked, _Q_SLOW_VAL);
> + /*
> + * We can get spurious wakeups from interrupts, cycle back.
> + */
> }
> done:
> - this_cpu_write(__pv_lock_wait, NULL);
> -
> /*
> * Lock is unlocked now; the caller will acquire it without waiting.
> * As with pv_wait_node() we rely on the caller to do a load-acquire
> * for us.
> */
> + return;
> }
>
> /*
> @@ -158,20 +257,20 @@ static void pv_wait_head(struct qspinloc
> void __pv_queue_spin_unlock(struct qspinlock *lock)
> {
> struct __qspinlock *l = (void *)lock;
> - int cpu;
> + struct pv_hash_bucket *hb;
>
> if (xchg(&l->locked, 0) != _Q_SLOW_VAL)
> return;
>
> /*
> * At this point the memory pointed at by lock can be freed/reused,
> - * however we can still use the pointer value to search in our cpu
> - * array.
> + * however we can still use the pointer value to search in our hash
> + * table.
> *
> - * XXX: get rid of this loop
> + * Also, if we observe _Q_SLOW_VAL we _must_ now observe 'our' hash
> + * bucket. See pv_wait_head().
> */
> - for_each_possible_cpu(cpu) {
> - if (per_cpu(__pv_lock_wait, cpu) == lock)
> - pv_kick(cpu);
> - }
> + hb = pv_hash_find(lock);
> + pv_kick(hb->cpu);
> + WRITE_ONCE(hb->lock, NULL); /* unhash */
> }
Thanks for the code. I am working on my own version, too. Will need to
run some tests to verify the correctness of the code. Hopefully I have
something for you to review early next week.
Cheers,
Longman
On Thu, Apr 02, 2015 at 09:48:34PM +0200, Peter Zijlstra wrote:
> @@ -158,20 +257,20 @@ static void pv_wait_head(struct qspinloc
> void __pv_queue_spin_unlock(struct qspinlock *lock)
> {
> struct __qspinlock *l = (void *)lock;
> + struct pv_hash_bucket *hb;
>
> if (xchg(&l->locked, 0) != _Q_SLOW_VAL)
> return;
>
> /*
> * At this point the memory pointed at by lock can be freed/reused,
> + * however we can still use the pointer value to search in our hash
> + * table.
> *
> + * Also, if we observe _Q_SLOW_VAL we _must_ now observe 'our' hash
> + * bucket. See pv_wait_head().
> */
> + hb = pv_hash_find(lock);
> + pv_kick(hb->cpu);
> + WRITE_ONCE(hb->lock, NULL); /* unhash */
> }
So I _think_ I found a problem with this approach :/
If, as per the above, the lock does indeed get freed, it can get
re-allocated and stuck in the hash table (again) before we get the
lookup and unhash it.
I'll have to ponder that a bit more.