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(¤t->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?
