Re: [PATCH] mm/page_alloc: don't check zonelist_update_seq from atomic allocations

[Petr Mladek <pmladek@suse.com>]

On Mon 2023-04-03 15:44:34, Michal Hocko wrote:
> On Mon 03-04-23 21:51:29, Tetsuo Handa wrote:
> > On 2023/04/03 21:09, Michal Hocko wrote:
> > > On Mon 03-04-23 20:14:28, Tetsuo Handa wrote:
> > >> Well, it seems that read_mems_allowed_begin() is protected by calling
> > >> local_irq_save(flags) before write_seqcount_begin(&current->mems_allowed_seq).
> > >> 
> > >> Can zonelist_iter_begin() be protected as well (i.e. call local_irq_save(flags)
> > >> before write_seqlock(&zonelist_update_seq)) ?
> > >> 
> > >> But even if write_seqlock(&zonelist_update_seq) is called with local irq disabled,
> > >> port_lock_key after all makes this warning again?
> > 
> > Hmm, local_irq_save(flags) before write_seqlock(&zonelist_update_seq) won't help.
> > Synchronous printk() will try to hold port->lock from process context even if local
> > irq is disabled, won't it? Not limited to interrupt handler but any synchronous printk()
> > inside write_seqlock(&zonelist_update_seq) / write_sequnlock(&zonelist_update_seq)
> > section is not safe.
> > 
> > > Thank you! IIUC this can only happen when there is a race with the
> > > memory hotplug. So pretty much a very rare event.
> > 
> > Right.
> > 
> > >                                                   Also I am not really
> > > sure this really requires any changes at the allocator level. I would
> > > much rather sacrifice the printk in build_zonelists or pull it out of
> > > the locked section. Or would printk_deferred help in this case?
> > 
> > Just moving printk() out of write_seqlock(&zonelist_update_seq) / write_sequnlock(&zonelist_update_seq)
> > section is not sufficient. This problem will happen as long as interrupt handler tries to hold port->lock.
> 
> I do not follow. How is a printk outside of zonelist_update_seq going to
> cause a dead/live lock? There shouldn't be any other locks (apart from
> hotplug) taken in that path IIRC.
> 
> > Also disabling local irq will be needed.
> 
> Why?
> 
> > By the way, is this case qualified as a user of printk_deferred(), for printk_deferred() says
> > 
> >   /*
> >    * Special printk facility for scheduler/timekeeping use only, _DO_NOT_USE_ !
> >    */
> >   __printf(1, 2) __cold int _printk_deferred(const char *fmt, ...);
> > 
> > ?
> 
> Dunno, question for printk maintainers. I know they want to limit the
> usage. Maybe this qualifies as a exception worth case as well.

Sigh, I am afraid that printk_deferred() is the best option at this
moment.

I see that there are more pr_info()/pr_cont() calls in build_zonelists.
You might want to use printk_deferred_enter()/exit() around them.

These problems should disappear once printk() messages get processesed
in a kthread. And pr_info() is not critical information by definition
and it is a perfect candidate for deferring.

Unfortunately, introducing the kthreads takes ages. So, we will have
to live with printk_deferred() for some time.


Important:

printk_deferred() still should be used with care. The messages will
be pushed to the console in another random context that might be
atomic. The more messages we deffer the bigger risk of softlockups
we create.

A rules of thumb:

   + printk_deferred() would make sense for printing few lines
     in a code where the dependency against the console driver
     code is really complicated. It seems to be this case.

  + printk_deferred() is not a good solution for long reports. It
    would just increase a risk of softlockups and could break
    the system.

Best Regards,
Petr