Re: [PATCH] mm/page_alloc: fix potential deadlock on zonelist_update_seq seqlock

[Michal Hocko <mhocko@suse.com>]

On Tue 04-04-23 17:20:02, Tetsuo Handa wrote:
> On 2023/04/04 16:54, Michal Hocko wrote:
> >> +	/*
> >> +	 * Since __alloc_pages_slowpath() spins if zonelist_update_seq.seqcount
> >> +	 * is odd, any memory allocation while zonelist_update_seq.seqcount is
> >> +	 * odd have to be avoided.
> >> +	 *
> >> +	 * Explicitly disable local irqs in order to avoid calling
> >> +	 * kmalloc(GFP_ATOMIC) from e.g. timer interrupt handler.
> >> +	 * Also, explicitly prevent printk() from synchronously waiting for
> >> +	 * port->lock because tty_insert_flip_string_and_push_buffer() might
> >> +	 * call kmalloc(GFP_ATOMIC | __GFP_NOWARN) while holding port->lock.
> > 
> > This explanation of local_irq_save just doesn't make any sense. You do
> > not prevent any other cpu from entering the IRQ and doing the same
> > thing.
> 
> There is no need to prevent other CPUs from doing the same thing.
> The intent of local_irq_save() here is to avoid below sequence.
> 
>   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) {
>               // forever spins because zonelist_update_seq.seqcount is odd
>             }
>           }
>         }
>       }
>     // e.g. timer interrupt handler finishes
>     write_sequnlock(&zonelist_update_seq); // makes zonelist_update_seq.seqcount even
>   }

OK, now we are on the same page. Your previous bc630622-8b24-e4ca-2685-64880b5a7647@I-love.SAKURA.ne.jp
explanation had an intermediary lock in the dependency (port->lock). I
haven't realized that the actual scenario could be simpler than that.
But you are right that GFP_ATOMIC allocations from IRQ context are quite
common so this is a more general situation that doesn't really need to
involve TTY and some locking oddities there.

This all is quite subtle and easy to miss so please make sure to
describe both scenarios in the changelog. For the comment above I would
rephrase as follows:
	/*
	 * Explicitly disable interrupts before taking seqlock to prevent
	 * any IRQ handler from calling into the page allocator
	 * (e.g. GFP_ATOMIC) that could hit zonelist_iter_begin and live
	 * lock.
	 */

Thanks!
-- 
Michal Hocko
SUSE Labs