| Message ID | 20231127151248.7232-3-petr.pavlu@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tracing: Simplify and fix "buffered event" synchronization | expand |
On Mon, 27 Nov 2023 16:12:48 +0100 Petr Pavlu <petr.pavlu@suse.com> wrote: > Make the disable operation in __ftrace_event_enable_disable() use the > reverse order of the respective enable operation. > > This has two minor benefits: > * Disabling of buffered events via trace_buffered_event_disable() is > done after unregistering the trace event. It closes a small window > where an event would be still registered and could be hit, but would > unnecessarily go directly to a ring buffer. There's little benefit to the above. Going to the ring buffer and reverting it is just a bit more expensive, but should not be an issue with this small window. > * The SOFT_DISABLED flag is now consistently set only when SOFT_MODE is > also set. This code is a bit fragile, and I rather not change the logic. There's a lot of corner cases. I'm not saying that this is a bad change, I just don't want to add it and find out later it broke one of the corner cases. To add this would require an analysis that every input produces the same output with and without this change. If you want to make a table showing all inputs between soft_disable and the flags, and show that the result produces the same updates, I'll then reconsidered applying this. Thanks! -- Steve > > Signed-off-by: Petr Pavlu <petr.pavlu@suse.com> > --- > kernel/trace/trace_events.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c > index f29e815ca5b2..5f14d68a0ced 100644 > --- a/kernel/trace/trace_events.c > +++ b/kernel/trace/trace_events.c > @@ -633,9 +633,6 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file, > if (atomic_dec_return(&file->sm_ref) > 0) > break; > disable = file->flags & EVENT_FILE_FL_SOFT_DISABLED; > - clear_bit(EVENT_FILE_FL_SOFT_MODE_BIT, &file->flags); > - /* Disable use of trace_buffered_event */ > - trace_buffered_event_disable(); > } else > disable = !(file->flags & EVENT_FILE_FL_SOFT_MODE); > > @@ -653,11 +650,17 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file, > > call->class->reg(call, TRACE_REG_UNREGISTER, file); > } > - /* If in SOFT_MODE, just set the SOFT_DISABLE_BIT, else clear it */ > - if (file->flags & EVENT_FILE_FL_SOFT_MODE) > - set_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &file->flags); > - else > + > + if (soft_disable) { > + /* Complete going out of SOFT_MODE */ > clear_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &file->flags); > + clear_bit(EVENT_FILE_FL_SOFT_MODE_BIT, &file->flags); > + /* Disable use of trace_buffered_event */ > + trace_buffered_event_disable(); > + } else if (file->flags & EVENT_FILE_FL_SOFT_MODE) { > + /* If in SOFT_MODE, just set the SOFT_DISABLE_BIT */ > + set_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &file->flags); > + } > break; > case 1: > /*
On 11/27/23 18:58, Steven Rostedt wrote: > On Mon, 27 Nov 2023 16:12:48 +0100 > Petr Pavlu <petr.pavlu@suse.com> wrote: > >> Make the disable operation in __ftrace_event_enable_disable() use the >> reverse order of the respective enable operation. >> >> This has two minor benefits: >> * Disabling of buffered events via trace_buffered_event_disable() is >> done after unregistering the trace event. It closes a small window >> where an event would be still registered and could be hit, but would >> unnecessarily go directly to a ring buffer. > > There's little benefit to the above. Going to the ring buffer and reverting > it is just a bit more expensive, but should not be an issue with this small > window. > >> * The SOFT_DISABLED flag is now consistently set only when SOFT_MODE is >> also set. > > This code is a bit fragile, and I rather not change the logic. There's a > lot of corner cases. > > I'm not saying that this is a bad change, I just don't want to add it and > find out later it broke one of the corner cases. To add this would require > an analysis that every input produces the same output with and without this > change. > > If you want to make a table showing all inputs between soft_disable and the > flags, and show that the result produces the same updates, I'll then > reconsidered applying this. Ok, that is fair. It looked to me as a reasonable change but I don't feel strongly about it and I understand your concern. I guess I'll drop it in v2. Thanks, Petr
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c index f29e815ca5b2..5f14d68a0ced 100644 --- a/kernel/trace/trace_events.c +++ b/kernel/trace/trace_events.c @@ -633,9 +633,6 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file, if (atomic_dec_return(&file->sm_ref) > 0) break; disable = file->flags & EVENT_FILE_FL_SOFT_DISABLED; - clear_bit(EVENT_FILE_FL_SOFT_MODE_BIT, &file->flags); - /* Disable use of trace_buffered_event */ - trace_buffered_event_disable(); } else disable = !(file->flags & EVENT_FILE_FL_SOFT_MODE); @@ -653,11 +650,17 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file, call->class->reg(call, TRACE_REG_UNREGISTER, file); } - /* If in SOFT_MODE, just set the SOFT_DISABLE_BIT, else clear it */ - if (file->flags & EVENT_FILE_FL_SOFT_MODE) - set_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &file->flags); - else + + if (soft_disable) { + /* Complete going out of SOFT_MODE */ clear_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &file->flags); + clear_bit(EVENT_FILE_FL_SOFT_MODE_BIT, &file->flags); + /* Disable use of trace_buffered_event */ + trace_buffered_event_disable(); + } else if (file->flags & EVENT_FILE_FL_SOFT_MODE) { + /* If in SOFT_MODE, just set the SOFT_DISABLE_BIT */ + set_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &file->flags); + } break; case 1: /*
Make the disable operation in __ftrace_event_enable_disable() use the reverse order of the respective enable operation. This has two minor benefits: * Disabling of buffered events via trace_buffered_event_disable() is done after unregistering the trace event. It closes a small window where an event would be still registered and could be hit, but would unnecessarily go directly to a ring buffer. * The SOFT_DISABLED flag is now consistently set only when SOFT_MODE is also set. Signed-off-by: Petr Pavlu <petr.pavlu@suse.com> --- kernel/trace/trace_events.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)