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

[Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>]

On 2023/04/03 17:15, Michal Hocko wrote:
> Is this
> https://lore.kernel.org/all/0000000000001d74d205f7c1821f@xxxxxxxxxx/ the
> underlying report ?

Yes.

> Could you explain the the deadlock scenario?

build_zonelists() from __build_all_zonelists() calls printk() with
zonelist_update_seq held.

printk() holds console_owner lock for synchronous printing, and then upon
unlock of console_owner lock, printk() holds port_lock_key and port->lock.

tty_insert_flip_string_and_push_buffer() from pty_write() conditionally calls
kmalloc(GFP_ATOMIC | __GFP_NOWARN) with port->lock held. But since commit 3d36424b3b58,
zonelist_update_seq is checked by GFP_ATOMIC allocation (i.e. a new locking dependency
was added by that commit).

  CPU0                                                       CPU1
  pty_write() {
    tty_insert_flip_string_and_push_buffer() {
                                                             __build_all_zonelists() {
      spin_lock_irqsave(&port->lock, flags);
      tty_insert_flip_string() {
        tty_insert_flip_string_fixed_flag() {
          __tty_buffer_request_room() {
            tty_buffer_alloc() {
              kmalloc(GFP_ATOMIC | __GFP_NOWARN) {
                __alloc_pages_slowpath() {
                                                               write_seqlock(&zonelist_update_seq); // makes zonelist_update_seq.seqcount odd
                                                               // interrupt handler starts
                                                                 handle_irq() {
                                                                   serial8250_interrupt() {
                                                                     serial8250_tx_chars() {
                                                                       tty_port_tty_get() {
                                                                         spin_lock_irqsave(&port->lock, flags); // spins here waiting for kmalloc() from tty_insert_flip_string() to complete
                  zonelist_iter_begin() {
                    read_seqbegin(&zonelist_update_seq) {
                      // spins here waiting for interrupt handler to complete if zonelist_update_seq.seqcount is odd
                                                                         tty = tty_kref_get(port->tty);
                                                                         spin_unlock_irqrestore(&port->lock, flags);
                                                                       }
                                                                     }
                                                                   }
                                                                 }
                                                               // interrupt handler ends
                                                               write_sequnlock(&zonelist_update_seq); // makes zonelist_update_seq.seqcount even
                                                             }
                    }
                  }
                }
              }
            }
          }
        }
      }
      spin_unlock_irqrestore(&port->lock, flags);
    }
  }

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?



This bug report might be a suggestion that we want to use two versions of
__alloc_pages_slowpath(), one for atomic context which is geared towards smaller
kernel stack usage and simplified locking dependency (because atomic allocation can
happen from subtle context including interrupt handler) and the other for noinline
version for schedulable context which is geared towards larger kernel stack usage
and complicated locking dependency for implementing rich retry paths including
direct reclaim and OOM kill...