Hi Steven, many thanks for looking into this On Wed, Jul 2, 2025 at 1:59 AM Steven Rostedt <rost...@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 <gpaol...@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 <gpaol...@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 >
