Re: [PATCH] mm/page_alloc: Use write_seqlock_irqsave() instead write_seqlock() + local_irq_save().

[Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>]

On 2023/06/21 22:06, Sebastian Andrzej Siewior wrote:
> On 2023-06-21 20:33:35 [+0900], Tetsuo Handa wrote:
>> On 2023/06/21 19:40, Sebastian Andrzej Siewior wrote:
>>> printk_deferred_enter() has to be invoked in non-migrate-able context to
>>> ensure that deferred printing is enabled and disabled on the same CPU.
>>
>> I can't catch. local_irq_save(flags); makes non-migrate-able context
>> because sleeping is not allowed while IRQ is disabled, doesn't it?
> 
> That is correct. The problem is the local_irq_save() which remains on
> PREEMPT_RT and this is problematic during write_seqlock() which acquires
> spinlock_t which becomes a sleeping lock. write_seqlock_irqsave()
> substitutes everything properly so on RT there is no local_irq_save().

include/linux/seqlock.h says

  static inline unsigned long __write_seqlock_irqsave(seqlock_t *sl)
  {
  	unsigned long flags;
  
  	spin_lock_irqsave(&sl->lock, flags);
  	do_write_seqcount_begin(&sl->seqcount.seqcount);
  	return flags;
  }
  
  /**
   * write_seqlock_irqsave() - start a non-interruptible seqlock_t write
   *                           section
   * @lock:  Pointer to seqlock_t
   * @flags: Stack-allocated storage for saving caller's local interrupt
   *         state, to be passed to write_sequnlock_irqrestore().
   *
   * _irqsave variant of write_seqlock(). Use it only if the read side
   * section, or other write sections, can be invoked from hardirq context.
   */
  #define write_seqlock_irqsave(lock, flags)				\
  	do { flags = __write_seqlock_irqsave(lock); } while (0)

  /**
   * write_seqlock() - start a seqlock_t write side critical section
   * @sl: Pointer to seqlock_t
   *
   * write_seqlock opens a write side critical section for the given
   * seqlock_t.  It also implicitly acquires the spinlock_t embedded inside
   * that sequential lock. All seqlock_t write side sections are thus
   * automatically serialized and non-preemptible.
   *
   * Context: if the seqlock_t read section, or other write side critical
   * sections, can be invoked from hardirq or softirq contexts, use the
   * _irqsave or _bh variants of this function instead.
   */
  static inline void write_seqlock(seqlock_t *sl)
  {
  	spin_lock(&sl->lock);
  	do_write_seqcount_begin(&sl->seqcount.seqcount);
  }

and regarding include/linux/spinlock.h , since spin_lock_irqsave(lock, flags) is
equivalent with local_irq_save(flags) + spin_lock(lock), there is no difference
between

  local_irq_save(flags);
  printk_deferred_enter();
  write_seqlock(&zonelist_update_seq);

and

  write_seqlock_irqsave(&zonelist_update_seq, flags);
  printk_deferred_enter();

(except potential lockdep warning).

However, regarding include/linux/spinlock_rt.h , since spin_lock_irqsave(lock, flags)
is equivalent with spin_lock(lock), there is difference between

  local_irq_save(flags);
  printk_deferred_enter();
  write_seqlock(&zonelist_update_seq);

and

  write_seqlock_irqsave(&zonelist_update_seq, flags);
  printk_deferred_enter();

.

Is above understanding correct?

And you are trying to replace

  local_irq_save(flags);
  printk_deferred_enter();
  write_seqlock(&zonelist_update_seq);

with

  write_seqlock_irqsave(&zonelist_update_seq, flags);
  printk_deferred_enter();

, aren't you?

But include/linux/printk.h says

  /*
   * The printk_deferred_enter/exit macros are available only as a hack for
   * some code paths that need to defer all printk console printing. Interrupts
   * must be disabled for the deferred duration.
   */
  #define printk_deferred_enter __printk_safe_enter
  #define printk_deferred_exit __printk_safe_exit

which means that local_irq_save() is _required_ before printk_deferred_enter().

If local_irq_save() is hidden by your patch, what guarantees that
printk_deferred_enter() and printk_deferred_exit() run on the same CPU?
Also, if local_irq_save() is hidden due to RT, what guarantees that

  write_seqlock_irqsave(&zonelist_update_seq, flags);
  <<IRQ>>
    some_timer_function() {
      printk();
    }
  <<IRQ>>
  printk_deferred_enter();

does not happen because write_seqlock_irqsave() does not disable IRQ?

Disabling IRQ before incrementing zonelist_update_seq is _required_ for both

  making printk_deferred_enter() safe

and

  making sure that printk_deferred_enter() takes effect

.