Message-ID: <20250702104058.3cf9c1a3@batman.local.home>
Date: Wed, 2 Jul 2025 10:40:58 -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

On Wed, 2 Jul 2025 15:45:41 +0200
Gabriele Paoloni <gpaoloni@...hat.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@...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;

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

Powered by blists - more mailing lists