On Tue, Sep 06, 2016 at 03:58:00AM +0200, Andreas Mohr wrote:
> Hi,
>
> [no properly binding reference via In-Reply-To: available thus manually
> re-creating, sorry]
>
> https://lkml.org/lkml/2016/9/5/832
Subscribe to lkml already.. also lkml.org is near useless these days,
please use any other archive.
> Two thoughts:
>
> ***multiple locks
>
> Don't have much insight into this
> (didn't spend much thinking on this),
> but of course it's unfortunate
> that two lock types need to be serviced
> rather than having the atomic handling exchange area/section
> be sufficiently guarded by one lock only
> (the question here could possibly be:
> what kind of currently existing
> structural disadvantage/layer distribution
> prevents us from being able to
> have things simply serviced
> within one granular lock area only?)
percpu rwsem is a sleeping lock, flc_flock is not. flc_flock also nests
under other non-sleeping locks, and therefore cannot (trivially) be made
a sleeping lock.
This is in the Changelog.
Furthermore, in the fast path of percpu_{down,up}_read() are exactly 0
atomic/serializing instructions.
> ***lock handling
>
> > + percpu_down_read(&file_rwsem);
> > spin_lock(&ctx->flc_lock);
>
>
> > spin_unlock(&ctx->flc_lock);
> > + percpu_up_read(&file_rwsem);
>
>
> These are repeated multiple times in this commit, thus error-prone.
>
> A possibly good way to commit-micro-manage this would be:
> 1. commit shoves things into a newly created encapsulation/wrapper helper
> stuff_lock(&flc_lock); /* <---- naming surely can be improved here */
No, because not all instances of flc_flock need the percpu-rwsem held,
creating such a wrapper could mistakenly create the impression it
should.
> That way it is pretty much guaranteed that:
> a) neither one nor the other lock type
> will get forgotten later, at a specific use site
That's what we have lockdep_assert_held() for. Note how this patch also
introduces percpu_rwsem_assert_held() usage, this insures we shall never
'forget' the appropriate locking.
> b) lock order will be correctly maintained at all times
> (AB-BA deadlock......)
lockdep is rather good at finding those, also might_sleep() debugging
would trivially catch those since percpu-rwsem is a sleeping lock while
fcl_flock is not.
