| Message ID | 20220608144517.444659212@infradead.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | cpuidle,rcu: Cleanup the mess | expand |
On Wed 2022-06-08 16:27:47, Peter Zijlstra wrote: > The problem, per commit fc98c3c8c9dc ("printk: use rcuidle console > tracepoint"), was printk usage from the cpuidle path where RCU was > already disabled. > > Per the patches earlier in this series, this is no longer the case. My understanding is that this series reduces a lot the amount of code called with RCU disabled. As a result the particular printk() call mentioned by commit fc98c3c8c9dc ("printk: use rcuidle console tracepoint") is called with RCU enabled now. Hence this particular problem is fixed better way now. But is this true in general? Does this "prevent" calling printk() a safe way in code with RCU disabled? I am not sure if anyone cares. printk() is the best effort functionality because of the consoles code anyway. Also I wonder if anyone uses this trace_console(). Therefore if this patch allows to remove some tricky tracing code then it might be worth it. But if trace_console_rcuidle() variant is still going to be available then I would keep using it. Best Regards, Petr > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > kernel/printk/printk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -2238,7 +2238,7 @@ static u16 printk_sprint(char *text, u16 > } > } > > - trace_console_rcuidle(text, text_len); > + trace_console(text, text_len); > > return text_len; > } >
On Thu, Jun 09, 2022 at 11:16:46AM +0200, Petr Mladek wrote: > On Wed 2022-06-08 16:27:47, Peter Zijlstra wrote: > > The problem, per commit fc98c3c8c9dc ("printk: use rcuidle console > > tracepoint"), was printk usage from the cpuidle path where RCU was > > already disabled. > > > > Per the patches earlier in this series, this is no longer the case. > > My understanding is that this series reduces a lot the amount > of code called with RCU disabled. As a result the particular printk() > call mentioned by commit fc98c3c8c9dc ("printk: use rcuidle console > tracepoint") is called with RCU enabled now. Hence this particular > problem is fixed better way now. > > But is this true in general? > Does this "prevent" calling printk() a safe way in code with > RCU disabled? On x86_64, yes. Other architectures, less so. Specifically, the objtool noinstr validation pass will warn at build time (DEBUG_ENTRY=y) if any noinstr/cpuidle code does a call to non-vetted code like printk(). At the same time; there's a few hacks that allow WARN to work, but mostly if you hit WARN in entry/noinstr you get to keep the pieces in any case. On other architecture we'll need to rely on runtime coverage with PROVE_RCU. That is, if a splat like in the above mentioned commit happens again, we'll need to fix it by adjusting the callchain, not by mucking about with RCU state. > I am not sure if anyone cares. printk() is the best effort > functionality because of the consoles code anyway. Also I wonder > if anyone uses this trace_console(). This is the tracepoint used to spool all of printk into ftrace, I suspect there's users, but I haven't used it myself. > Therefore if this patch allows to remove some tricky tracing > code then it might be worth it. But if trace_console_rcuidle() > variant is still going to be available then I would keep using it. My ultimate goal is to delete trace_.*_rcuidle() and RCU_NONIDLE() entirely. We're close, but not quite there yet.
Sending again. The previous attempt was rejected by several recipients. It was caused by a mail server changes on my side. I am sorry for spamming those who got the 1st mail already. On Wed 2022-06-08 16:27:47, Peter Zijlstra wrote: > The problem, per commit fc98c3c8c9dc ("printk: use rcuidle console > tracepoint"), was printk usage from the cpuidle path where RCU was > already disabled. > > Per the patches earlier in this series, this is no longer the case. My understanding is that this series reduces a lot the amount of code called with RCU disabled. As a result the particular printk() call mentioned by commit fc98c3c8c9dc ("printk: use rcuidle console tracepoint") is called with RCU enabled now. Hence this particular problem is fixed better way now. But is this true in general? Does this "prevent" calling printk() a safe way in code with RCU disabled? I am not sure if anyone cares. printk() is the best effort functionality because of the consoles code anyway. Also I wonder if anyone uses this trace_console(). Therefore if this patch allows to remove some tricky tracing code then it might be worth it. But if trace_console_rcuidle() variant is still going to be available then I would keep using it. Best Regards, Petr > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > kernel/printk/printk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -2238,7 +2238,7 @@ static u16 printk_sprint(char *text, u16 > } > } > > - trace_console_rcuidle(text, text_len); > + trace_console(text, text_len); > > return text_len; > } >
My emails are getting rejected... Let me try web-interface Kudos to Petr for the questions and thanks to PeterZ for the answers. On Thu, Jun 9, 2022 at 7:02 PM Peter Zijlstra <peterz@infradead.org> wrote: > This is the tracepoint used to spool all of printk into ftrace, I > suspect there's users, but I haven't used it myself. I'm somewhat curious whether we can actually remove that trace event.
On Thu 2022-06-09 20:30:58, Sergey Senozhatsky wrote: > My emails are getting rejected... Let me try web-interface Bad day for mail sending. I have problems as well ;-) > Kudos to Petr for the questions and thanks to PeterZ for the answers. > > On Thu, Jun 9, 2022 at 7:02 PM Peter Zijlstra <peterz@infradead.org> wrote: > > This is the tracepoint used to spool all of printk into ftrace, I > > suspect there's users, but I haven't used it myself. > > I'm somewhat curious whether we can actually remove that trace event. Good question. Well, I think that it might be useful. It allows to see trace and printk messages together. It was ugly when it was in the console code. The new location in vprintk_store() allows to have it even "correctly" sorted (timestamp) against other tracing messages. Best Regards, Petr
On Thu 2022-06-09 12:02:04, Peter Zijlstra wrote: > On Thu, Jun 09, 2022 at 11:16:46AM +0200, Petr Mladek wrote: > > On Wed 2022-06-08 16:27:47, Peter Zijlstra wrote: > > > The problem, per commit fc98c3c8c9dc ("printk: use rcuidle console > > > tracepoint"), was printk usage from the cpuidle path where RCU was > > > already disabled. > > > > > Does this "prevent" calling printk() a safe way in code with > > RCU disabled? > > On x86_64, yes. Other architectures, less so. > > Specifically, the objtool noinstr validation pass will warn at build > time (DEBUG_ENTRY=y) if any noinstr/cpuidle code does a call to > non-vetted code like printk(). > > At the same time; there's a few hacks that allow WARN to work, but > mostly if you hit WARN in entry/noinstr you get to keep the pieces in > any case. > > On other architecture we'll need to rely on runtime coverage with > PROVE_RCU. That is, if a splat like in the above mentioned commit > happens again, we'll need to fix it by adjusting the callchain, not by > mucking about with RCU state. Makes sense. Feel free to use for this patch: Acked-by: Petr Mladek <pmladek@suse.com> > > Therefore if this patch allows to remove some tricky tracing > > code then it might be worth it. But if trace_console_rcuidle() > > variant is still going to be available then I would keep using it. > > My ultimate goal is to delete trace_.*_rcuidle() and RCU_NONIDLE() > entirely. We're close, but not quite there yet. I keep my fingers crossed. Best Regards, Petr
On Thu, Jun 9, 2022 at 10:06 PM Petr Mladek <pmladek@suse.com> wrote: > > Makes sense. Feel free to use for this patch: > > Acked-by: Petr Mladek <pmladek@suse.com> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
On Thu, Jun 9, 2022 at 10:02 PM Petr Mladek <pmladek@suse.com> wrote: > > On Thu 2022-06-09 20:30:58, Sergey Senozhatsky wrote: > > My emails are getting rejected... Let me try web-interface > > Bad day for mail sending. I have problems as well ;-) For me the problem is still there and apparently it's an "too many recipients" error. > > I'm somewhat curious whether we can actually remove that trace event. > > Good question. > > Well, I think that it might be useful. It allows to see trace and > printk messages together. Fair enough. Seems that back in 2011 people were pretty happy with it https://lore.kernel.org/all/1322161388.5366.54.camel@jlt3.sipsolutions.net/T/#m7bf6416f469119372191f22a6ecf653c5f7331d2 but... reportedly, one of the folks who Ack-ed it (*cough cough* PeterZ) has never used it. > It was ugly when it was in the console code. The new location > in vprintk_store() allows to have it even "correctly" sorted > (timestamp) against other tracing messages. That's true.
On Thu, 9 Jun 2022 15:02:20 +0200 Petr Mladek <pmladek@suse.com> wrote: > > I'm somewhat curious whether we can actually remove that trace event. > > Good question. > > Well, I think that it might be useful. It allows to see trace and > printk messages together. Yes people still use it. I was just asked about it at Kernel Recipes. That is, someone wanted printk mixed in with the tracing, and I told them about this event (which they didn't know about but was happy to hear that it existed). -- Steve
--- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -2238,7 +2238,7 @@ static u16 printk_sprint(char *text, u16 } } - trace_console_rcuidle(text, text_len); + trace_console(text, text_len); return text_len; }
The problem, per commit fc98c3c8c9dc ("printk: use rcuidle console tracepoint"), was printk usage from the cpuidle path where RCU was already disabled. Per the patches earlier in this series, this is no longer the case. Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- kernel/printk/printk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)