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

[Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>]

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.
Also disabling local irq will be needed.

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, ...);

?

Since this is a problem introduced by mm change, I think fixing this problem on the
mm side is the cleaner. 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?