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(). > > Even if you do rename them to hlist_bit_spin_lock, etc. Then you need > to add variants for each type of locking a caller wants to do on it. > Ask Linus what he thinks about that. And why do we need all these versions? We never had irqsave or bhsave versions of bitlocks. The only thing we might eventually want are trylock and is_locked operations once we grow user of it. 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 Now your initial version exposed the ugly defaults of that to the user which is required to case the hash head to and unsigned long and use bit_spin_lock on bit 0 of it. There's zero abstraction and a lot internal gutting required there. The version I suggest and that dave implemented instead adds wrappers to call bit_spin_lock/unlock with thos conventions as helper that operate on the abstract type. This frees the caller from revinventing just those same wrappers, as done in your demo tree for the inode and dentry hashes. 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. To me that's very clearly preferable, and I'd really like to see a clearly reasoned argument against it. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html