Messages in this thread | | | From | John Ogness <> | Subject | Re: [PATCH printk v3 13/15] printk: add kthread console printers | Date | Wed, 20 Apr 2022 22:08:39 +0206 |
| |
On 2022-04-20, Petr Mladek <pmladek@suse.com> wrote: > On Wed 2022-04-20 01:52:35, John Ogness wrote: >> @@ -2280,10 +2295,10 @@ asmlinkage int vprintk_emit(int facility, int level, >> printed_len = vprintk_store(facility, level, dev_info, fmt, args); >> >> /* If called from the scheduler, we can not call up(). */ >> - if (!in_sched) { >> + if (!in_sched && allow_direct_printing()) { > > allow_direct_printing() is racy here. But I think that we could live > with it, see below.
Well, it is not racy for its intended purpose, which is a context that does:
printk_prefer_direct_enter(); printk(); printk_prefer_direct_exit();
It is only racy for _other_ contexts that might end up direct printing. But since those other contexts don't have a preference, I see no problem with it.
>> @@ -3524,7 +3774,16 @@ void defer_console_output(void) >> * New messages may have been added directly to the ringbuffer >> * using vprintk_store(), so wake any waiters as well. >> */ >> - __wake_up_klogd(PRINTK_PENDING_WAKEUP | PRINTK_PENDING_OUTPUT); >> + int val = PRINTK_PENDING_WAKEUP; >> + >> + /* >> + * If console deferring was called with preferred direct printing, >> + * make the irqwork perform the direct printing. >> + */ >> + if (atomic_read(&printk_prefer_direct)) >> + val |= PRINTK_PENDING_DIRECT_OUTPUT; > > We actually need: > > /* > * Make sure that someone will handle the messages when direct > * printing is allowed. It happens when the kthreads are less > * reliable or unusable at all. > */ > if (allow_direct_printing()) > val |= PRINTK_PENDING_DIRECT_OUTPUT;
Agreed. I will update the comments appropriately as well.
> It is racy. But the same race is also in vprintk_emit().
It is not racy for the intended purpose, so I think it is fine.
> False positive is fine. console_flush_all() will bail out when > the direct printing gets disabled in the meantime. > > False negative is worse. But we will still queue PRINTK_PENDING_WAKEUP > that will try to wake up the kthreads that should still be around. > > And it was always problem even with console_trylock() approach. > Failure means an expectation that someone else is doing the printing. > It might be either a kthread or the current console_lock owner. > But it is never guaranteed because both might be sleeping.
By "sleeping" I guess you mean "scheduled out". The console_lock owner or mutex/atomic_t holder will be within printing code. And if a kthread sees new records available, it will continue rather than wait.
> We do our best by calling pr_flush() or console_flush_on_panic() > on various places. Also PRINTK_PENDING_WAKEUP will always try to wake > up the kthreads.
Yes.
> Anyway, we should document this somewhere. At least in the commit > message. > > My dream is Documentation/core-api/printk-design.rst but I do not > want to force you to do it ;-)
I would be happy to contribute to such a document.
John
| |