Re: [RFC][PATCH v2 1/2] printk: Make printk() completely async

[Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>]

On (03/06/16 16:18), Tetsuo Handa wrote:
> Sergey Senozhatsky wrote:
> > printk() is expected to work under different conditions and in different
> > scenarios, including corner cases of OOM when all of the workers are busy
> > (e.g. allocating memory). Thus by default printk() uses its own dedicated
> > workqueue with WQ_MEM_RECLAIM bit set. It falls back to system_long_wq
> > (console_unlock() is time unbound) only if it has failed to allocate
> > printk_wq. Another thing to mention, is that deferred printk() messages
> > may appear before printk_wq is allocated, so in the very beginning we
> > have to printk deferred messages the old way -- in IRQ context.
> 
> I think we should not depend on system_long_wq which does not have
> WQ_MEM_RECLAIM bit. If workqueue allocation upon boot fails (due to ENOMEM),
> such systems won't be able to start userspace programs.

well, yes. system_long_wq is a fallback, but probably just BUG_ON(!printk_wq)
would be a better thing to do.

> Moreover, I don't like use of a workqueue even if printk_wq was allocated
> with WQ_MEM_RECLAIM bit. As you can see in the discussion of the OOM reaper,
> the OOM reaper chose a dedicated kernel thread rather than a workqueue
> ( http://lkml.kernel.org/r/1454505240-23446-2-git-send-email-mhocko@kernel.org ).
>
> Blocking actual printing until ongoing workqueue item calls schedule_timeout_*()
> is not nice (see commit 373ccbe59270 and 564e81a57f97).

printk and console_drivers are expected to be 'callable' from any
context, including IRQs, or under spin_locks, etc. so neither part of
	printk->console_unlock()->console_driver->write()
can sleep.

console_driver->write() is quite often something like this

	foo_write()
	{
		spin_lock_irqsave(&port->lock);
		uart_console_write( ... foo_put_char);
		spin_unlock_iqrestore(&port->lock);
	}

and foo_put_char(...), perhaps, waits for device to become ready
transaction

	while (!(UART_GET_STATUS(port) & TXEMPTY))
		cpu_relax();

and then sends out the character to the device

	UART_SET_DATA(port, (unsigned char)ch)



so an already executing printk_wq item shouldn't block because of
OOM happening on another CPU.

do you mean a new worker allocation delay and a MAYDAY timer delay?


> Use of WQ_MEM_RECLAIM means we add a task_struct for that workqueue. Thus, using
> a kernel thread does not change total number of task_struct compared to WQ_MEM_RECLAIM
> approach. I think actual printing should occur as soon as possible rather than randomly
> deferred until workqueue item sleeps.

hm, need to take a look.


> > +static void printing_work_func(struct work_struct *work)
> > +{
> > +	console_lock();
> > +	console_unlock();
> > +}
> 
> Is this safe? If somebody invokes the OOM killer between console_lock()
> and console_unlock(), won't this cause OOM killer messages not printed?

between console_lock() and console_unlock() nothing can steal that lock
from worker; so once a worker grabbed the console_lock() it will print
(and console_lock() does not mean "lock logbuf", printk() from other CPUs
can add messages to the logbuf while worker loops in console_unlock()).

if you mean something like

	console_lock();
	for (...) {
		do_foo() {
			...
				pr_err(" ... foo message ...\n");
			...
		}
	}
	console_unlock();

then yes, nothing will be printed until that process executes console_unlock(),
because it's console_unlock() that pushes the messages to console drivers.
(console_lock()/console_unlock() have other problems and issues, which are
not addressed in this patch set). there is only one function that ignores the
state of console semaphore:
	panic()->console_flush_on_panic()

but that's a separate thing, not related to this patch set. if you mean something
else here, then please more details.

	-ss