On Wed, Mar 16, 2016 at 11:01:15AM +0900, Sergey Senozhatsky wrote:
> 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();
else // This shoule be here, IMHO. Hm?
> > __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
