Message-ID: <CA+wEVJZqEiR7PzK6DSaSxmAihf7v4kfq1ht9OTFFDUVnXixJWw@mail.gmail.com>
Date: Thu, 22 Jan 2026 17:20:58 +0100
From: Gabriele Paoloni <gpaoloni@...hat.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: mhiramat@...nel.org, mathieu.desnoyers@...icios.com, 
	linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH v2] tracing: add kernel documentation for
 trace_array_set_clr_event, trace_set_clr_event and supporting functions

Hi Steve

On Wed, Dec 17, 2025 at 7:36 PM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> On Thu, 14 Aug 2025 14:21:41 +0200
> Gabriele Paoloni <gpaoloni@...hat.com> wrote:
>
> Hi Gabriele,
>
> As this patch is still sitting in patchwork, and I like the comments, I
> want to move it forward. I know we are still discussing posting
> requirements, but regardless of that, this has some good information that I
> want to add anyway. But the patch itself needs some clean up.

Many thanks for this and I am very sorry for the late reply. Basically
for some reason this email went into my spam folder and I realized
about your reply when looking for a different email thread on the
lore website.

>
> Note, this email is about documenting the functions and has nothing to do
> with the discussions about requirements and such.
>
> First, the subject requires some changes. One, the tracing subsystem uses
> capitalized subjects. Two, the subject can be shorter:
>
>   tracing: Add kerneldoc to set_clr_event() functions

Noted thanks, I'll keep this in mind for this and future patches

>
>
> > 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
>
> Change logs should be in imperative mode and never say "this patch".

Noted, I'll switch to imperative.

>
> > 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" and "Assumptions
> > of Use" 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.
> >
>
> Actually, the above can simply be replaced with:
>
> Add kernedoc to the following functions:
>
>   <list of functions>

Ok noted, I will change it in the next revision

>
>
> > Signed-off-by: Gabriele Paoloni <gpaoloni@...hat.com>
> > ---
> > Changes from v1:
> > - Added "Context:" information
> > - Added "Assumptions of Use"
> > - __ftrace_event_enable_disable Function's expectations have been
> >   rewritten to replace the soft mode flag with the soft mode reference
> >   counter
> > - Addressed other comments from https://lore.kernel.org/linux-trace-kernel/20250620085618.4489-1-gpaoloni@redhat.com/T/#u
> > ---
> >  kernel/trace/trace_events.c | 150 ++++++++++++++++++++++++++++++++----
> >  1 file changed, 133 insertions(+), 17 deletions(-)
> >
> > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> > index 9f3e9537417d..9bad5e9669df 100644
> > --- a/kernel/trace/trace_events.c
> > +++ b/kernel/trace/trace_events.c
> > @@ -763,6 +763,58 @@ 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.
> > + * @soft_disable: 1 or 0 respectively to mark if the enable parameter IS or
> > + * IS NOT a soft enable/disable.
>
> The parameter description should be short oneliners. Any more detail should
> be in the body of the kerneldoc. Also, please be consistent. @enable uses
> "0 or 1" and @soft_disable uses "1 or 0".

Ok I can change it to
"@soft_disable: 0 or 1 respectively if enable above IS NOT or IS soft"

>
>
> > + *
> > + * Function's expectations:
>
> Also, thinking about this particular function. It should describe what the
> intent is. Something to the effect of:
>
>     This function is used to enable or disable trace events for a given
>     instance. The @file maps the event to a specific instance. Normally,
>     this will be called with @soft_disabled = 0, where @enable set to 1 will
>     enabled the event to start recording, and if @enable is set to 0, will
>     disable the event. The @enable is a on/off switch, where calling this
>     function multiple times with @enable = 1 will enable the event the
>     first time, and the then calling this with @enable = 0 will disable the
>     event, regardless of the number of times it was called with @enable for
>     a given @file.
>
>     As there are triggers that can enable tracing in a context that cannot
>     modify kernel text, the triggers when enabled must "soft enable" the
>     events before it has to enable them in one of those prohibited
>     contexts. The code would call this function with @soft_disable set
>     before it needs to enable the event. If the event is not already
>     enabled, the @file flags SOFT_DISABLED bit is set and the event is
>     enabled but the callback will see the SOFT_DISABLED set and not do
>     anything.
>
>     When the trigger needs the event to be fully enabled, it only needs to
>     clear the SOFT_DISABLED flag, which will cause the event to start
>     recording.
>
>     Unlike the @enable flag, the @soft_disable has a counter, as multiple
>     triggers may need to use it. For every time this is called with
>     @enable=1 and @soft_disable=1, it requires a @enable=0 and
>     @soft_disable=1 to fully disable the event.
>
> The above can replace the below.

Yes. I think we can keep your description and format it as a bulleted list
to allow linking corresponding tests (e.g. KUnit). Basically it could be:

    This function is used to enable or disable trace events for a given
    instance. The @file maps the event to a specific instance.

    __ftrace_event_enable_disable behavior:
    1. Under normal operation (not in soft_mode), the function is called
    with @soft_disabled = 0, where @enable set to 1 will enable the
    event to start recording and @enable set to 0 will disable the event.

    2. The @enable is a on/off switch; when not in soft_mode, calling
    this function multiple times with @enable = 1 will enable the event
    the first time, then calling this with @enable = 0 will disable the
    event, regardless of the number of times it was called with @enable
    for a given @file.

    Note: as there are triggers that can enable tracing in a context that
    cannot modify kernel text, the triggers, when enabled, must "soft
    enable" the events before these can be enabled in one of those
    prohibited contexts.
    3. To do so this function must be called with @soft_disable = 1 and
    @enable = 1: If the event is not already enabled, the @file flags
    SOFT_DISABLED bit is set and the event is enabled but the callback
    will see the SOFT_DISABLED set and not do anything.

   4. When the trigger needs the event to be fully enabled, it only needs to
   clear the SOFT_DISABLED flag, which will cause the event to start
   recording. This is achieved by calling the function with @soft_disable = 0
   and @enable = 1.

   6. Unlike the @enable flag, the @soft_disable has a counter, as multiple
   triggers may need to use it. For every time the function is called with
   @enable=1 and @soft_disable=1, the counter is increased and it requires
   a corresponding times of invocations with @enable=0 and
   @soft_disable=1 to have the counter reach zero and hence to fully disable
   the event.

   7. If the trigger was previously fully enabled invoking the function with
   @soft_disable = 0 and @enable = 1, invoking the function with
   @soft_disable = 1 and @enable = 1 will not set the SOFT_DISABLED
   bit, hence the trigger will stay fully enabled but the @soft_disable counter
   will be increased.

   8. If the trigger was previously fully enabled (i.e. by an invocation with
   @soft_disable = 0 and @enable = 1) and the @soft_disable counter is
   not zero, an invocation with @soft_disable = 0 and @enable = 0 will
   set the the SOFT_DISABLED bit.

Note that I added point 7 and 8 to capture what happens when a soft_disable
enable command follows and normal mode enable command

>
> > + * - If soft_disable is 1 a soft mode 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 soft mode reference counter is > 0 before the increase or after
> > + *   the decrease, no other actions shall be taken.
> > + *
> > + * - if soft_disable is 1 and the soft mode reference counter is 0 before
> > + *   the increase or after the decrease, an enable value set to 0 or 1 shall
> > + *   result in disabling or enabling the use of trace_buffered_event
> > + *   respectively.
> > + *
> > + * - If soft_disable is 1 and enable is 0 and the soft mode 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 reference counter is 0.
> > + *   Additionally the soft disabled flag shall be set or cleared according to
> > + *   the soft mode reference counter being greater than 0 or 0 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 soft mode
> > + *   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.
> > + *
> > + * Assumptions of Use:
> > + * - for thread-safe execution, event_mutex shall be locked before calling
> > + *   this function;
> > + * - the file input pointer is assumed to be a valid one;
> > + * - the enable input parameter shall not be set to any value other than 0
> > + *   or 1.
> > + *
> > + * Context: process context.
>
> > + *
> > + * Return:
> > + * * 0 on success
> > + * * any error returned by the event register or unregister callbacks
>
> Note, the error document here should not reference other functions.
>
> Should just say "Negative on error" or something similar.
>
> > + */
>
>
> The rest can be updated similar.
>
> Are you OK with this, or would you prefer that I make these changes?

Yes I can take care of sending an updated patch. I guess that the rest of
kernel-doc headers below look good to you, so for consistency I will just
use a numbered bulleted list as done above....

Sorry again for the delay (I should look into the spam folder more frequently)

Thanks
Gab

>
> -- Steve
>
> >  static int __ftrace_event_enable_disable(struct trace_event_file *file,
> >                                        int enable, int soft_disable)
> >  {
> > @@ -1296,8 +1348,46 @@ static void remove_event_file_dir(struct trace_event_file *file)
> >       event_file_put(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.
> > + * @tr: target trace_array containing the events list.
> > + * @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.
> > + * @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.
> > + *
> > + * Assumptions of Use:
> > + * - for thread-safe execution, event_mutex shall be locked before calling
> > + *   this function;
> > + * - the tr input pointer is assumed to be a valid one;
> > + * - the set input parameter shall not be set to any value other than 0
> > + *   or 1.
> > + *
> > + * NOTE: __ftrace_set_clr_event_nolock(NULL, NULL, NULL, set, NULL) will
> > + * set/unset all events.
> > + *
> > + * Context: process context.
> > + *
> > + * Return:
> > + * * 0 on success
> > + * * %-EINVAL - the input parameters do not match any registered event
> > + * * %-ENOMEM -  memory allocation fails for the module pointer
> > + * * any value returned by the first call to ftrace_event_enable_disable that
> > + * returned an error
> >   */
> >  static int
> >  __ftrace_set_clr_event_nolock(struct trace_array *tr, const char *match,
> > @@ -1440,16 +1530,32 @@ 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.
> >   *
> >   * 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.
> > + *
> > + * Assumptions of Use:
> > + * - the set input parameter shall not be set to any value other than 0
> > + *   or 1.
> > + *
> > + * Context: process context, locks and unlocks event_mutex.
> > + *
> > + * Return:
> > + * * 0 on success
> > + * * %-ENODEV - the global tracer cannot be retrieved
> > + * * any other error condition returned by __ftrace_set_clr_event_nolock
> >   */
> >  int trace_set_clr_event(const char *system, const char *event, int set)
> >  {
> > @@ -1463,17 +1569,27 @@ 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.
> > + *
> > + * Context: process context, locks and unlocks event_mutex.
> >   *
> > - * Returns 0 on success, -EINVAL if the parameters do not match any
> > - * registered events.
> > + * Return:
> > + * * 0 on success
> > + * * %-ENOENT - the input tr is NULL
> > + * * 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)
>

Powered by blists - more mailing lists