Messages in this thread Patch in this message | | | Date | Tue, 21 Mar 2017 13:01:05 +0900 | From | Sergey Senozhatsky <> | Subject | Re: [RFC][PATCH 2/4] printk: offload printing from wake_up_klogd_work_func() |
| |
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
| |