Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1477016 > unrolled thread

Re: [PATCH 2/7] fs/locks: Replace lg_global with a percpu-rwsem

Back to article view | Back to linux.kernel

Contents

  Re: [PATCH 2/7] fs/locks: Replace lg_global with a percpu-rwsem Andreas Mohr <andi@lisas.de> - 2016-09-06 04:00 +0200
    Re: [PATCH 2/7] fs/locks: Replace lg_global with a percpu-rwsem Peter Zijlstra <peterz@infradead.org> - 2016-09-06 10:30 +0200
      Re: [PATCH 2/7] fs/locks: Replace lg_global with a percpu-rwsem Andreas Mohr <andi@lisas.de> - 2016-09-06 10:40 +0200
        Re: [PATCH 2/7] fs/locks: Replace lg_global with a percpu-rwsem Peter Zijlstra <peterz@infradead.org> - 2016-09-06 11:10 +0200

#1477016 — Re: [PATCH 2/7] fs/locks: Replace lg_global with a percpu-rwsem

Hi,

[no properly binding reference via In-Reply-To: available thus manually re-creating, sorry]

https://lkml.org/lkml/2016/9/5/832


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?)


***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 */
2. [this commit]
   extend encapsulation/wrapper helper
   to be servicing file_rwsem, too:
   stuff_lock(&flc_lock, &file_rwsem);

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
b) lock order will be correctly maintained at all times
   (AB-BA deadlock......)


[or, IOW, you get some symmetry/consistency malus points
for having provided
a relatively large amount of LOC extensions
which put certain amounts of stress on
code implementation quality/maintainability ;)]

HTH,

Andreas Mohr

[toc] | [next] | [standalone]

#1477150

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.

[toc] | [prev] | [next] | [standalone]

#1477158

On Tue, Sep 06, 2016 at 10:23:28AM +0200, Peter Zijlstra wrote:
> 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.

I cannot count the number of times that I did so
(sometimes directly failing, sometimes getting dropped after few weeks).
Hopefully with that other address things will now work out ok...


There might be further archives (other than gmane.org or some such),
however so far lkml.org seemed to be quite ok
and with IMHO better usability than some others
(although it has degraded a lot indeed in recent times,
judging from
many server/connection issues and
not listing details any more due to SPAM etc.).


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

OK, that aspect sounds valid.
However with a helper appropriately named to be focussing on
that use case (protecting that section communication),
it might be less of a concern.


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

So perhaps the only thing left to argue on my side is
that this way it would be
n+1 vs. n protection mechanisms ;)
(with some of those n+1 mechanisms
known to be failing one time or another...)

Thanks,

Andreas Mohr

[toc] | [prev] | [next] | [standalone]

#1477179

On Tue, Sep 06, 2016 at 10:36:01AM +0200, Andreas Mohr wrote:
> There might be further archives (other than gmane.org or some such),
> however so far lkml.org seemed to be quite ok
> and with IMHO better usability than some others

Agreed, in that I liked the interface best too, however:

> (although it has degraded a lot indeed in recent times,
> judging from
> many server/connection issues and
> not listing details any more due to SPAM etc.).

this has gotten to the point where it often simply doesn't show messages
anymore, threads are incomplete etc..

> > > 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.
> 
> OK, that aspect sounds valid.
> However with a helper appropriately named to be focussing on
> that use case (protecting that section communication),
> it might be less of a concern.

Dunno, I'd struggling to come up with a sensible name for such a
construct. I'll leave that to others. If it really is desired we can
always do so later.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web