Messages in this thread | | | Date | Wed, 16 Mar 2016 11:01:15 +0900 | From | Sergey Senozhatsky <> | Subject | Re: [RFC][PATCH v4 1/2] printk: Make printk() completely async |
| |
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
| |