Re: [PATCH v2] mm: workingset: replace IRQ-off check with a lockdep assert.

[Johannes Weiner <hannes@cmpxchg.org>]

On Mon, Feb 11, 2019 at 08:13:45PM +0100, Sebastian Andrzej Siewior wrote:
> On 2019-02-11 13:53:18 [-0500], Johannes Weiner wrote:
> > On Mon, Feb 11, 2019 at 12:38:29PM +0100, Sebastian Andrzej Siewior wrote:
> > > Commit
> > > 
> > >   68d48e6a2df57 ("mm: workingset: add vmstat counter for shadow nodes")
> > > 
> > > introduced an IRQ-off check to ensure that a lock is held which also
> > > disabled interrupts. This does not work the same way on -RT because none
> > > of the locks, that are held, disable interrupts.
> > > Replace this check with a lockdep assert which ensures that the lock is
> > > held.
> > > 
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > 
> > I'm not against checking for the lock, but if IRQs aren't disabled,
> > what ensures __mod_lruvec_state() is safe?
> 
> how do you define safe? I've been looking for dependencies of
> __mod_lruvec_state() but found only that the lock is held during the RMW
> operation with WORKINGSET_NODES idx.

These stat functions are not allowed to nest, and the executing thread
cannot migrate to another CPU during the operation, otherwise they
corrupt the state they're modifying.

They are called from interrupt handlers, such as when NR_WRITEBACK is
decreased. Thus workingset_node_update() must exclude preemption from
irq handlers on the local CPU.

They rely on IRQ-disabling to also disable CPU migration.

> >                                            I'm guessing it's because
> > preemption is disabled and irq handlers are punted to process context.
> preemption is enabled and IRQ are processed in forced-threaded mode.

That doesn't sound safe.