Re: [RFC][PATCH 2/4] printk: offload printing from wake_up_klogd_work_func()

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

On (03/20/17 17:09), Petr Mladek wrote:
[..]
> > I don't want that printk_kthread_need_flush_console to exist. instead,
> > I think, I want to move printk_pending out of per-cpu memory and use a
> > global printk_pending. set PRINTK_PENDING_OUTPUT bit to true in
> > vprintk_emit(), clear it in console_unlock(). and make both printk_kthread
> > scheduling condition and console_unlock() retry path depend on
> > `printk_pending == 0' being true.
> 
> I like the idea. The things closely related.
>  
> > something like below (the code is ugly and lacks a ton of barriers, etc.
> > etc.)
> 
> Sigh, I wanted to add few comments and it got me deeper than I wanted.

no worries, Petr.


[..]
> Anyway, it might make sense to do the change in more steps.

yes, sure. "per-CPU -> global printk_pending" transition first,
and then printk kthread.


[..]
> > +#define PRINTK_PENDING_WAKEUP	0x01
> > +#define PRINTK_PENDING_OUTPUT	0x02
> > +
> > +static int printk_pending = 0;
> 
> Something tells me that we need to use atomic_t. Otherwise, we could
> not safely manipulate the bits withtout a lock.

yes, I'm doing atomic set_bit/test_bit/clear_bit in current (unpublished) version.


> Alternative solution would be to use two separate variables.
> This might make the code easier to read. I think that they
> were combined only to safe space in the per-CPU area.

hm. I think one variable still can work for us; but can split it.

as of rename. dunno. I'm kinda OK with its current name.
PENDING_OUTPUT looks a bit better that POKE_CONSOLE to me.


[..]
> >  	if (console_suspended) {
> > +		printk_pending &= ~PRINTK_PENDING_OUTPUT;
> 
> Hmm, this is pretty non-intuitive. I guess that it is needed to
> avoid a busy cycle in the printk kthread?

it absolutely is.
sorry, the "code" I posted was too cryptic.

> >  		up_console_sem();
> >  		return;
> >  	}
> > @@ -2242,6 +2247,8 @@ void console_unlock(void)
> >  	console_may_schedule = 0;
> >  
> >  again:
> > +	wake_klogd = printk_pending & PRINTK_PENDING_WAKEUP;
> > +	printk_pending = 0;
> 
> This might be racy. PRINTK_PENDING_WAKEUP is set without
> a lock in bust_spinlocks() via wake_up_klogd(). The above
> code read and clears the state non-atomically.

the patch I'm looking at right now does atomic set_bit() and a bunch of
atomic test_and_clear_bit/test_bit/etc.

> >  	/*
> >  	 * We released the console_sem lock, so we need to recheck if
> >  	 * cpu is online and (if not) is there at least one CON_ANYTIME
> > @@ -2330,15 +2337,16 @@ void console_unlock(void)
> >  	 * flush, no worries.
> >  	 */
> >  	raw_spin_lock(&logbuf_lock);
> > -	retry = console_seq != log_next_seq;
> > +	if (printk_pending != 0 || console_seq != log_next_seq)
> 
> printk_pending != 0 also when PRINTK_PENDING_WAKEUP is set.

yes.

> I would do it the other way. I would clear PRINTK_PENDING_OUTPUT
> when console_seq == log_next_seq and keep the check as is here.
[..]
> > +		retry = true;
> >  	raw_spin_unlock(&logbuf_lock);
> >  	printk_safe_exit_irqrestore(flags);
> >  
> > -	if (retry && console_trylock())
> > -		goto again;
> > -
> >  	if (wake_klogd)
> >  		wake_up_klogd();
> > +
> > +	if (retry && console_trylock())
> > +		goto again;
> 
> Why do you actually modify the logic for klogd()?
> It might make sense but it is questionable. For example,
> klogd() will need logbuf_lock as well. It might fight over
> it with the console when the again target is used.
> I would do it in separate patch and probably not
> in this patchset.

I just wanted to keep printk_prnding check simpler and I figured out
that klogd logbuf_lock contention will not be something new, because
of the while() loop in kthread_printk function

	printk_tkread func
		while (1) {
			if (!pending_output)
				schedule();

			console_lock()
			console_unlock()
				wake_up klogd	/*
						 * and may be do another
						 * console_lock() straight ahead if pending_output != 0
						 */
		}

but yes. I'll drop that part and will handle only PRINTK_PENDING_OUTPUT
bit in console_unlock(), leaving the PRINTK_PENDING_WAKEUP stuff to
irq work.

I'll try to send out a refreshed version soon.


> >  EXPORT_SYMBOL(console_unlock);
> >  
> > @@ -2722,19 +2730,9 @@ static int __init printk_late_init(void)
> >  late_initcall(printk_late_init);
> >  
> >  #if defined CONFIG_PRINTK
> > -/*
> > - * Delayed printk version, for scheduler-internal messages:
> > - */
> > -#define PRINTK_PENDING_WAKEUP	0x01
> > -#define PRINTK_PENDING_OUTPUT	0x02
> > -
> > -static DEFINE_PER_CPU(int, printk_pending);
> 
> BTW: wake_up_klogd_work does not need to be per-CPU as well.
> irq_work infrastructure heavily uses per-CPU variables.
> But a global struct irq_work is safe, see irq_work_claim().

interesting. need to look at it.

we also can move printk_kthread irq_work out of per-CPU and drop
the whole 'if (printk_safe_irq_ready) smp_rmb() ' thing in this case
and simplify printk_safe_init().

may be in a separate patch set, though. since this is not really
related to printk kthread.


> > [..]
> > > If I remember correctly, you were not much happy with this
> > > solution because it did spread the logic. I think that you did not
> > > believe that it was worth fixing the second problem.
> > 
> > hm, I think Jan Kara was the first one who said that we
> > are overcomplicating the whole thing... or may be it was me.
> > don't deny it either.
> 
> I do not remember as well :-) Anyway, it really looks more
> complicated than I thought.
> 
> I think that some clean up and optimization of the printk_pending
> stuff is needed and worth it. I am just not sure whether to do it
> before or after the printk kthread patchset.
> 
> I would slightly prefer to clean the printk_pending stuff first.
> It might delay printk kthread patchset a bit but it will be cleaner.

absolutely agree. and thanks for looking into it.

	-ss