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

[Byungchul Park <byungchul.park@lge.com>]

On Wed, Mar 16, 2016 at 11:01:15AM +0900, Sergey Senozhatsky wrote:
> Hello Petr,
> 
> On (03/15/16 16:58), Petr Mladek wrote:
> [..]
> > > +static bool __read_mostly printk_sync = !IS_ENABLED(CONFIG_SMP);
> > > +module_param_named(synchronous, printk_sync, bool, S_IRUGO | S_IWUSR);
> > 
> > If we make it writtable, we also need to handle the situation that
> > it gets disabled at runtime. It means to make sure that the kthread
> > will be running event printk_sync was set during the boot.
> 
> yes, I just thought this morning that may be disabling 'write' here would
> be ok.
> 
> > What about this?
> > 
> > int need_flush_console;
> > 
> > 	while(1) {
> > 		set_current_state(TASK_INTERRUPTIBLE);
> > 		if (!need_flush_console)
> > 			schedule();

                else // This shoule be here, IMHO. Hm?

> > 		__set_current_state(TASK_RUNNING);
> > 
> > 		need_flush_console =  false;
> > 
> > > +		console_lock();
> > > +		console_unlock();
> > > +	}
> 
> much better, indeed.
> I assume `need_flush_console' is primarily for avoiding schedule() cost?
> not that it closes the race window 100%, it can be false at the time we
> check it, and become true by the time we schedule(). TASK_INTERRUPTIBLE
> should prevent lost wake_up() case, AFAIK.
> 
> > Also I wonder if we need some special handling of the system freezing
> > but I do not thing so.
> 
> hm, I don't think so either.
> 
> > > +	printk_thread = kthread_run(printing_func, NULL, "printk");
> > > +	BUG_ON(IS_ERR(printk_thread));
> > 
> > I would prefer to force the synchronous mode instead.
> 
> ok, no strong opinion here, I thought that if the system can't create
> a kthread in late_initcall(), then it probably doesn't have many chances
> to survive anyway.
> 
> > > + * Delayed printk version, for scheduler-internal messages:
> > 
> > This is not longer related to sheduler only.
> 
> this has changed. KTHREAD/IRQ  split is not needed anymore, please
> see below.
> 
> > BTW: I suggest to move this whole section in a separate patch.
> > It will be more clear what has changed for the async printk
> > and what stays for the deferred printk.
> 
> hm, sounds good.
> 
> 
> > 	if (pending & PRINTK_PENDING_CONSOLE_OUTPUT) {
> > 		if (printk_sync || !printk_kthread) {
> > 			/* If trylock fails, someone else is doing the printing */
> > 			if (console_trylock())
> > 				console_unlock();
> > 		} else {
> > 			wake_up_process(printk_kthread);
> > 		}
> > 
> > 	if (pending & PRINTK_PENDING_KLOGD_WAKEUP)
> > 		wake_up_interruptible(&log_wait);
> 
> yes, agree. this is what I have here:
> http://marc.info/?l=linux-kernel&m=145805101825604
> 
> > > +	bool in_panic = console_loglevel == CONSOLE_LOGLEVEL_MOTORMOUTH;
> > > +	bool sync_print = printk_sync;
> > 
> > I would force the global printk_sync if we are in_panic
> > 
> > 	if (in_panic)
> > 		printk_sync = true;
> 
> can add, yes.
> 
> > > -	/* If called from the scheduler, we can not call up(). */
> > > -	if (!in_sched) {
> > > +	if (sync_print) {
> > >  		lockdep_off();
> > 
> > I wonder if it might be much easier with If we used only the two
> > PRINTK_PENDING flags and force global printk_sync when in panic.
> 
> two PENDING flags stuff was my bad. (I replied here
> http://marc.info/?l=linux-kernel&m=145805101825604)
> 
> in short, my intention was to move it out of that part of vprintk_emit() that
> can recurse, but cannot detect the recursion. wake_up()/wake_up_process()
> add spin_locks/etc., which add possibilities of
>         vprint_emit()->spin_lock()->spin_dump()->vprintk_emit()->...
> that will not be handled by vprintk_emit() recursion detection code. but
> I guess I simply want to move this under the logbuf lock section after all,
> so printk recursion detection will have better chances to help us out.
> 
> > Sigh, it would be great to rename also wake_up_klogd_work and
> > wake_up_klogd_work_func(). They are not only about klogd.
> > Well, this should be separate patch as well because it
> > was even before.
> 
> hm, yes, as a separate patch later I think.
> 
> > I still to thing about possible races. Especially, when checking
> > printk_kthread and printk_sync.
> 
> hm, I don't think we risk anything here. if CPU saw an 'old' (NULL) @printk_kthread
> then it just would do direct printk. once it's !NULL, we can wake it up.
> is your concern here that `pointer = VALUE' can be !atomic?
> 
> > I hope that some of the above suggestions makes sense. vprintk_emit()
> > is crazy already now. I feel motivated to do not make it worse ;-)
> 
> thanks for review.
> 
> 	-ss