Re: [PATCH 2/3] mm: workingset: make shadow_lru_isolate() use locking suffix

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Wed, Jun 27, 2018 at 11:20:59AM +0200, Sebastian Andrzej Siewior wrote:
> On 2018-06-27 11:50:03 [+0300], Vladimir Davydov wrote:
> > > it is not asymmetric because a later patch makes it use
> > > spin_lock_irq(), too. If you use local_irq_disable() and a spin_lock()
> > > (like you suggest in 3/3 as well) then you separate the locking
> > > instruction. It works as expected on vanilla but break other locking
> > > implementations like those on RT.
> > 
> > As I said earlier, I don't like patch 3 either, because I find the
> > notion of list_lru::lock_irq flag abstruse since it doesn't make all
> > code paths taking the lock disable irq: list_lru_add/del use spin_lock
> > no matter whether the flag is set or not. That is, when you initialize a
> > list_lru and pass lock_irq=true, you'll have to keep in mind that it
> > only protects list_lru_walk, while list_lru_add/del must be called with
> > irq disabled by the caller. Disabling irq before list_lru_walk
> > explicitly looks much more straightforward IMO.
> 
> It helps to keep the locking annotation in one place. If it helps I
> could add the _irqsave() suffix to list_lru_add/del like it is already
> done in other places (in this file).

AFAIK local_irqsave/restore don't come for free so using them just to
keep the code clean doesn't seem to be reasonable.

> 
> > As for RT, it wouldn't need mm/workingset altogether AFAIU. 
> Why wouldn't it need it?

I may be wrong, but AFAIU RT kernel doesn't do swapping.

> 
> > Anyway, it's
> > rather unusual to care about out-of-the-tree patches when changing the
> > vanilla kernel code IMO. 
> The plan is not stay out-of-tree forever. And I don't intend to make
> impossible or hard to argue changes just for RT's sake. This is only to
> keep the correct locking context/primitives in one place and not
> scattered around.
> The only reason for the separation is that most users don't disable
> interrupts (one user does) and there a few places which already use
> irqsave() because they can be called from both places. This
> list_lru_walk() is the only which can't do so due to the callback it
> invokes. I could also add a different function (say
> list_lru_walk_one_irq()) which behaves like list_lru_walk_one() but does
> spin_lock_irq() instead.

That would look better IMHO. I mean, passing the flag as an argument to
__list_lru_walk_one and introducing list_lru_shrink_walk_irq.

> 
> > Using local_irq_disable + spin_lock instead of
> > spin_lock_irq is a typical pattern, and I don't see how changing this
> > particular place would help RT.
> It is not that typical. It is how the locking primitives work, yes, but
> they are not so many places that do so and those that did got cleaned
> up.

Missed that. There used to be a lot of places like that in the past.
I guess things have changed.

> 
> > > Also if the locking changes then the local_irq_disable() part will be
> > > forgotten like you saw in 1/3 of this series.
> > 
> > If the locking changes, we'll have to revise all list_lru users anyway.
> > Yeah, we missed it last time, but it didn't break anything, and it was
> > finally found and fixed (by you, thanks BTW).
> You are very welcome. But having the locking primitives in one place you
> would have less things to worry about.