[RFC PATCH RESEND] tracing: add kernel documentation for trace_array_set_clr_event, trace_set_clr_event and supporting functions

[RFC PATCH RESEND] tracing: add kernel documentation for trace_array_set_clr_event, trace_set_clr_event and supporting functions

[Gabriele Paoloni]

As per Linux Kernel documentation guidelines
(https://docs.kernel.org/doc-guide/kernel-doc.html),
<<Every function that is exported to loadable modules using
EXPORT_SYMBOL or EXPORT_SYMBOL_GPL should have a kernel-doc
comment>>; hence this patch adds detailed kernel-doc headers
documentation for trace_array_set_clr_event, trace_set_clr_event
and the main functions in the respective call-trees that support
their functionalities.
For each of the documented functions, as part of the extensive
description, a set of "Function's expectations" are described in
a way that facilitate:
1) evaluating the current code and any proposed modification
   to behave as described;
2) writing kernel tests to verify the code to behave as described.

Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.com>
---
Re-sending as no feedbacks have been received.
---
 kernel/trace/trace_events.c | 125 +++++++++++++++++++++++++++++++-----
 1 file changed, 109 insertions(+), 16 deletions(-)

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 120531268abf..4bd1f6e73ef1 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -763,6 +763,54 @@ void trace_event_enable_tgid_record(bool enable)
 	} while_for_each_event_file();
 }
 
+/*
+ * __ftrace_event_enable_disable - enable or disable a trace event
+ * @file: trace event file associated with the event.
+ * @enable: 0 or 1 respectively to disable/enable the event (any other value is
+ * invalid).
+ * @soft_disable: 1 or 0 respectively to mark if the enable parameter IS or
+ * IS NOT a soft enable/disable.
+ *
+ * Function's expectations:
+ * - If soft_disable is 1 a reference counter associated with the trace
+ * event shall be increased or decreased according to the enable parameter
+ * being 1 (enable) or 0 (disable) respectively.
+ * If the reference counter is > 0 before the increase or after the decrease,
+ * no other actions shall be taken.
+ *
+ * - if soft_disable is 1 and the trace event reference counter is 0 before
+ * the increase or after the decrease, an enable value set to 0 or 1 shall set
+ * or clear the soft mode flag respectively; this is characterized by disabling
+ * or enabling the use of trace_buffered_event respectively.
+ *
+ * - If soft_disable is 1 and enable is 0 and the reference counter reaches
+ * zero and if the soft disabled flag is set (i.e. if the event was previously
+ * enabled with soft_disable = 1), tracing for the trace point event shall be
+ * disabled and the soft disabled flag shall be cleared.
+ *
+ * - If soft_disable is 0 and enable is 0, tracing for the trace point event
+ * shall be disabled only if the soft mode flag is clear (i.e. event previously
+ * enabled with soft_disable = 0). Additionally the soft disabled flag shall be
+ * set or cleared according to the soft mode flag being set or clear
+ * respectively.
+ *
+ * - If enable is 1, tracing for the trace point event shall be enabled (if
+ * previously disabled); in addition if soft_disable is 1 and the reference
+ * counter is 0 before the increase, the soft disabled flag shall be set.
+ *
+ * - When enabling or disabling tracing for the trace point event
+ * the flags associated with comms and tgids shall be checked and, if set,
+ * respectively tracing of comms and tgdis at sched_switch shall be
+ * enabled/disabled.
+ *
+ * Returns 0 on success, or any error returned by the event register or
+ * unregister callbacks.
+ *
+ * NOTE: in order to invoke this code in a thread-safe way, event_mutex shall
+ * be locked before calling it.
+ * NOTE: the validity of the input pointer file shall be checked by the caller
+ *
+ */
 static int __ftrace_event_enable_disable(struct trace_event_file *file,
 					 int enable, int soft_disable)
 {
@@ -1296,7 +1344,37 @@ static void remove_event_file_dir(struct trace_event_file *file)
 }
 
 /*
- * __ftrace_set_clr_event(NULL, NULL, NULL, set) will set/unset all events.
+ * __ftrace_set_clr_event_nolock - enable or disable an event within a system
+ * (thread-unsafe).
+ * @tr: target trace_array containing the events list (shall be a non-NULL
+ * valid pointer).
+ * @match: target system or event name (NULL for any).
+ * @sub: target system name (NULL for any).
+ * @event: target event name (NULL for any).
+ * @set: 1 to enable, 0 to disable (any other value is invalid).
+ * @mod: target module name (NULL for any).
+ *
+ * Function's expectations:
+ * - If mod is set, the mod name shall be sanitized by replacing all '-' with
+ *   '_' to match the modules' naming convention used in the Kernel.
+ * - From the events' list in the input tr, the ensemble of events to be enabled
+ *   or disabled shall be selected according to the input match, sub, event and
+ *   mod parameters. Each of these parameters, if set, shall restrict the events
+ *   ensemble to those with a matching parameter's name.
+ * - For each of the selected events the IGNORE_ENABLE flag shall be checked,
+ *   and, if not set, ftrace_event_enable_disable shall be invoked passing the
+ *   input set parameter to either enable or disable the event.
+ *
+ * Returns 0 on success, -EINVAL if the parameters do not match any registered
+ * events, -ENOMEM if memory allocation fails for the module pointer or the
+ * error condition returned by the first call to ftrace_event_enable_disable
+ * that returned an error.
+ *
+ * NOTE: in order to invoke this code in a thread-safe way, event_mutex shall
+ * be locked before calling it.
+ * NOTE: the validity of the input pointer tr shall be checked by the caller.
+ * NOTE: __ftrace_set_clr_event_nolock(NULL, NULL, NULL, set, NULL) will
+ * set/unset all events.
  */
 static int
 __ftrace_set_clr_event_nolock(struct trace_array *tr, const char *match,
@@ -1439,16 +1517,24 @@ int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
 }
 
 /**
- * trace_set_clr_event - enable or disable an event
- * @system: system name to match (NULL for any system)
- * @event: event name to match (NULL for all events, within system)
- * @set: 1 to enable, 0 to disable
+ * trace_set_clr_event - enable or disable an event within a system.
+ * @system: system name (NULL for any system).
+ * @event: event name (NULL for all events, within system).
+ * @set: 1 to enable, 0 to disable (any other value is invalid).
  *
  * This is a way for other parts of the kernel to enable or disable
  * event recording.
  *
- * Returns 0 on success, -EINVAL if the parameters do not match any
- * registered events.
+ * Function's expectations:
+ * - This function shall retrieve the pointer of the global trace array (global
+ *   tracer) and pass it, along the rest of input parameters, to
+ *   __ftrace_set_clr_event_nolock.
+ * - This function shall properly lock/unlock the global event_mutex
+ *   before/after invoking ftrace_set_clr_event_nolock.
+ *
+ * Returns 0 on success, -ENODEV if the global tracer cannot be retrieved,
+ * -EINVAL if the parameters do not match any registered events, any other
+ * error condition returned by __ftrace_set_clr_event_nolock.
  */
 int trace_set_clr_event(const char *system, const char *event, int set)
 {
@@ -1462,17 +1548,24 @@ int trace_set_clr_event(const char *system, const char *event, int set)
 EXPORT_SYMBOL_GPL(trace_set_clr_event);
 
 /**
- * trace_array_set_clr_event - enable or disable an event for a trace array.
- * @tr: concerned trace array.
- * @system: system name to match (NULL for any system)
- * @event: event name to match (NULL for all events, within system)
- * @enable: true to enable, false to disable
+ * trace_array_set_clr_event - enable or disable an event within a system for
+ * a trace array.
+ * @tr: input trace array.
+ * @system: system name (NULL for any system).
+ * @event: event name (NULL for all events, within system).
+ * @enable: true to enable, false to disable.
  *
- * This is a way for other parts of the kernel to enable or disable
- * event recording.
+ * This is a way for other parts of the kernel to enable or disable event
+ * recording.
+ *
+ * Function's expectations:
+ * - This function shall properly lock/unlock the global event_mutex
+ *   before/after invoking ftrace_set_clr_event_nolock passing along the same
+ *   input parameters.
  *
- * Returns 0 on success, -EINVAL if the parameters do not match any
- * registered events.
+ * Returns 0 on success, -ENOENT if the input tr is NULL, -EINVAL if the
+ * parameters do not match any registered events, any other error condition
+ * returned by __ftrace_set_clr_event_nolock.
  */
 int trace_array_set_clr_event(struct trace_array *tr, const char *system,
 		const char *event, bool enable)
-- 
2.48.1

Re: [RFC PATCH RESEND] tracing: add kernel documentation for trace_array_set_clr_event, trace_set_clr_event and supporting functions

[Steven Rostedt]

FYI, I know some maintainers prefer a "RESEND" of a patch, but I personally
prefer a simple "ping" reply to the patch. Actually, I'll take either, but
my workflow is with patchwork[1] and I tend to give older patches in
patchwork priority. By sending a patch again via "RESEND" that patch will
supersede the older patch which actually pushes the patch further down into
the queue, which makes it even longer for me to see it (having the opposite
effect of the purpose of sending "RESEND").

[1] https://patchwork.kernel.org/project/linux-trace-kernel/list/

On Fri, 20 Jun 2025 10:56:18 +0200
Gabriele Paoloni <gpaoloni@redhat.com> wrote:

> As per Linux Kernel documentation guidelines
> (https://docs.kernel.org/doc-guide/kernel-doc.html),
> <<Every function that is exported to loadable modules using
> EXPORT_SYMBOL or EXPORT_SYMBOL_GPL should have a kernel-doc
> comment>>; hence this patch adds detailed kernel-doc headers  
> documentation for trace_array_set_clr_event, trace_set_clr_event


When referencing functions, please add parenthesis "func()" when naming
them.

> and the main functions in the respective call-trees that support
> their functionalities.

Also add newlines in the change log, to make it visually easier to read.

> For each of the documented functions, as part of the extensive
> description, a set of "Function's expectations" are described in
> a way that facilitate:
> 1) evaluating the current code and any proposed modification
>    to behave as described;
> 2) writing kernel tests to verify the code to behave as described.
> 
> Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.com>
> ---
> Re-sending as no feedbacks have been received.
> ---
>  kernel/trace/trace_events.c | 125 +++++++++++++++++++++++++++++++-----
>  1 file changed, 109 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 120531268abf..4bd1f6e73ef1 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -763,6 +763,54 @@ void trace_event_enable_tgid_record(bool enable)
>  	} while_for_each_event_file();
>  }
>  
> +/*

If you are going to use kerneldoc comments, might as well make it a
kerneldoc format: /**

> + * __ftrace_event_enable_disable - enable or disable a trace event
> + * @file: trace event file associated with the event.
> + * @enable: 0 or 1 respectively to disable/enable the event (any other value is
> + * invalid).

Saying 0 or 1 should assume that those are the only values. Don't need the
content in the parenthesis.

> + * @soft_disable: 1 or 0 respectively to mark if the enable parameter IS or
> + * IS NOT a soft enable/disable.
> + *
> + * Function's expectations:
> + * - If soft_disable is 1 a reference counter associated with the trace
> + * event shall be increased or decreased according to the enable parameter
> + * being 1 (enable) or 0 (disable) respectively.
> + * If the reference counter is > 0 before the increase or after the decrease,
> + * no other actions shall be taken.

Although this has newlines (which I like!), the indentation should be with
the first word after the hyphen. That is, instead of:

 * - If soft_disable is 1 a reference counter associated with the trace
 * event shall be increased or decreased according to the enable parameter
 * being 1 (enable) or 0 (disable) respectively.
 * If the reference counter is > 0 before the increase or after the decrease,
 * no other actions shall be taken.

It should be:

 * - If soft_disable is 1 a reference counter associated with the trace
 *   event shall be increased or decreased according to the enable parameter
 *   being 1 (enable) or 0 (disable) respectively.
 *   If the reference counter is > 0 before the increase or after the decrease,
 *   no other actions shall be taken.

Making it easier to read.

> + *
> + * - if soft_disable is 1 and the trace event reference counter is 0 before
> + * the increase or after the decrease, an enable value set to 0 or 1 shall set
> + * or clear the soft mode flag respectively; this is characterized by disabling
> + * or enabling the use of trace_buffered_event respectively.
> + *
> + * - If soft_disable is 1 and enable is 0 and the reference counter reaches
> + * zero and if the soft disabled flag is set (i.e. if the event was previously
> + * enabled with soft_disable = 1), tracing for the trace point event shall be
> + * disabled and the soft disabled flag shall be cleared.

Would it be possible to group the requirements within "If soft_disable is
1"? Seeing three different lines starting with the same state seems
inefficient.

> + *
> + * - If soft_disable is 0 and enable is 0, tracing for the trace point event
> + * shall be disabled only if the soft mode flag is clear (i.e. event previously
> + * enabled with soft_disable = 0). Additionally the soft disabled flag shall be
> + * set or cleared according to the soft mode flag being set or clear
> + * respectively.
> + *
> + * - If enable is 1, tracing for the trace point event shall be enabled (if
> + * previously disabled); in addition if soft_disable is 1 and the reference
> + * counter is 0 before the increase, the soft disabled flag shall be set.
> + *
> + * - When enabling or disabling tracing for the trace point event
> + * the flags associated with comms and tgids shall be checked and, if set,
> + * respectively tracing of comms and tgdis at sched_switch shall be
> + * enabled/disabled.
> + *
> + * Returns 0 on success, or any error returned by the event register or
> + * unregister callbacks.
> + *
> + * NOTE: in order to invoke this code in a thread-safe way, event_mutex shall
> + * be locked before calling it.
> + * NOTE: the validity of the input pointer file shall be checked by the caller

I have to find some time to make sure the above is correct. I'm looking at
this more at a superficial level. And I'll look at the rest later.

Cheers,

-- Steve

Re: [RFC PATCH RESEND] tracing: add kernel documentation for trace_array_set_clr_event, trace_set_clr_event and supporting functions

[Gabriele Paoloni]

Hi Steven, many thanks for looking into this

On Wed, Jul 2, 2025 at 1:59 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
>
> FYI, I know some maintainers prefer a "RESEND" of a patch, but I personally
> prefer a simple "ping" reply to the patch. Actually, I'll take either, but
> my workflow is with patchwork[1] and I tend to give older patches in
> patchwork priority. By sending a patch again via "RESEND" that patch will
> supersede the older patch which actually pushes the patch further down into
> the queue, which makes it even longer for me to see it (having the opposite
> effect of the purpose of sending "RESEND").

Apologies, next time I'll ping instead of RESENDing (I recently
started following
this mailing list, so I was not aware).

>
> [1] https://patchwork.kernel.org/project/linux-trace-kernel/list/
>
> On Fri, 20 Jun 2025 10:56:18 +0200
> Gabriele Paoloni <gpaoloni@redhat.com> wrote:
>
> > As per Linux Kernel documentation guidelines
> > (https://docs.kernel.org/doc-guide/kernel-doc.html),
> > <<Every function that is exported to loadable modules using
> > EXPORT_SYMBOL or EXPORT_SYMBOL_GPL should have a kernel-doc
> > comment>>; hence this patch adds detailed kernel-doc headers
> > documentation for trace_array_set_clr_event, trace_set_clr_event
>
>
> When referencing functions, please add parenthesis "func()" when naming
> them.

Thanks, I'll change the commit msg in v2.


>
> > and the main functions in the respective call-trees that support
> > their functionalities.
>
> Also add newlines in the change log, to make it visually easier to read.

got it, will change in v2.

>
> > For each of the documented functions, as part of the extensive
> > description, a set of "Function's expectations" are described in
> > a way that facilitate:
> > 1) evaluating the current code and any proposed modification
> >    to behave as described;
> > 2) writing kernel tests to verify the code to behave as described.
> >
> > Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.com>
> > ---
> > Re-sending as no feedbacks have been received.

Now that I am reading this I realized that I missed the most important
discussion comments from v1, so I am adding them back here inline
below (BTW one more reason to avoid RESENDs):

While working on the documentation of __ftrace_event_enable_disable,
I realized that the EVENT_FILE_FL_SOFT_MODE flag is mainly used
internally in the function itself, whereas it is EVENT_FILE_FL_SOFT_DISABLED
that prevents tracing the event.
In this perspective I see that, starting from the initial state, if for
a specific event we invoke __ftrace_event_enable_disable with enable=1
and soft_disable=0, the EVENT_FILE_FL_ENABLED is set whereas
EVENT_FILE_FL_SOFT_MODE and EVENT_FILE_FL_SOFT_DISABLED are not.
Now if for that event we invoke __ftrace_event_enable_disable again with
enable=1 and soft_disable=1, EVENT_FILE_FL_ENABLED stays set,
EVENT_FILE_FL_SOFT_MODE is set, while EVENT_FILE_FL_SOFT_DISABLED
remains not set. Instead if from the initial state we directly invoke
__ftrace_event_enable_disable with enable=1 and soft_disable=1, all
the status flag mentioned above are all set (EVENT_FILE_FL_ENABLED,
EVENT_FILE_FL_SOFT_MODE and EVENT_FILE_FL_SOFT_DISABLED).
Now I wonder if:
a) such a behaviour is consistent with the code expectation;
b) if it would make sense to have a standard enable invocation followed
   by a soft enable invocation to end up in the same state as a single
   invocation of soft enable;
c) eventually if we could get rid of the soft_mode flag and simplify
   the code to only use the soft_disabled flag.

Probably there are aspects that I am missing and I really appreciate
your inputs/views.

> > ---
> >  kernel/trace/trace_events.c | 125 +++++++++++++++++++++++++++++++-----
> >  1 file changed, 109 insertions(+), 16 deletions(-)
> >
> > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> > index 120531268abf..4bd1f6e73ef1 100644
> > --- a/kernel/trace/trace_events.c
> > +++ b/kernel/trace/trace_events.c
> > @@ -763,6 +763,54 @@ void trace_event_enable_tgid_record(bool enable)
> >       } while_for_each_event_file();
> >  }
> >
> > +/*
>
> If you are going to use kerneldoc comments, might as well make it a
> kerneldoc format: /**

Uh this is bad, sorry I'll fix this in v2.

>
> > + * __ftrace_event_enable_disable - enable or disable a trace event
> > + * @file: trace event file associated with the event.
> > + * @enable: 0 or 1 respectively to disable/enable the event (any other value is
> > + * invalid).
>
> Saying 0 or 1 should assume that those are the only values. Don't need the
> content in the parenthesis.

Agreed, I'll remove it in v2.

>
> > + * @soft_disable: 1 or 0 respectively to mark if the enable parameter IS or
> > + * IS NOT a soft enable/disable.
> > + *
> > + * Function's expectations:
> > + * - If soft_disable is 1 a reference counter associated with the trace
> > + * event shall be increased or decreased according to the enable parameter
> > + * being 1 (enable) or 0 (disable) respectively.
> > + * If the reference counter is > 0 before the increase or after the decrease,
> > + * no other actions shall be taken.
>
> Although this has newlines (which I like!), the indentation should be with
> the first word after the hyphen. That is, instead of:
>
>  * - If soft_disable is 1 a reference counter associated with the trace
>  * event shall be increased or decreased according to the enable parameter
>  * being 1 (enable) or 0 (disable) respectively.
>  * If the reference counter is > 0 before the increase or after the decrease,
>  * no other actions shall be taken.
>
> It should be:
>
>  * - If soft_disable is 1 a reference counter associated with the trace
>  *   event shall be increased or decreased according to the enable parameter
>  *   being 1 (enable) or 0 (disable) respectively.
>  *   If the reference counter is > 0 before the increase or after the decrease,
>  *   no other actions shall be taken.
>
> Making it easier to read.

Agreed, will be changed in v2.

>
> > + *
> > + * - if soft_disable is 1 and the trace event reference counter is 0 before
> > + * the increase or after the decrease, an enable value set to 0 or 1 shall set
> > + * or clear the soft mode flag respectively; this is characterized by disabling
> > + * or enabling the use of trace_buffered_event respectively.
> > + *
> > + * - If soft_disable is 1 and enable is 0 and the reference counter reaches
> > + * zero and if the soft disabled flag is set (i.e. if the event was previously
> > + * enabled with soft_disable = 1), tracing for the trace point event shall be
> > + * disabled and the soft disabled flag shall be cleared.
>
> Would it be possible to group the requirements within "If soft_disable is
> 1"? Seeing three different lines starting with the same state seems
> inefficient.

Possibly yes but IMO it would not save much; e.g:
  - if soft_disable is 1:
    - if the trace event reference counter is 0 before the increase or after
      the decrease, an enable value set to 0 or 1 shall set or clear the soft
      mode flag respectively; this is characterized by disabling or enabling
      the use of trace_buffered_event respectively.

    - if enable is 0 and the reference counter reaches zero and the soft
      disabled flag is set (i.e. if the event was previously enabled with
      soft_disable = 1), tracing for the trace point event shall be disabled
      and the soft disabled flag shall be cleared.

However IMO we can revisit this point after we have a discussion on the
considerations that were missed in the RESEND process and that are
now pasted above

>
> > + *
> > + * - If soft_disable is 0 and enable is 0, tracing for the trace point event
> > + * shall be disabled only if the soft mode flag is clear (i.e. event previously
> > + * enabled with soft_disable = 0). Additionally the soft disabled flag shall be
> > + * set or cleared according to the soft mode flag being set or clear
> > + * respectively.
> > + *
> > + * - If enable is 1, tracing for the trace point event shall be enabled (if
> > + * previously disabled); in addition if soft_disable is 1 and the reference
> > + * counter is 0 before the increase, the soft disabled flag shall be set.
> > + *
> > + * - When enabling or disabling tracing for the trace point event
> > + * the flags associated with comms and tgids shall be checked and, if set,
> > + * respectively tracing of comms and tgdis at sched_switch shall be
> > + * enabled/disabled.
> > + *
> > + * Returns 0 on success, or any error returned by the event register or
> > + * unregister callbacks.

FYI I also noticed here that "Context" is missing and "Returns" should be
"Return:", so I'll fix this in v2

> > + *
> > + * NOTE: in order to invoke this code in a thread-safe way, event_mutex shall
> > + * be locked before calling it.
> > + * NOTE: the validity of the input pointer file shall be checked by the caller
>
> I have to find some time to make sure the above is correct. I'm looking at
> this more at a superficial level. And I'll look at the rest later.

Many Thanks!
Gab

>
> Cheers,
>
> -- Steve
>

Re: [RFC PATCH RESEND] tracing: add kernel documentation for trace_array_set_clr_event, trace_set_clr_event and supporting functions

[Steven Rostedt]

On Wed, 2 Jul 2025 15:45:41 +0200
Gabriele Paoloni <gpaoloni@redhat.com> wrote:

> >  
> > > For each of the documented functions, as part of the extensive
> > > description, a set of "Function's expectations" are described in
> > > a way that facilitate:
> > > 1) evaluating the current code and any proposed modification
> > >    to behave as described;
> > > 2) writing kernel tests to verify the code to behave as described.
> > >
> > > Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.com>
> > > ---
> > > Re-sending as no feedbacks have been received.  
> 
> Now that I am reading this I realized that I missed the most important
> discussion comments from v1, so I am adding them back here inline
> below (BTW one more reason to avoid RESENDs):
> 
> While working on the documentation of __ftrace_event_enable_disable,
> I realized that the EVENT_FILE_FL_SOFT_MODE flag is mainly used
> internally in the function itself, whereas it is EVENT_FILE_FL_SOFT_DISABLED
> that prevents tracing the event.
> In this perspective I see that, starting from the initial state, if for
> a specific event we invoke __ftrace_event_enable_disable with enable=1
> and soft_disable=0, the EVENT_FILE_FL_ENABLED is set whereas
> EVENT_FILE_FL_SOFT_MODE and EVENT_FILE_FL_SOFT_DISABLED are not.
> Now if for that event we invoke __ftrace_event_enable_disable again with
> enable=1 and soft_disable=1, EVENT_FILE_FL_ENABLED stays set,
> EVENT_FILE_FL_SOFT_MODE is set, while EVENT_FILE_FL_SOFT_DISABLED
> remains not set. Instead if from the initial state we directly invoke
> __ftrace_event_enable_disable with enable=1 and soft_disable=1, all
> the status flag mentioned above are all set (EVENT_FILE_FL_ENABLED,
> EVENT_FILE_FL_SOFT_MODE and EVENT_FILE_FL_SOFT_DISABLED).
> Now I wonder if:
> a) such a behaviour is consistent with the code expectation;

Yes, and I verified it by looking at the comments in the code. But this
should have been documented at the top of the function too.

> b) if it would make sense to have a standard enable invocation followed
>    by a soft enable invocation to end up in the same state as a single
>    invocation of soft enable;

No, because the two need to be done together.

> c) eventually if we could get rid of the soft_mode flag and simplify
>    the code to only use the soft_disabled flag.

The reason for the soft_mode flag is that this needs to handle two
users of the same event. One has it enabled (no soft mode at all) and
the other has it disabled in a soft mode.

The SOFT_MODE flag is to state that there's at least one user that is
using this in soft mode and has it disabled.

Let me explain the purpose of SOFT_MODE.

When you echo 1 into the enable file of an event it enables the event
and it starts tracing immediately. This would be: enable=1 soft_disable=0.

Same for echoing in 0 into the enable file. It would disable the event:
enable=0 soft_disable=0.

To enable or disable an event, it requires an expensive update to the
static branches to turn the nops in the code into calls to the tracing
infrastructure.

Now we have a feature where you could enable one event when another
event is hit with specific fields (or any field).

  echo 'enable_event:irq:irq_handler_entry if next_comm=="migrate"' > /sys/kernel/tracing/events/sched/sched_switch/trigger

The above adds a trigger to the sched_switch event where if the
next_comm is "migrate", it will enable the irq:irq_handler_entry event.

But to enable an event from an event handler which doesn't allow
sleeping or locking, it can't simply call
__ftrace_event_enable_disable() to enable the event. That would most
likely cause a kernel crash or lockup if it did.

To handle this case, when the trigger is added, it enables the event
but puts the event into "soft mode" disabled. The trigger code would
call __ftrace_event_enable_disable() with enable=1 and soft_disable=1.
Meaning, "enable this event, but also set the soft_disable bit".

This enables the event with the soft_disable flag set. That means, the
irq_handler_entry event will call into the tracing system every time.
But because the SOFT_DISABLE is set in the event, it will simply do
nothing.

After doing the above trigger:

  # cat /sys/kernel/tracing/events/irq/irq_handler_entry/enable 
  0*

That means it's disabled in "soft mode".

But let's say I also want to enable the event!

  # echo 1 > /sys/kernel/tracing/events/irq/irq_handler_entry/enable 
  # cat /sys/kernel/tracing/events/irq/irq_handler_entry/enable 
  1*

The above called __ftrace_event_enable_disable() with: enable=1 and soft_disable=0.

Which means "enable this event for real". Well, it can't forget that
there's a trigger on it. But the event shouldn't be ignored when
triggered, so it will clear the SOFT_DISABLE flag and have the event be
traced.

But if we disable it again:

  # echo 0 > /sys/kernel/tracing/events/irq/irq_handler_entry/enable 
  # cat /sys/kernel/tracing/events/irq/irq_handler_entry/enable 
  0*

It must still remember that there's a user that has this event soft
disabled and may enable it in the future.

When the trigger fires, it will clear the SOFT_DISABLE bit making the
event "enabled".

The __ftrace_event_enable_disable() needs to keep track of all the
users that have this event enabled but will switch between soft disable
and enable. To add itself, it calls this function with enable=1
soft_disable=1 and to remove itself, it calls it with enable=0 and
soft_disable=1.

Now technically, the SOFT_MODE bit should only be set iff the ref count
is greater than zero. But it's easier to test a flag than to always
test a ref count.

Hope that explains this better. And yes, it can get somewhat confusing,
which is why I said this is a good function to document the
requirements for ;-)

-- Steve

Re: [RFC PATCH RESEND] tracing: add kernel documentation for trace_array_set_clr_event, trace_set_clr_event and supporting functions

[Steven Rostedt]

On Wed, 2 Jul 2025 10:40:58 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> Now technically, the SOFT_MODE bit should only be set iff the ref count
> is greater than zero. But it's easier to test a flag than to always
> test a ref count.

So the reason for why we have both a SOFT_MODE flag and a ref count is
because the original code expected only one user of the SOFT_MODE case.
But in 2013, Masami found that if two users used soft mode the
accounting broke and added a ref count. This does make the SOFT_MODE
flag redundant.

I just tried to remove it and I think it can work. Instead of using a
SOFT_MODE flag in the file flags field, simply using the ref counter is
just as fine.

Also note, I'm not sure there's any reason the ref counter is an atomic
value. All updates to it are done under the event_mutex lock. But that
can be another clean up.

-- Steve

diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index fa9cf4292dff..04307a19cde3 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -480,7 +480,6 @@ enum {
 	EVENT_FILE_FL_RECORDED_TGID_BIT,
 	EVENT_FILE_FL_FILTERED_BIT,
 	EVENT_FILE_FL_NO_SET_FILTER_BIT,
-	EVENT_FILE_FL_SOFT_MODE_BIT,
 	EVENT_FILE_FL_SOFT_DISABLED_BIT,
 	EVENT_FILE_FL_TRIGGER_MODE_BIT,
 	EVENT_FILE_FL_TRIGGER_COND_BIT,
@@ -618,7 +617,6 @@ extern int __kprobe_event_add_fields(struct dynevent_cmd *cmd, ...);
  *  RECORDED_TGID - The tgids should be recorded at sched_switch
  *  FILTERED	  - The event has a filter attached
  *  NO_SET_FILTER - Set when filter has error and is to be ignored
- *  SOFT_MODE     - The event is enabled/disabled by SOFT_DISABLED
  *  SOFT_DISABLED - When set, do not trace the event (even though its
  *                   tracepoint may be enabled)
  *  TRIGGER_MODE  - When set, invoke the triggers associated with the event
@@ -633,7 +631,6 @@ enum {
 	EVENT_FILE_FL_RECORDED_TGID	= (1 << EVENT_FILE_FL_RECORDED_TGID_BIT),
 	EVENT_FILE_FL_FILTERED		= (1 << EVENT_FILE_FL_FILTERED_BIT),
 	EVENT_FILE_FL_NO_SET_FILTER	= (1 << EVENT_FILE_FL_NO_SET_FILTER_BIT),
-	EVENT_FILE_FL_SOFT_MODE		= (1 << EVENT_FILE_FL_SOFT_MODE_BIT),
 	EVENT_FILE_FL_SOFT_DISABLED	= (1 << EVENT_FILE_FL_SOFT_DISABLED_BIT),
 	EVENT_FILE_FL_TRIGGER_MODE	= (1 << EVENT_FILE_FL_TRIGGER_MODE_BIT),
 	EVENT_FILE_FL_TRIGGER_COND	= (1 << EVENT_FILE_FL_TRIGGER_COND_BIT),
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 120531268abf..0980f4def360 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -768,6 +768,7 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
 {
 	struct trace_event_call *call = file->event_call;
 	struct trace_array *tr = file->tr;
+	bool soft_mode = atomic_read(&file->sm_ref) != 0;
 	int ret = 0;
 	int disable;
 
@@ -782,7 +783,7 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
 		 * is set we do not want the event to be enabled before we
 		 * clear the bit.
 		 *
-		 * When soft_disable is not set but the SOFT_MODE flag is,
+		 * When soft_disable is not set but the soft_mode is,
 		 * we do nothing. Do not disable the tracepoint, otherwise
 		 * "soft enable"s (clearing the SOFT_DISABLED bit) wont work.
 		 */
@@ -790,11 +791,11 @@ 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);
+			soft_mode = false;
 			/* Disable use of trace_buffered_event */
 			trace_buffered_event_disable();
 		} else
-			disable = !(file->flags & EVENT_FILE_FL_SOFT_MODE);
+			disable = !soft_mode;
 
 		if (disable && (file->flags & EVENT_FILE_FL_ENABLED)) {
 			clear_bit(EVENT_FILE_FL_ENABLED_BIT, &file->flags);
@@ -812,8 +813,8 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
 
 			WARN_ON_ONCE(ret);
 		}
-		/* If in SOFT_MODE, just set the SOFT_DISABLE_BIT, else clear it */
-		if (file->flags & EVENT_FILE_FL_SOFT_MODE)
+		/* If in soft mode, just set the SOFT_DISABLE_BIT, else clear it */
+		if (soft_mode)
 			set_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &file->flags);
 		else
 			clear_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &file->flags);
@@ -823,7 +824,7 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
 		 * When soft_disable is set and enable is set, we want to
 		 * register the tracepoint for the event, but leave the event
 		 * as is. That means, if the event was already enabled, we do
-		 * nothing (but set SOFT_MODE). If the event is disabled, we
+		 * nothing (but set soft_mode). If the event is disabled, we
 		 * set SOFT_DISABLED before enabling the event tracepoint, so
 		 * it still seems to be disabled.
 		 */
@@ -832,7 +833,7 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
 		else {
 			if (atomic_inc_return(&file->sm_ref) > 1)
 				break;
-			set_bit(EVENT_FILE_FL_SOFT_MODE_BIT, &file->flags);
+			soft_mode = true;
 			/* Enable use of trace_buffered_event */
 			trace_buffered_event_enable();
 		}
@@ -840,7 +841,7 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
 		if (!(file->flags & EVENT_FILE_FL_ENABLED)) {
 			bool cmd = false, tgid = false;
 
-			/* Keep the event disabled, when going to SOFT_MODE. */
+			/* Keep the event disabled, when going to soft mode. */
 			if (soft_disable)
 				set_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &file->flags);
 
@@ -1792,8 +1793,7 @@ event_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
 	    !(flags & EVENT_FILE_FL_SOFT_DISABLED))
 		strcpy(buf, "1");
 
-	if (flags & EVENT_FILE_FL_SOFT_DISABLED ||
-	    flags & EVENT_FILE_FL_SOFT_MODE)
+	if (atomic_read(&file->sm_ref) != 0)
 		strcat(buf, "*");
 
 	strcat(buf, "\n");
@@ -3584,7 +3584,7 @@ static int probe_remove_event_call(struct trace_event_call *call)
 			continue;
 		/*
 		 * We can't rely on ftrace_event_enable_disable(enable => 0)
-		 * we are going to do, EVENT_FILE_FL_SOFT_MODE can suppress
+		 * we are going to do, soft mode can suppress
 		 * TRACE_REG_UNREGISTER.
 		 */
 		if (file->flags & EVENT_FILE_FL_ENABLED)
@@ -3997,7 +3997,7 @@ static int free_probe_data(void *data)
 
 	edata->ref--;
 	if (!edata->ref) {
-		/* Remove the SOFT_MODE flag */
+		/* Remove soft mode */
 		__ftrace_event_enable_disable(edata->file, 0, 1);
 		trace_event_put_ref(edata->file->event_call);
 		kfree(edata);

Re: [RFC PATCH RESEND] tracing: add kernel documentation for trace_array_set_clr_event, trace_set_clr_event and supporting functions

[Gabriele Paoloni]

On Wed, Jul 2, 2025 at 4:41 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 2 Jul 2025 15:45:41 +0200
> Gabriele Paoloni <gpaoloni@redhat.com> wrote:
>
> > >
> > > > For each of the documented functions, as part of the extensive
> > > > description, a set of "Function's expectations" are described in
> > > > a way that facilitate:
> > > > 1) evaluating the current code and any proposed modification
> > > >    to behave as described;
> > > > 2) writing kernel tests to verify the code to behave as described.
> > > >
> > > > Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.com>
> > > > ---
> > > > Re-sending as no feedbacks have been received.
> >
> > Now that I am reading this I realized that I missed the most important
> > discussion comments from v1, so I am adding them back here inline
> > below (BTW one more reason to avoid RESENDs):
> >
> > While working on the documentation of __ftrace_event_enable_disable,
> > I realized that the EVENT_FILE_FL_SOFT_MODE flag is mainly used
> > internally in the function itself, whereas it is EVENT_FILE_FL_SOFT_DISABLED
> > that prevents tracing the event.
> > In this perspective I see that, starting from the initial state, if for
> > a specific event we invoke __ftrace_event_enable_disable with enable=1
> > and soft_disable=0, the EVENT_FILE_FL_ENABLED is set whereas
> > EVENT_FILE_FL_SOFT_MODE and EVENT_FILE_FL_SOFT_DISABLED are not.
> > Now if for that event we invoke __ftrace_event_enable_disable again with
> > enable=1 and soft_disable=1, EVENT_FILE_FL_ENABLED stays set,
> > EVENT_FILE_FL_SOFT_MODE is set, while EVENT_FILE_FL_SOFT_DISABLED
> > remains not set. Instead if from the initial state we directly invoke
> > __ftrace_event_enable_disable with enable=1 and soft_disable=1, all
> > the status flag mentioned above are all set (EVENT_FILE_FL_ENABLED,
> > EVENT_FILE_FL_SOFT_MODE and EVENT_FILE_FL_SOFT_DISABLED).
> > Now I wonder if:
> > a) such a behaviour is consistent with the code expectation;
>
> Yes, and I verified it by looking at the comments in the code. But this
> should have been documented at the top of the function too.

Ok thanks, in the next revision I will also add info that
contextualises the possible
state transitions.

>
> > b) if it would make sense to have a standard enable invocation followed
> >    by a soft enable invocation to end up in the same state as a single
> >    invocation of soft enable;
>
> No, because the two need to be done together.

Yes, following your explanation below I see now.

>
> > c) eventually if we could get rid of the soft_mode flag and simplify
> >    the code to only use the soft_disabled flag.
>
> The reason for the soft_mode flag is that this needs to handle two
> users of the same event. One has it enabled (no soft mode at all) and
> the other has it disabled in a soft mode.
>
> The SOFT_MODE flag is to state that there's at least one user that is
> using this in soft mode and has it disabled.
>
> Let me explain the purpose of SOFT_MODE.
>
> When you echo 1 into the enable file of an event it enables the event
> and it starts tracing immediately. This would be: enable=1 soft_disable=0.
>
> Same for echoing in 0 into the enable file. It would disable the event:
> enable=0 soft_disable=0.
>
> To enable or disable an event, it requires an expensive update to the
> static branches to turn the nops in the code into calls to the tracing
> infrastructure.
>
> Now we have a feature where you could enable one event when another
> event is hit with specific fields (or any field).
>
>   echo 'enable_event:irq:irq_handler_entry if next_comm=="migrate"' > /sys/kernel/tracing/events/sched/sched_switch/trigger
>
> The above adds a trigger to the sched_switch event where if the
> next_comm is "migrate", it will enable the irq:irq_handler_entry event.
>
> But to enable an event from an event handler which doesn't allow
> sleeping or locking, it can't simply call
> __ftrace_event_enable_disable() to enable the event. That would most
> likely cause a kernel crash or lockup if it did.
>
> To handle this case, when the trigger is added, it enables the event
> but puts the event into "soft mode" disabled. The trigger code would
> call __ftrace_event_enable_disable() with enable=1 and soft_disable=1.
> Meaning, "enable this event, but also set the soft_disable bit".
>
> This enables the event with the soft_disable flag set. That means, the
> irq_handler_entry event will call into the tracing system every time.
> But because the SOFT_DISABLE is set in the event, it will simply do
> nothing.
>
> After doing the above trigger:
>
>   # cat /sys/kernel/tracing/events/irq/irq_handler_entry/enable
>   0*
>
> That means it's disabled in "soft mode".
>
> But let's say I also want to enable the event!
>
>   # echo 1 > /sys/kernel/tracing/events/irq/irq_handler_entry/enable
>   # cat /sys/kernel/tracing/events/irq/irq_handler_entry/enable
>   1*
>
> The above called __ftrace_event_enable_disable() with: enable=1 and soft_disable=0.
>
> Which means "enable this event for real". Well, it can't forget that
> there's a trigger on it. But the event shouldn't be ignored when
> triggered, so it will clear the SOFT_DISABLE flag and have the event be
> traced.
>
> But if we disable it again:
>
>   # echo 0 > /sys/kernel/tracing/events/irq/irq_handler_entry/enable
>   # cat /sys/kernel/tracing/events/irq/irq_handler_entry/enable
>   0*
>
> It must still remember that there's a user that has this event soft
> disabled and may enable it in the future.
>
> When the trigger fires, it will clear the SOFT_DISABLE bit making the
> event "enabled".
>
> The __ftrace_event_enable_disable() needs to keep track of all the
> users that have this event enabled but will switch between soft disable
> and enable. To add itself, it calls this function with enable=1
> soft_disable=1 and to remove itself, it calls it with enable=0 and
> soft_disable=1.
>
> Now technically, the SOFT_MODE bit should only be set iff the ref count
> is greater than zero. But it's easier to test a flag than to always
> test a ref count.
>
> Hope that explains this better. And yes, it can get somewhat confusing,
> which is why I said this is a good function to document the
> requirements for ;-)

I think I understand now:
SOFT_MODE means that one user requested a soft enable for the event,
however that does not guarantee that the event is not already enabled from
a previous standard enable (in which case the SOFT_DISABLED flag will
not be set as it would compromise the previous user).
So a soft disable request is needed to "undo" previous soft enables; this in
consideration of the SOFT_DISABLED flag (telling if there is an active user
with a standard enable).

Many thanks! This really helps to better contextualize function expectations
in the next revision of the patch.

Gab

>
> -- Steve
>

Re: [RFC PATCH RESEND] tracing: add kernel documentation for trace_array_set_clr_event, trace_set_clr_event and supporting functions

[Gabriele Paoloni]

On Wed, Jul 2, 2025 at 5:19 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 2 Jul 2025 10:40:58 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > Now technically, the SOFT_MODE bit should only be set iff the ref count
> > is greater than zero. But it's easier to test a flag than to always
> > test a ref count.
>
> So the reason for why we have both a SOFT_MODE flag and a ref count is
> because the original code expected only one user of the SOFT_MODE case.
> But in 2013, Masami found that if two users used soft mode the
> accounting broke and added a ref count. This does make the SOFT_MODE
> flag redundant.
>
> I just tried to remove it and I think it can work. Instead of using a
> SOFT_MODE flag in the file flags field, simply using the ref counter is
> just as fine.
>
> Also note, I'm not sure there's any reason the ref counter is an atomic
> value. All updates to it are done under the event_mutex lock. But that
> can be another clean up.

I also cannot see a reason for an atomic counter. Please let me know
if you want me to add this cleanup patch as part of next patchset
revision (if so the doc patch will change accordingly).

Many Thanks
Gab

>
> -- Steve
>
> diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
> index fa9cf4292dff..04307a19cde3 100644
> --- a/include/linux/trace_events.h
> +++ b/include/linux/trace_events.h
> @@ -480,7 +480,6 @@ enum {
>         EVENT_FILE_FL_RECORDED_TGID_BIT,
>         EVENT_FILE_FL_FILTERED_BIT,
>         EVENT_FILE_FL_NO_SET_FILTER_BIT,
> -       EVENT_FILE_FL_SOFT_MODE_BIT,
>         EVENT_FILE_FL_SOFT_DISABLED_BIT,
>         EVENT_FILE_FL_TRIGGER_MODE_BIT,
>         EVENT_FILE_FL_TRIGGER_COND_BIT,
> @@ -618,7 +617,6 @@ extern int __kprobe_event_add_fields(struct dynevent_cmd *cmd, ...);
>   *  RECORDED_TGID - The tgids should be recorded at sched_switch
>   *  FILTERED     - The event has a filter attached
>   *  NO_SET_FILTER - Set when filter has error and is to be ignored
> - *  SOFT_MODE     - The event is enabled/disabled by SOFT_DISABLED
>   *  SOFT_DISABLED - When set, do not trace the event (even though its
>   *                   tracepoint may be enabled)
>   *  TRIGGER_MODE  - When set, invoke the triggers associated with the event
> @@ -633,7 +631,6 @@ enum {
>         EVENT_FILE_FL_RECORDED_TGID     = (1 << EVENT_FILE_FL_RECORDED_TGID_BIT),
>         EVENT_FILE_FL_FILTERED          = (1 << EVENT_FILE_FL_FILTERED_BIT),
>         EVENT_FILE_FL_NO_SET_FILTER     = (1 << EVENT_FILE_FL_NO_SET_FILTER_BIT),
> -       EVENT_FILE_FL_SOFT_MODE         = (1 << EVENT_FILE_FL_SOFT_MODE_BIT),
>         EVENT_FILE_FL_SOFT_DISABLED     = (1 << EVENT_FILE_FL_SOFT_DISABLED_BIT),
>         EVENT_FILE_FL_TRIGGER_MODE      = (1 << EVENT_FILE_FL_TRIGGER_MODE_BIT),
>         EVENT_FILE_FL_TRIGGER_COND      = (1 << EVENT_FILE_FL_TRIGGER_COND_BIT),
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 120531268abf..0980f4def360 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -768,6 +768,7 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
>  {
>         struct trace_event_call *call = file->event_call;
>         struct trace_array *tr = file->tr;
> +       bool soft_mode = atomic_read(&file->sm_ref) != 0;
>         int ret = 0;
>         int disable;
>
> @@ -782,7 +783,7 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
>                  * is set we do not want the event to be enabled before we
>                  * clear the bit.
>                  *
> -                * When soft_disable is not set but the SOFT_MODE flag is,
> +                * When soft_disable is not set but the soft_mode is,
>                  * we do nothing. Do not disable the tracepoint, otherwise
>                  * "soft enable"s (clearing the SOFT_DISABLED bit) wont work.
>                  */
> @@ -790,11 +791,11 @@ 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);
> +                       soft_mode = false;
>                         /* Disable use of trace_buffered_event */
>                         trace_buffered_event_disable();
>                 } else
> -                       disable = !(file->flags & EVENT_FILE_FL_SOFT_MODE);
> +                       disable = !soft_mode;
>
>                 if (disable && (file->flags & EVENT_FILE_FL_ENABLED)) {
>                         clear_bit(EVENT_FILE_FL_ENABLED_BIT, &file->flags);
> @@ -812,8 +813,8 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
>
>                         WARN_ON_ONCE(ret);
>                 }
> -               /* If in SOFT_MODE, just set the SOFT_DISABLE_BIT, else clear it */
> -               if (file->flags & EVENT_FILE_FL_SOFT_MODE)
> +               /* If in soft mode, just set the SOFT_DISABLE_BIT, else clear it */
> +               if (soft_mode)
>                         set_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &file->flags);
>                 else
>                         clear_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &file->flags);
> @@ -823,7 +824,7 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
>                  * When soft_disable is set and enable is set, we want to
>                  * register the tracepoint for the event, but leave the event
>                  * as is. That means, if the event was already enabled, we do
> -                * nothing (but set SOFT_MODE). If the event is disabled, we
> +                * nothing (but set soft_mode). If the event is disabled, we
>                  * set SOFT_DISABLED before enabling the event tracepoint, so
>                  * it still seems to be disabled.
>                  */
> @@ -832,7 +833,7 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
>                 else {
>                         if (atomic_inc_return(&file->sm_ref) > 1)
>                                 break;
> -                       set_bit(EVENT_FILE_FL_SOFT_MODE_BIT, &file->flags);
> +                       soft_mode = true;
>                         /* Enable use of trace_buffered_event */
>                         trace_buffered_event_enable();
>                 }
> @@ -840,7 +841,7 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
>                 if (!(file->flags & EVENT_FILE_FL_ENABLED)) {
>                         bool cmd = false, tgid = false;
>
> -                       /* Keep the event disabled, when going to SOFT_MODE. */
> +                       /* Keep the event disabled, when going to soft mode. */
>                         if (soft_disable)
>                                 set_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &file->flags);
>
> @@ -1792,8 +1793,7 @@ event_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
>             !(flags & EVENT_FILE_FL_SOFT_DISABLED))
>                 strcpy(buf, "1");
>
> -       if (flags & EVENT_FILE_FL_SOFT_DISABLED ||
> -           flags & EVENT_FILE_FL_SOFT_MODE)
> +       if (atomic_read(&file->sm_ref) != 0)
>                 strcat(buf, "*");
>
>         strcat(buf, "\n");
> @@ -3584,7 +3584,7 @@ static int probe_remove_event_call(struct trace_event_call *call)
>                         continue;
>                 /*
>                  * We can't rely on ftrace_event_enable_disable(enable => 0)
> -                * we are going to do, EVENT_FILE_FL_SOFT_MODE can suppress
> +                * we are going to do, soft mode can suppress
>                  * TRACE_REG_UNREGISTER.
>                  */
>                 if (file->flags & EVENT_FILE_FL_ENABLED)
> @@ -3997,7 +3997,7 @@ static int free_probe_data(void *data)
>
>         edata->ref--;
>         if (!edata->ref) {
> -               /* Remove the SOFT_MODE flag */
> +               /* Remove soft mode */
>                 __ftrace_event_enable_disable(edata->file, 0, 1);
>                 trace_event_put_ref(edata->file->event_call);
>                 kfree(edata);
>

Re: [RFC PATCH RESEND] tracing: add kernel documentation for trace_array_set_clr_event, trace_set_clr_event and supporting functions

[Steven Rostedt]

On Wed, 2 Jul 2025 19:16:56 +0200
Gabriele Paoloni <gpaoloni@redhat.com> wrote:

> I also cannot see a reason for an atomic counter. Please let me know
> if you want me to add this cleanup patch as part of next patchset
> revision (if so the doc patch will change accordingly).

You can send a patch but it shouldn't be related to this series.

Just send it separately.

-- Steve

Re: [RFC PATCH RESEND] tracing: add kernel documentation for trace_array_set_clr_event, trace_set_clr_event and supporting functions

[Masami Hiramatsu]

On Tue, 1 Jul 2025 19:59:39 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> > + * __ftrace_event_enable_disable - enable or disable a trace event
> > + * @file: trace event file associated with the event.
> > + * @enable: 0 or 1 respectively to disable/enable the event (any other value is
> > + * invalid).
> 
> Saying 0 or 1 should assume that those are the only values. Don't need the
> content in the parenthesis.

BTW, it should be "0 or !0"? (or we should make it boolean)
This description means if it is "2", that is undefined.

Thanks,

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [RFC PATCH RESEND] tracing: add kernel documentation for trace_array_set_clr_event, trace_set_clr_event and supporting functions

[Steven Rostedt]

On Wed, 9 Jul 2025 10:06:26 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> On Tue, 1 Jul 2025 19:59:39 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > > + * __ftrace_event_enable_disable - enable or disable a trace event
> > > + * @file: trace event file associated with the event.
> > > + * @enable: 0 or 1 respectively to disable/enable the event (any other value is
> > > + * invalid).  
> > 
> > Saying 0 or 1 should assume that those are the only values. Don't need the
> > content in the parenthesis.  
> 
> BTW, it should be "0 or !0"? (or we should make it boolean)
> This description means if it is "2", that is undefined.

Hmm, now here's an interesting point. So this is to define requirements
of a function based on what the function is doing. But does the
function have to have strict requirements?

If it can handle "0" or "!0" does that mean that's how it will be
defined? Or can it just state "0" or "1" and yes "2" is UB. That is,
it's not part of the requirements but if someone passes in 2, it could
act as a 1 as it's UB and implementation defined. Not a requirement.

-- Steve

Re: [RFC PATCH RESEND] tracing: add kernel documentation for trace_array_set_clr_event, trace_set_clr_event and supporting functions

[Gabriele Paoloni]

[resending as my previous reply had HTML stuff and was rejected]
Apologies for duplicates

On Wed, Jul 9, 2025 at 3:25 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 9 Jul 2025 10:06:26 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
>
> > On Tue, 1 Jul 2025 19:59:39 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > > > + * __ftrace_event_enable_disable - enable or disable a trace event
> > > > + * @file: trace event file associated with the event.
> > > > + * @enable: 0 or 1 respectively to disable/enable the event (any other value is
> > > > + * invalid).
> > >
> > > Saying 0 or 1 should assume that those are the only values. Don't need the
> > > content in the parenthesis.
> >
> > BTW, it should be "0 or !0"? (or we should make it boolean)
> > This description means if it is "2", that is undefined.
>
> Hmm, now here's an interesting point. So this is to define requirements
> of a function based on what the function is doing. But does the
> function have to have strict requirements?

IMO one of the main goals for these requirements is testability.
In order to have testable requirements we should state what the
valid input values are. In this case:
0 -> disable, 1 -> enable, everything else -> Error.

Now checking the code again it seems that the switch statement
is missing a default "ret = -EINVAL" (or else it should be changed
to boolean, but I guess it would have a wider impact on the rest
of the code...).

>
> If it can handle "0" or "!0" does that mean that's how it will be
> defined? Or can it just state "0" or "1" and yes "2" is UB. That is,
> it's not part of the requirements but if someone passes in 2, it could
> act as a 1 as it's UB and implementation defined. Not a requirement.

Right now if 2 is passed the function would silently return success,
but do we have a use case for this? I am trying to understand
where the implementation defined behavior would be....

Thanks
Gab

>
> -- Steve
>

Re: [RFC PATCH RESEND] tracing: add kernel documentation for trace_array_set_clr_event, trace_set_clr_event and supporting functions

[Steven Rostedt]

On Wed, 9 Jul 2025 15:35:50 +0200
Gabriele Paoloni <gpaoloni@redhat.com> wrote:

> > Hmm, now here's an interesting point. So this is to define requirements
> > of a function based on what the function is doing. But does the
> > function have to have strict requirements?  
> 
> IMO one of the main goals for these requirements is testability.
> In order to have testable requirements we should state what the
> valid input values are. In this case:
> 0 -> disable, 1 -> enable, everything else -> Error.
> 
> Now checking the code again it seems that the switch statement
> is missing a default "ret = -EINVAL" (or else it should be changed
> to boolean, but I guess it would have a wider impact on the rest
> of the code...).

Well, it's mostly used internally and the only places that call it uses
0 or 1, so there's never been any issue.

> 
> >
> > If it can handle "0" or "!0" does that mean that's how it will be
> > defined? Or can it just state "0" or "1" and yes "2" is UB. That is,
> > it's not part of the requirements but if someone passes in 2, it could
> > act as a 1 as it's UB and implementation defined. Not a requirement.  
> 
> Right now if 2 is passed the function would silently return success,
> but do we have a use case for this? I am trying to understand
> where the implementation defined behavior would be....

The issue is that all the callers pass in the proper value, and that
can be easily verified, but by adding the "anything else ERROR", it
would require adding more code that is not needed.

I rather just switch that and soft_disable into a boolean than to add
superficial error checks.

-- Steve

Re: [RFC PATCH RESEND] tracing: add kernel documentation for trace_array_set_clr_event, trace_set_clr_event and supporting functions

[Gabriele Paoloni]

On Wed, Jul 9, 2025 at 4:12 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 9 Jul 2025 15:35:50 +0200
> Gabriele Paoloni <gpaoloni@redhat.com> wrote:
>
> > > Hmm, now here's an interesting point. So this is to define requirements
> > > of a function based on what the function is doing. But does the
> > > function have to have strict requirements?
> >
> > IMO one of the main goals for these requirements is testability.
> > In order to have testable requirements we should state what the
> > valid input values are. In this case:
> > 0 -> disable, 1 -> enable, everything else -> Error.
> >
> > Now checking the code again it seems that the switch statement
> > is missing a default "ret = -EINVAL" (or else it should be changed
> > to boolean, but I guess it would have a wider impact on the rest
> > of the code...).
>
> Well, it's mostly used internally and the only places that call it uses
> 0 or 1, so there's never been any issue.
>
> >
> > >
> > > If it can handle "0" or "!0" does that mean that's how it will be
> > > defined? Or can it just state "0" or "1" and yes "2" is UB. That is,
> > > it's not part of the requirements but if someone passes in 2, it could
> > > act as a 1 as it's UB and implementation defined. Not a requirement.
> >
> > Right now if 2 is passed the function would silently return success,
> > but do we have a use case for this? I am trying to understand
> > where the implementation defined behavior would be....
>
> The issue is that all the callers pass in the proper value, and that
> can be easily verified, but by adding the "anything else ERROR", it
> would require adding more code that is not needed.
>
> I rather just switch that and soft_disable into a boolean than to add
> superficial error checks.

Many thanks for the explanation. Please consider that, from a requirement
point of view, it is also perfectly fine to document the assumption of use that
no other values than 0 or 1 shall be passed by the caller (so the bool rework
can be avoided eventually).

Gab

>
>
> -- Steve
>