| Message ID | 20150319122536.GD11574@worktop.ger.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 19, 2015 at 01:25:36PM +0100, Peter Zijlstra wrote: > +static struct qspinlock **pv_hash(struct qspinlock *lock) > +{ > + u32 hash = hash_ptr(lock, PV_LOCK_HASH_BITS); > + struct pv_hash_bucket *hb, *end; > + > + if (!hash) > + hash = 1; > + > + hb = &__pv_lock_hash[hash_align(hash)]; > + for (;;) { > + for (end = hb + PV_HB_PER_LINE; hb < end; hb++) { > + if (cmpxchg(&hb->lock, NULL, HB_RESERVED)) { That should be: !cmpxchg(), bit disturbing that that booted. > + WRITE_ONCE(hb->cpu, smp_processor_id()); > + /* > + * Since we must read lock first and cpu > + * second, we must write cpu first and lock > + * second, therefore use HB_RESERVE to mark an > + * entry in use before writing the values. > + * > + * This can cause hb_hash_find() to not find a > + * cpu even though _Q_SLOW_VAL, this is not a > + * problem since we re-check l->locked before > + * going to sleep and the unlock will have > + * cleared l->locked already. > + */ > + smp_wmb(); /* matches rmb from pv_hash_find */ > + WRITE_ONCE(hb->lock, lock); > + goto done; > + } > + } > + > + hash = lfsr(hash, PV_LOCK_HASH_BITS); > + hb = &__pv_lock_hash[hash_align(hash)]; > + } > + > +done: > + return &hb->lock; > +} -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(-) This is a much better alternative. > --- /dev/null > +++ b/include/linux/lfsr.h > @@ -0,0 +1,49 @@ > +#ifndef _LINUX_LFSR_H > +#define _LINUX_LFSR_H > + > +/* > + * Simple Binary Galois Linear Feedback Shift Register > + * > + * http://en.wikipedia.org/wiki/Linear_feedback_shift_register > + * > + */ > + > +extern void __lfsr_needs_more_taps(void); > + > +static __always_inline u32 lfsr_taps(int bits) > +{ > + if (bits == 1) return 0x0001; > + if (bits == 2) return 0x0001; > + if (bits == 3) return 0x0003; > + if (bits == 4) return 0x0009; > + if (bits == 5) return 0x0012; > + if (bits == 6) return 0x0021; > + if (bits == 7) return 0x0041; > + if (bits == 8) return 0x008E; > + if (bits == 9) return 0x0108; > + if (bits == 10) return 0x0204; > + if (bits == 11) return 0x0402; > + if (bits == 12) return 0x0829; > + if (bits == 13) return 0x100D; > + if (bits == 14) return 0x2015; > + > + /* > + * For more taps see: > + * http://users.ece.cmu.edu/~koopman/lfsr/index.html > + */ > + __lfsr_needs_more_taps(); > + > + return 0; > +} > + > +static inline u32 lfsr(u32 val, int bits) > +{ > + u32 bit = val& 1; > + > + val>>= 1; > + if (bit) > + val ^= lfsr_taps(bits); > + return val; > +} > + > +#endif /* _LINUX_LFSR_H */ > --- a/kernel/locking/qspinlock_paravirt.h > +++ b/kernel/locking/qspinlock_paravirt.h > @@ -2,6 +2,9 @@ > #error "do not include this file" > #endif > > +#include<linux/hash.h> > +#include<linux/lfsr.h> > + > /* > * Implement paravirt qspinlocks; the general idea is to halt the vcpus instead > * of spinning them. > @@ -107,7 +110,120 @@ 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 LFSR 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 addressing > + * 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 > + * > + */ > + > +#define HB_RESERVED ((struct qspinlock *)1) > + > +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) > + As said here, we should make it dynamically allocated depending on num_possible_cpus(). > +#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); > +} > + > +static struct qspinlock **pv_hash(struct qspinlock *lock) > +{ > + u32 hash = hash_ptr(lock, PV_LOCK_HASH_BITS); > + struct pv_hash_bucket *hb, *end; > + > + if (!hash) > + hash = 1; > + > + hb =&__pv_lock_hash[hash_align(hash)]; > + for (;;) { > + for (end = hb + PV_HB_PER_LINE; hb< end; hb++) { > + if (cmpxchg(&hb->lock, NULL, HB_RESERVED)) { > + WRITE_ONCE(hb->cpu, smp_processor_id()); > + /* > + * Since we must read lock first and cpu > + * second, we must write cpu first and lock > + * second, therefore use HB_RESERVE to mark an > + * entry in use before writing the values. > + * > + * This can cause hb_hash_find() to not find a > + * cpu even though _Q_SLOW_VAL, this is not a > + * problem since we re-check l->locked before > + * going to sleep and the unlock will have > + * cleared l->locked already. > + */ > + smp_wmb(); /* matches rmb from pv_hash_find */ > + WRITE_ONCE(hb->lock, lock); > + goto done; > + } > + } > + > + hash = lfsr(hash, PV_LOCK_HASH_BITS); > + hb =&__pv_lock_hash[hash_align(hash)]; > + } > + > +done: > + return&hb->lock; > +} > + > +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; > + > + if (l == lock) { > + smp_rmb(); /* matches wmb from pv_hash() */ > + cpu = READ_ONCE(hb->cpu); > + goto done; > + } > + } > + > + hash = lfsr(hash, PV_LOCK_HASH_BITS); > + hb =&__pv_lock_hash[hash_align(hash)]; > + } > +done: > + return cpu; > +} > We should probably abstract out the pv_hash and pv_hash_find into generic functions that can be put into header like hash.h instead of doing it locally here. -Longman -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ;-) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- /dev/null +++ b/include/linux/lfsr.h @@ -0,0 +1,49 @@ +#ifndef _LINUX_LFSR_H +#define _LINUX_LFSR_H + +/* + * Simple Binary Galois Linear Feedback Shift Register + * + * http://en.wikipedia.org/wiki/Linear_feedback_shift_register + * + */ + +extern void __lfsr_needs_more_taps(void); + +static __always_inline u32 lfsr_taps(int bits) +{ + if (bits == 1) return 0x0001; + if (bits == 2) return 0x0001; + if (bits == 3) return 0x0003; + if (bits == 4) return 0x0009; + if (bits == 5) return 0x0012; + if (bits == 6) return 0x0021; + if (bits == 7) return 0x0041; + if (bits == 8) return 0x008E; + if (bits == 9) return 0x0108; + if (bits == 10) return 0x0204; + if (bits == 11) return 0x0402; + if (bits == 12) return 0x0829; + if (bits == 13) return 0x100D; + if (bits == 14) return 0x2015; + + /* + * For more taps see: + * http://users.ece.cmu.edu/~koopman/lfsr/index.html + */ + __lfsr_needs_more_taps(); + + return 0; +} + +static inline u32 lfsr(u32 val, int bits) +{ + u32 bit = val & 1; + + val >>= 1; + if (bit) + val ^= lfsr_taps(bits); + return val; +} + +#endif /* _LINUX_LFSR_H */ --- a/kernel/locking/qspinlock_paravirt.h +++ b/kernel/locking/qspinlock_paravirt.h @@ -2,6 +2,9 @@ #error "do not include this file" #endif +#include <linux/hash.h> +#include <linux/lfsr.h> + /* * Implement paravirt qspinlocks; the general idea is to halt the vcpus instead * of spinning them. @@ -107,7 +110,120 @@ 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 LFSR 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 addressing + * 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 + * + */ + +#define HB_RESERVED ((struct qspinlock *)1) + +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); +} + +static struct qspinlock **pv_hash(struct qspinlock *lock) +{ + u32 hash = hash_ptr(lock, PV_LOCK_HASH_BITS); + struct pv_hash_bucket *hb, *end; + + if (!hash) + hash = 1; + + hb = &__pv_lock_hash[hash_align(hash)]; + for (;;) { + for (end = hb + PV_HB_PER_LINE; hb < end; hb++) { + if (cmpxchg(&hb->lock, NULL, HB_RESERVED)) { + WRITE_ONCE(hb->cpu, smp_processor_id()); + /* + * Since we must read lock first and cpu + * second, we must write cpu first and lock + * second, therefore use HB_RESERVE to mark an + * entry in use before writing the values. + * + * This can cause hb_hash_find() to not find a + * cpu even though _Q_SLOW_VAL, this is not a + * problem since we re-check l->locked before + * going to sleep and the unlock will have + * cleared l->locked already. + */ + smp_wmb(); /* matches rmb from pv_hash_find */ + WRITE_ONCE(hb->lock, lock); + goto done; + } + } + + hash = lfsr(hash, PV_LOCK_HASH_BITS); + hb = &__pv_lock_hash[hash_align(hash)]; + } + +done: + return &hb->lock; +} + +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; + + if (l == lock) { + smp_rmb(); /* matches wmb from pv_hash() */ + cpu = READ_ONCE(hb->cpu); + goto done; + } + } + + hash = lfsr(hash, PV_LOCK_HASH_BITS); + hb = &__pv_lock_hash[hash_align(hash)]; + } +done: + return cpu; +} /* * Wait for l->locked to become clear; halt the vcpu after a short spin. @@ -116,6 +232,7 @@ static DEFINE_PER_CPU(struct qspinlock * static void pv_wait_head(struct qspinlock *lock) { struct __qspinlock *l = (void *)lock; + struct qspinlock **lp = NULL; int loop; for (;;) { @@ -126,13 +243,13 @@ static void pv_wait_head(struct qspinloc cpu_relax(); } - this_cpu_write(__pv_lock_wait, lock); + lp = pv_hash(lock); /* - * __pv_lock_wait must be set before setting _Q_SLOW_VAL + * lp must be set before setting _Q_SLOW_VAL * - * [S] __pv_lock_wait = lock [RmW] l = l->locked = 0 + * [S] lp = lock [RmW] l = l->locked = 0 * MB MB - * [S] l->locked = _Q_SLOW_VAL [L] __pv_lock_wait + * [S] l->locked = _Q_SLOW_VAL [L] lp * * Matches the xchg() in pv_queue_spin_unlock(). */ @@ -142,7 +259,8 @@ static void pv_wait_head(struct qspinloc pv_wait(&l->locked, _Q_SLOW_VAL); } done: - this_cpu_write(__pv_lock_wait, NULL); + if (lp) + WRITE_ONCE(*lp, NULL); /* * Lock is unlocked now; the caller will acquire it without waiting. @@ -165,13 +283,10 @@ void __pv_queue_spin_unlock(struct qspin /* * 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. - * - * XXX: get rid of this loop + * however we can still use the pointer value to search in our hash + * table. */ - for_each_possible_cpu(cpu) { - if (per_cpu(__pv_lock_wait, cpu) == lock) - pv_kick(cpu); - } + cpu = pv_hash_find(lock); + if (cpu >= 0) + pv_kick(cpu); }