On Tue, Apr 04, 2023 at 05:20:35PM +0200, Michal Hocko wrote:
> On Tue 04-04-23 23:31:58, Tetsuo Handa wrote:
> > syzbot is reporting circular locking dependency which involves
> > zonelist_update_seq seqlock [1], for this lock is checked by memory
> > allocation requests which do not need to be retried.
> >
> > One deadlock scenario is kmalloc(GFP_ATOMIC) from an interrupt handler.
> >
> > CPU0
> > ----
> > __build_all_zonelists() {
> > write_seqlock(&zonelist_update_seq); // makes zonelist_update_seq.seqcount odd
> > // e.g. timer interrupt handler runs at this moment
> > some_timer_func() {
> > kmalloc(GFP_ATOMIC) {
> > __alloc_pages_slowpath() {
> > read_seqbegin(&zonelist_update_seq) {
> > // spins forever because zonelist_update_seq.seqcount is odd
> > }
> > }
> > }
> > }
> > // e.g. timer interrupt handler finishes
> > write_sequnlock(&zonelist_update_seq); // makes zonelist_update_seq.seqcount even
> > }
> >
> > This deadlock scenario can be easily eliminated by not calling
> > read_seqbegin(&zonelist_update_seq) from !__GFP_DIRECT_RECLAIM allocation
> > requests, for retry is applicable to only __GFP_DIRECT_RECLAIM allocation
> > requests. But Michal Hocko does not know whether we should go with this
> > approach.
>
> It would have been more useful to explain why that is not preferred or
> desirable.
>
It would have undesirable side-effects. !__GFP_DIRECT_RECLAIM does not mean
that the caller is happening from interrupt context or is a re-entrant
allocation request from IRQ context. Special casing __GFP_DIRECT_RECLAIM
could allow a speculative allocation to simply prematurely fail due to
memory hotplug.
This deadlock could be mitigated by not calling
read_seqbegin(&zonelist_update_seq) from !__GFP_DIRECT_RECLAIM
allocation request but it has undesirable consequences.
!GFP_DIRECT_RECLAIM applies to allocations other than
atomic allocations from interrupt context such as GFP_NOWAIT
allocations. These type of allocations could prematurely fail due to
a memory hotplug race or cpuset update and while this is probably
harmless and recoverable, it's undesirable and unnecessary to fix
the deadlock.
I imagine there could also be checks for callers from IRQ context but on
some older chips, checking if IRQs are disabled is expensive so also is
an undesirable fix. Maybe the same is true for newer CPUs, but I didn't
check. The printk_deferred option seems the most harmless option for now
and maybe printk_deferred will ultimately go away.
Other than potential changelog updates
Acked-by: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>
--
Mel Gorman
SUSE Labs
