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

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2019-02-11 11:17:45 [-0800], Matthew Wilcox wrote:
> 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:
> > > 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.
> > 
> > >                                            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 said, it seems weird to me that
> > > 
> > > 	spin_lock_irqsave();
> > > 	BUG_ON(!irqs_disabled());
> > > 	spin_unlock_irqrestore();
> > > 
> > > would trigger. Wouldn't it make sense to have a raw_irqs_disabled() or
> > > something and keep the irqs_disabled() abstraction layer intact?
> > 
> > maybe if I know why interrupts should be disabled in the first place.
> > The ->i_pages lock is never acquired with disabled interrupts so it
> > should be safe to proceed as-is. Should there be a spot in -RT where the
> > lock is acquired with disabled interrupts then lockdep would scream. And
> > then we would have to decide to either move everything raw_ locks (and
> > live with the consequences) or avoid acquiring the lock with disabled
> > interrupts.
> 
> I think you mean 'the i_pages lock is never acquired with interrupts
> enabled".  Lockdep would scream if it were -- you'd be in a situation
> where an interrupt handler which acquired the i_pages lock could deadlock
> against you.
With RT enabled the i_pages lock is always acquired with interrupts
enabled because spin_lock_irq() does not disable interrupts.

Sebastian