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
