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

[Michal Hocko <mhocko@suse.com>]

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.


> Since this is a problem introduced by mm change, I think fixing this problem on the
> mm side is the cleaner.

Agreed. That would be one of the options I have mentioned. I do not
think the printk information serves such a big role we couldn't live
without it.

> Can't there be a different approach? For example, can't we
> replace
> 
> 	cpuset_mems_cookie = read_mems_allowed_begin();
> 	zonelist_iter_cookie = zonelist_iter_begin();
> 
> and
> 
> 	if (check_retry_cpuset(cpuset_mems_cookie, ac) ||
> 	    check_retry_zonelist(zonelist_iter_cookie))
> 
> with different conditions, like recalculate cpuset/zonelist in the last second and
> check immediately before giving up allocation or OOM kill whether they have changed?

Dunno and honestly that is a subtle piece of code and I would rather not
touch it just because we have limitations in printk usage. Especially
considerenig the above.

-- 
Michal Hocko
SUSE Labs