On Wed, Jul 2, 2025 at 4:41 PM Steven Rostedt <[email protected]> wrote: > > On Wed, 2 Jul 2025 15:45:41 +0200 > Gabriele Paoloni <[email protected]> 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 <[email protected]> > > > > --- > > > > 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 >
