Message-ID: <20250701195939.3e297e20@gandalf.local.home>
Date: Tue, 1 Jul 2025 19:59:39 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Gabriele Paoloni <gpaoloni@...hat.com>
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


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@...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.

> 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@...hat.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

Powered by blists - more mailing lists