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

[Petr Mladek <pmladek@suse.com>]

On Fri 2023-06-23 10:12:50, Sebastian Andrzej Siewior wrote:
> On 2023-06-21 17:38:03 [+0200], Petr Mladek wrote:
> > But wait. AFAIK, the printk kthread is implemented only for the serial
> > console. So, it won't be safe for other consoles, especially
> > the problematic tty_insert_flip_string_and_push_buffer() call.
> 
> > OK, what about using migrate_disable() in printk_deferred_enter()?
> > Something like:
> > 
> > /*
> >  * The printk_deferred_enter/exit macros are available only as a hack.
> >  * They define a per-CPU context where all printk console printing
> >  * is deferred because it might cause a deadlock otherwise.
> >  *
> >  * It is highly recommended to use them only in a context with interrupts
> >  * disabled. Otherwise, other unrelated printk() calls might be deferred
> >  * when they interrupt/preempt the deferred code section.
> >  *
> >  * They should not be used for to deffer printing of many messages. It might
> >  * create softlockup when they are flushed later.
> >  *
> >  * IMPORTANT: Any new use of these MUST be consulted with printk maintainers.
> >  *    It might have unexpected side effects on the printk infrastructure.
> >  */
> > #ifdef CONFIG_PREEMPT_RT
> > 
> > #define printk_deferred_enter()			\
> > 	do {					\
> > 		migrate_disable();		\
> > 		__printk_safe_enter();		\
> > 	} while (0)
> > 
> > #define printk_deferred_exit()			\
> > 	do {					\
> > 		__printk_safe_exit();		\
> > 		migrate_enable();		\
> > 	} while (0)
> > 
> > #else  /* CONFIG_PREEMPT_RT */
> > 
> > #define printk_deferred_enter()			\
> > 	do {					\
> > 		preempt_disable();		\
> > 		__printk_safe_enter();		\
> > 	} while (0)
> > 
> > #define printk_deferred_exit()			\
> > 	do {					\
> > 		__printk_safe_exit();		\
> > 		preempt_enable();		\
> > 	} while (0)
> > 
> > #endif   /* CONFIG_PREEMPT_RT */
> > 
> > Note that I have used preempt_disable() on non-RT because it is much
> > cheaper. And IRQs will be disabled anyway on non-RT system
> > in this code path.
> 
> It would do _but_ is it really needed? All users but one (the one in
> __build_all_zonelists()) have interrupts already disabled. This isn't a
> hot path and is not used very common. Does it justify the ifdef?
>
> An unconditional migrate_disable() would do the job if you want it to be
> safe

Makes sense. The ifdef does not look worth it.

> but I would rather suggest lockdep annotation instead of disabling
> either preemption or migration. If I read the comment on top right, this
> API isn't open for the public.

It might work when mm people agree to explicitly call
migrate_disable() in __build_all_zonelists(). I mean
to call there:

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

Plus it would require some comments.

For me, it is acceptable to hide at least the migrate_disable() part
into printk_deferred_enter().

> The global variable would be probably be simpler but I guess you want
> to have direct printing on other CPUs.

Yes, the scope should be as limited as possible.

> To be honst, I would suggest to work towards always printing in a thread
> with an emergency console than this deferred/ safe duckt tape.

I have used this argument many years. But it is not much convincing
after 10 years. Note that John has sent 1st RFC 4 years ago and we
still do not have the kthreads :-/

Best Regards,
Petr