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();
> __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
