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

[Johannes Weiner <hannes@cmpxchg.org>]

On Wed, Feb 13, 2019 at 10:27:54AM +0100, Sebastian Andrzej Siewior wrote:
> On 2019-02-11 16:02:08 [-0500], Johannes Weiner wrote:
> > > 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.
> 
> If everyone is taking the same lock (like i_pages.xa_lock) then there
> will not be two instances updating the same stat. The owner of the
> (sleeping)-spinlock will not be migrated to another CPU.

This might be true for this particular stat item, but they are general
VM statistics. They're assuredly not all taking the xa_lock.

> > 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.
> 
> Do you have an example for a code path to check NR_WRITEBACK?

end_page_writeback()
 test_clear_page_writeback()
   dec_lruvec_state(lruvec, NR_WRITEBACK)

> > They rely on IRQ-disabling to also disable CPU migration.
> The spinlock disables 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.
> 
> Do you have test-case or something I could throw at it and verify that
> this still works? So far nothing complains…

It's not easy to get the timing right on purpose, but we've seen in
production what happens when you don't protect these counter updates
from interrupts. See c3cc39118c36 ("mm: memcontrol: fix NR_WRITEBACK
leak in memcg and system stats").