printk: Should console related code avoid __GFP_DIRECT_RECLAIM memory allocations?

[Sergey Senozhatsky]

On (07/11/17 11:31), Sergey Senozhatsky wrote:
[..]
> (replying to both Petr and Daniel)
> 
> interesting direction, gents.
> 
> and this is what I thought about over the weekend; it's very sketchy and
> I didn't spend too much time on it. (I'm on a sick leave now, sorry).
> 
> it's quite close to what you guys have mentioned above.
> 
> a) keep console_sem only to protect console drivers list modification
> b) add a semaphore/mutex to struct console
> c) move global console_seq/etc to struct console
> e) use a single kthread for printing, but do console_unlock() multi passes,
>    printing unseen logbuf messages on per-console basis
> 
> 
> so console_lock()/console_unlock() will simply protect console drivers
> list from concurrent manipulation; it will not prevent us from printing.
> now, there are places where console_lock() serves a special purpose - it
> makes sure that no new lines are printed to the console while we scroll
> it/flip it/etc. IOW while we do "some things" to a particular console.
> the problem here, is that this also blocks printing to all of the registered
> console drivers, not just the one we are touching now. therefore, what I was
> thinking about is to disable/enable that particular console in all of the
> places where we really want to stop printing to this console for a bit.
> 
> IOW, something like
> 
> 
> 
> 	console_lock()
> 	:	down(console_sem);
> 
> 	console_disable(con)
> 	:	lock(con->lock);
> 	:	con->flags &= ~CON_ENABLED;
> 	:	unlock(con->lock)
> 
> 	console_unlock()
> 	:	for_each_console(con)
> 	:		while (con->console_seq != log_next_seq) {
> 	:			msg_print_text();
> 	:			con->console_seq++;
> 	:		
> 	:			call_console_drivers()
> 	:			:	if (con->flags & CON_ENABLED)
> 	:			:		con->write()
> 	:		}
> 	:	up(console_sem);
> 
> 
> 	// do "some things" to this console. it's disabled, so no
> 	// ->write() callback would be called in the meantime
> 
> 	console_lock()
> 	:	down(console_sem);
> 
> 	console_enable(con)
> 	:	lock(con->lock);
> 	:	con->flags |= CON_ENABLED;
> 	:	unlock(con->lock)
> 
> 
> 	// so now we enabled that console again. it's ->console_seq is
> 	// probably behind the rest of consoles, so console_unlock()
> 	// will ->write() all the unseen message to this console.
> 
> 	console_unlock()
> 	:	for_each_console(con)
> 	:		while (con->console_seq != log_next_seq) {
> 	:			msg_print_text();
> 	:			con->console_seq++;
> 	:		
> 	:			call_console_drivers()
> 	:			:	if (con->flags & CON_ENABLED)
> 	:			:		con->write()
> 	:		}
> 	:	up(console_sem);
> 

ok, obviously stupid.

I meant to hold con->lock between console_disable() and console_enable().
so no other CPU can unregister it, etc. printk->console_unlock(), thus,
can either have a racy con->flags check (no con->lock taken) or try
something like down_trylock(&con->lock): if it fails, continue.

but need to look more.

	-ss