Messages in this thread | | | Date | Fri, 1 Apr 2016 10:08:03 +0900 | From | Sergey Senozhatsky <> | Subject | Re: [RFC][PATCH v8 1/2] printk: Make printk() completely async |
| |
Hello Petr,
On (03/31/16 13:12), Petr Mladek wrote: > > + * Set printing kthread sleep condition early, under the > > + * logbuf_lock, so it (if RUNNING) will go to console_lock() > > + * and spin on logbuf_lock. > > + */ > > + if (!in_panic && printk_kthread && !need_flush_console) > > + need_flush_console = true; > > I would remove the if-statement and always set it: > > + It does not matter if we set it in panic. It will not affect > anything.
hm... yes, you're right.
> + The check for printk_kthread is racy. It might be false here > and it might be true later when check whether to wakeup > the kthread or try to get console directly.
which is fine, isn't it? we will wakeup the printing kthread, it will console_lock()/console_unlock() (regardless the state of need_flush_console). printing thread checks need_flush_console only when it decides whether it shall schedule.
> + We might set it true even when it was true before. > > I think that this was an attempt to avoid a spurious wake up. > But we solve it better way now.
we also may have 'printk.synchronous = 1', which will purposelessly dirty need_flush_console from various CPUs from every printk /* and upon every return from console_unlock() */; that's why I added both printk_kthread and !need_flush_console (re-dirty already dirtied) checks.
> > raw_spin_lock(&logbuf_lock); > > retry = console_seq != log_next_seq; > > + if (!retry && printk_kthread) > > + need_flush_console = false; > > Similar here. I remove the if-statement and always set it. We will > either retry or it should be false anyway.
well, 'printk.synchronous = 1'. seems that `!retry' check can be dropped, I agree.
a side nano-note, apart from 'printk.synchronous = 1', we also can have !printk_kthread because kthread_run(printk_kthread_func) failed. it's quite unlikely, but still.
[..] > > + while (1) { > > + set_current_state(TASK_INTERRUPTIBLE); > > + if (!need_flush_console) > > + schedule(); > > + > > + __set_current_state(TASK_RUNNING); > > > We still must do here: > > need_flush_console = false;
oh, wow! that's a major mistake. thanks a lot for catching this! shame on me.
> /* > * Avoid an infinite loop when console_unlock() cannot > * access consoles, e.g. because of a suspend. We > * could get asleep here. Someone else will call > * consoles if conditions change. > */
looks ok.
> Also another name might help. If we set it false here, the value > does describe a global state. The variable describes if this > kthread needs to flush the console. So, more precise name would be > > printk_kthread_need_flush_console
yes, makes sense.
> I think that we are close. I really like the current state of > the patch and how minimalistic it is.
thanks for your review.
I'll re-spin.
-ss
| |