Re: [PATCH] printk: stop spining waiter when console resume to flush prb

[Petr Mladek]

On Thu 2021-05-06 23:12:02, Sergey Senozhatsky wrote:
> On (21/05/06 23:07), Sergey Senozhatsky wrote:
> > 
> > Can we count the number of lines that we print from the `current` context
> > in console_unlock() and if after N messages there is no console_lock waiter
> > waiting for the `current` to handover console lock ownership, then create
> > one: schedule IRQ work that will become a console lock owner, spin on
> > console lock and call console_unlock() once it acquired the ownership.
> > That 'artificial' console lock owner will do the same - print N
> > messages, if nothing wants to become a console lock owner then it'll
> > queue another IRQ work.
> 
> Or even simpler
> 
> console_unlock()
> {
> 	...
> 
> 	if (printed_messages > limit && !console_lock_spinning_disable_and_check()) {
> 		printk_safe_exit_irqrestore(flags);
> 
> 		console_locked = 0;
> 		up_console_sem();
> 
> 		defer_console_output();
> 		return;
> 	}
> 
> 	...
> }

No, please, no.

This is exactly the opposite. The original patch tried to keep the
work in the preemtible context. This proposal moves the work into irq
context which is bad. Not to say, that defer_console_output() would
trigger IRQ on the same CPU again and again.

All the problems with printk() are that we try to support all
scenarios. But it simply does not work. We need to say that some
situations are not supported.

Flood of messages and slow console requires a miracle. The only chance
is a huge buffer and get them out later. Infinite flood of messages
is impossible to handle by definition.

I hope that we agreed that the priority is to keep printk() safe
and do not break the system. The ultimate solution is to handle
consoles in a separate preemtible context (kthread).

There should be atomic consoles for those who want to see the messages
immediately with the cost of slowing down the code doing the real job.


I do not have strong opinion whether the proposed patch is worth it.
It is just another compromise that might be better in some situations
and worse in others.

Well, this situation is special. There might be many accumulated
messages during the hibernation. They did not have any chance to
be handled by more CPUs. Using the well known preemtible context
sounds slightly better than risking a random victim in atomic
context.

Anyway, I am fine with discarding this patch and focusing on
the offload to kthreads.

Best Regards,
Petr