lglock: introduce special lglock and brlock spin locks
in [Kernel]
|
Prev: Commit 9b8c4a0b215e603497daebe8ecbc9b1f0f035808 broken X11 pointer
Next: writeback: limit write_cache_pages integrity scanning to current EOF
From: Thomas Gleixner on 24 Jun 2010 14:20 On Thu, 24 Jun 2010, npiggin(a)suse.de wrote: > +#define DEFINE_LGLOCK(name) \ > + \ > + DEFINE_PER_CPU(arch_spinlock_t, name##_lock); \ Uuurgh. You want to make that an arch_spinlock ? Just to avoid the preempt_count overflow when you lock all cpu locks nested ? I'm really not happy about that, it's going to be a complete nightmare for RT. If you wanted to make this a present for RT giving the scalability stuff massive testing, then you failed miserably :) I know how to fix it, but can't we go for an approach which does not require massive RT patching again ? struct percpu_lock { spinlock_t lock; unsigned global_state; }; And let the lock function do: spin_lock(&pcp->lock); while (pcp->global_state) cpu_relax(); So the global lock side can take each single lock, modify the percpu "global state" and release the lock. On unlock you just need to reset the global state w/o taking the percpu lock and be done. I doubt that the extra conditional in the lock path is going to be relevant overhead, compared to the spin_lock it's noise. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Nick Piggin on 25 Jun 2010 02:30 On Thu, Jun 24, 2010 at 08:15:54PM +0200, Thomas Gleixner wrote: > On Thu, 24 Jun 2010, npiggin(a)suse.de wrote: > > > +#define DEFINE_LGLOCK(name) \ > > + \ > > + DEFINE_PER_CPU(arch_spinlock_t, name##_lock); \ > > Uuurgh. You want to make that an arch_spinlock ? Just to avoid the > preempt_count overflow when you lock all cpu locks nested ? Yep, and the lockdep wreckage too :) Actually it's nice to avoid the function call too (lglock/brlocks are already out of line). Calls aren't totally free, especially on small chips without RSBs. And even with RSBs it's helpful not to overflow them, although Nehalem seems to have 12-16 entries. > I'm really not happy about that, it's going to be a complete nightmare > for RT. If you wanted to make this a present for RT giving the > scalability stuff massive testing, then you failed miserably :) Heh, it's a present for -rt because the locking is quite isolated (I did the same thing with hashtable bitlocks, added a new structure for them, in case you prefer spinlocks than bit spinlocks there). -rt already changes locking primitives, so in the worst case you might have to tweak this. My previous patches were open coding these locks in fs/ so I can understand why that was a headache. > I know how to fix it, but can't we go for an approach which > does not require massive RT patching again ? > > struct percpu_lock { > spinlock_t lock; > unsigned global_state; > }; > > And let the lock function do: > > spin_lock(&pcp->lock); > while (pcp->global_state) > cpu_relax(); > > So the global lock side can take each single lock, modify the percpu > "global state" and release the lock. On unlock you just need to reset > the global state w/o taking the percpu lock and be done. > > I doubt that the extra conditional in the lock path is going to be > relevant overhead, compared to the spin_lock it's noise. Hmm. We need a smp_rmb() which costs a bit (on non-x86). For -rt you would need to do priority boosting too. reader lock: spin_lock(&pcp->rlock); if (unlikely(pcp->global_state)) { spin_unlock(&pcp->rlock); spin_lock(&wlock); spin_lock(&pcp->rlock); spin_unlock(&wlock); } else smp_rmb(); I think I'll keep it as is for now, it's hard enough to keep single threaded performance up. But it should be much easier to override this in -rt and I'll be happy to try restructuring things to help rt if and when it's possible. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Thomas Gleixner on 25 Jun 2010 06:00 On Fri, 25 Jun 2010, Nick Piggin wrote: > On Thu, Jun 24, 2010 at 08:15:54PM +0200, Thomas Gleixner wrote: > > On Thu, 24 Jun 2010, npiggin(a)suse.de wrote: > > > > > +#define DEFINE_LGLOCK(name) \ > > > + \ > > > + DEFINE_PER_CPU(arch_spinlock_t, name##_lock); \ > > > > Uuurgh. You want to make that an arch_spinlock ? Just to avoid the > > preempt_count overflow when you lock all cpu locks nested ? > > Yep, and the lockdep wreckage too :) > > Actually it's nice to avoid the function call too (lglock/brlocks > are already out of line). Calls aren't totally free, especially > on small chips without RSBs. And even with RSBs it's helpful not > to overflow them, although Nehalem seems to have 12-16 entries. > > > > I'm really not happy about that, it's going to be a complete nightmare > > for RT. If you wanted to make this a present for RT giving the > > scalability stuff massive testing, then you failed miserably :) > > Heh, it's a present for -rt because the locking is quite isolated > (I did the same thing with hashtable bitlocks, added a new structure > for them, in case you prefer spinlocks than bit spinlocks there). Sure, bitlocks are equally horrible. > -rt already changes locking primitives, so in the worst case you > might have to tweak this. My previous patches were open coding > these locks in fs/ so I can understand why that was a headache. I agree that having the code isolated makes my life easier, but I'm a bit worried about the various new locking primitives which pop up in all corners of the kernel. > I think I'll keep it as is for now, it's hard enough to keep single > threaded performance up. But it should be much easier to override > this in -rt and I'll be happy to try restructuring things to help rt > if and when it's possible. Ok, lets see how bad it gets :) Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Nick Piggin on 25 Jun 2010 06:20
On Fri, Jun 25, 2010 at 11:50:02AM +0200, Thomas Gleixner wrote: > On Fri, 25 Jun 2010, Nick Piggin wrote: > > On Thu, Jun 24, 2010 at 08:15:54PM +0200, Thomas Gleixner wrote: > > > On Thu, 24 Jun 2010, npiggin(a)suse.de wrote: > > > > > > > +#define DEFINE_LGLOCK(name) \ > > > > + \ > > > > + DEFINE_PER_CPU(arch_spinlock_t, name##_lock); \ > > > > > > Uuurgh. You want to make that an arch_spinlock ? Just to avoid the > > > preempt_count overflow when you lock all cpu locks nested ? > > > > Yep, and the lockdep wreckage too :) > > > > Actually it's nice to avoid the function call too (lglock/brlocks > > are already out of line). Calls aren't totally free, especially > > on small chips without RSBs. And even with RSBs it's helpful not > > to overflow them, although Nehalem seems to have 12-16 entries. > > > > > > > I'm really not happy about that, it's going to be a complete nightmare > > > for RT. If you wanted to make this a present for RT giving the > > > scalability stuff massive testing, then you failed miserably :) > > > > Heh, it's a present for -rt because the locking is quite isolated > > (I did the same thing with hashtable bitlocks, added a new structure > > for them, in case you prefer spinlocks than bit spinlocks there). > > Sure, bitlocks are equally horrible. > > > -rt already changes locking primitives, so in the worst case you > > might have to tweak this. My previous patches were open coding > > these locks in fs/ so I can understand why that was a headache. > > I agree that having the code isolated makes my life easier, but I'm a > bit worried about the various new locking primitives which pop up in > all corners of the kernel. Be sure to shout at people for it :). Locking primitives really need to be reviewed, and having them in common places lets other people use them too. > > I think I'll keep it as is for now, it's hard enough to keep single > > threaded performance up. But it should be much easier to override > > this in -rt and I'll be happy to try restructuring things to help rt > > if and when it's possible. > > Ok, lets see how bad it gets :) Ok good. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |