linux-kernel - Re: [PATCH 11/18] fs: Introduce per-bucket inode hash locks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20101020031105.GI3740@amd>
Date:	Wed, 20 Oct 2010 14:11:05 +1100
From:	Nick Piggin <npiggin@...nel.dk>
To:	Christoph Hellwig <hch@...radead.org>
Cc:	Nick Piggin <npiggin@...nel.dk>, Andi Kleen <andi@...stfloor.org>,
	Dave Chinner <david@...morbit.com>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	tglx@...utronix.de
Subject: Re: [PATCH 11/18] fs: Introduce per-bucket inode hash locks

On Tue, Oct 19, 2010 at 12:50:44PM -0400, 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().
> > 
> > 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.

Or sleeping locks. And there is no reason not to have irqsave or
bhsave versions of bitlocks (which could enable interrupts etc on
a contended lock) but they just haven't been put in yet. If someone
needs them, and implements them in the bitlock code, everybody can
use them.

> 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

To use bit 0 as a lock bit, yes. But if another use for the API was
required like to store some other OOB state (not a lock), then some
assertions can come out of the hlist_bl code quite easily and it
can be extended to have an arbitrary bit there. (or several arbitrary
bits if the pointer is aligned correctly).

> 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.

What do you mean zero abstraction? It changes all the hlist operations
to handle bit 0 as lock bit.

Locking and unlocking a bit, we already have abstractions for. They work
really well, and nobody has to check the implementation to know what
they do.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/