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(¤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. 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
