Message-ID: <CA+wEVJY+WbHECt3Vu+23B2PrpVoaRnaY=ncnXP3aHAjbgVNVMg@mail.gmail.com>
Date: Wed, 2 Jul 2025 18:38:51 +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

On Wed, Jul 2, 2025 at 4:41 PM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> 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.

Ok thanks, in the next revision I will also add info that
contextualises the possible
state transitions.

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

Yes, following your explanation below I see now.

>
> > 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 ;-)

I think I understand now:
SOFT_MODE means that one user requested a soft enable for the event,
however that does not guarantee that the event is not already enabled from
a previous standard enable (in which case the SOFT_DISABLED flag will
not be set as it would compromise the previous user).
So a soft disable request is needed to "undo" previous soft enables; this in
consideration of the SOFT_DISABLED flag (telling if there is an active user
with a standard enable).

Many thanks! This really helps to better contextualize function expectations
in the next revision of the patch.

Gab

>
> -- Steve
>

Powered by blists - more mailing lists