Message-ID: <CA+wEVJY2a_ERXemup7EefPPXHOv8DAfyauuP6Mn5vHYFkbbBcQ@mail.gmail.com>
Date: Wed, 2 Jul 2025 15:45:41 +0200
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: [RFC PATCH RESEND] tracing: add kernel documentation for
 trace_array_set_clr_event, trace_set_clr_event and supporting functions

Hi Steven, many thanks for looking into this

On Wed, Jul 2, 2025 at 1:59 AM Steven Rostedt <rostedt@...dmis.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@...hat.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@...hat.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
>

Powered by blists - more mailing lists