Messages in this thread | | | Date | Sun, 24 Oct 2010 17:44:24 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH 11/18] fs: Introduce per-bucket inode hash locks |
| |
On Tue, 19 Oct 2010, Christoph Hellwig wrote:
> On Tue, Oct 19, 2010 at 06:00:57PM +1100, Nick Piggin wrote: > > But it is still "magic". Because you don't even know whether it > > is a spin or sleeping lock, let alone whether it is irq or bh safe. > > You get far more information seeing a bit_spin_lock(0, &hlist) call > > than hlist_lock().
Errm, when hlist_lock() has proper documentation than it should not be rocket science to figure out what it does.
And if you use bit 0 of hlist then you better have helper functions to access it anyway. We do that with other data types which (ab)use the lower two bits of pointers.
> To get back a bit to the point: > > - we have a new bl_hlist sturcture which combines a hash list and a > lock embedded into the head > - the reason why we do it is to be able to use a bitlock
And if you design that structure clever, then simple dereferencing of it (w/o casting magic) should make the compiler barf. So you are forced to use the helper functions.
> Furthermore it allows the RT people to simply throw a mutex into the > head and everything keeps working without touching a sinlge line of > code outside of hlist_bl.h.
Yes, please use proper helper functions. Having to change code is a horror for RT, when we can get away with a single change in a header file.
Aside of RT there is another advantage of being able to change the lock implementation at a single place: you can change it to a real spinlock and have lockdep coverage of that code. I fundamentally hate bit_spin_locks for sneaking around lockdep.
Thanks,
tglx
| |