On 2023-06-21 17:38:03 [+0200], Petr Mladek wrote:
> If I get it correctly, RT solve both possible deadlocks by offloading
> the nested operation into a kthread (irq and printk threads).
> Plus, printk uses emergency_write() when the kthread is not usable.
correct.
> If this is true then printk_safe_enter() might be a nop on RT.
it is gone actually.
> All possible deadlocks are prevented either by the kthread or
> the console->emergency_write() call.
correct.
> 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.
if the special printing is not implemented then there is only printing
via the kthread meaning no emergency printing if needed.
Once the interface is upstream, more drivers can be added including GPU
based.
> Note that adding printk thread for the graphical consoles will
> be a real challenge. We have hard times even with the basic
> UART 8250. There are still races possible in the implementation
> in the RT tree...
John has been mumbling something about those but did not send anything.
> 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 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.
The global variable would be probably be simpler but I guess you want
to have direct printing on other CPUs.
To be honst, I would suggest to work towards always printing in a thread
with an emergency console than this deferred/ safe duckt tape. Once
tty_port::lock is acquired, every possible printk output, say a BUG,
kasan,… is a possible death trap. This limitation here wasn't obvious
and syzbot had to figure it out. There might be other side effects.
> > > 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
> > >
> > > .
> > Did I explain why it is sufficient to do
> > write_seqlock_irqsave()
> > printk_deferred_enter()
> >
> > assuming we have
> >
> > | static inline void do_write_seqcount_begin_nested(seqcount_t *s, int subclass)
> > | {
> > | seqcount_acquire(&s->dep_map, subclass, 0, _RET_IP_);
> > | do_raw_write_seqcount_begin(s);
> > | }
>
> Will this prevent any printk() called on the same CPU before
> printk_deferred_enter() is called?
Yes. There is the _irqsave() which disables interrupts at the very
beginning. Then we have here (seqcount_acquire()) the last possible
origin of a lockdep splat. It ends with do_raw_write_seqcount_begin()
which increments the counter (the "locking") and this is followed by
printk_deferred_enter().
> Best Regards,
> Petr
Sebastian
