| Message ID | 201701051927.IJF65159.OFFtSLQOOFHJMV@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On (01/05/17 19:27), Tetsuo Handa wrote: [..] > > the other thing is... do we really need to console_conditional_schedule() > > from fbcon_*()? console_unlock() does cond_resched() after every line it > > prints. wouldn't that be enough? > > Every record, isn't it? yes. after every call to console drivers. > How many bytes can a record write to consoles? 1024 - PREFIX_MAX bytes at most. > > so may be we can drop some of console_conditional_schedule() > > call sites in fbcon. or update console_conditional_schedule() > > function to always return the current preemption value, not the > > one we saw in console_trylock(). > > Replacing console_may_schedule with get_console_may_schedule() will avoid > this bug. I noticed that we forgot to reset console_may_schedule to 0 > because the "again:" label is located after the assignment line. well... we call cond_resched() from under the spin_lock(), which modifies the preempt count and cond_resched() which we call from console_conditional_schedule() checks that current->preempt count is 0 before it calls tif_need_resched(). there are configs/setups where spinlock does not modify preempt count, but I suspect that on those setups cond_resched() does nothing. so it looks to me that we just WARN from ___might_sleep() but, at least in this particular case, I don't think we can actually schedule(). but I just had a very quick look, so I may be completely wrong. need to double check. what I tried to say: -- I will send out a patch for printk() once we settle down the current work in progress. why do I want to address this in printk? because who knows, may be there is (or there will be) something out there that takes rcu_read_lock() and then does the console_conditional_schedule(). rcu does not modify current preempt count, it has its own preempt counter, and get_console_may_schedule() takes that into account. > What happened here was: > > [...] I need more time to walk through your analysis/proposal. -ss -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 8b26964..ce53488 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -2186,6 +2186,7 @@ void console_unlock(void) bool wake_klogd = false; bool do_cond_resched, retry; +again: if (console_suspended) { up_console_sem(); return; @@ -2204,7 +2205,6 @@ void console_unlock(void) do_cond_resched = console_may_schedule; console_may_schedule = 0; -again: /* * 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