Re: [PATCH v2 03/11] sched: Add sched tracepoints for RV task model

From: Gabriele Monaco
Date: Tue Feb 18 2025 - 02:06:57 EST




On Mon, 2025-02-17 at 12:16 -0500, Steven Rostedt wrote:
> On Mon, 17 Feb 2025 17:49:17 +0100
> Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> > On Mon, Feb 17, 2025 at 11:38:44AM -0500, Steven Rostedt wrote:
> >
> > > > +void __do_trace_set_current_state(int state_value)
> > > > +{
> > > > + __do_trace_sched_set_state_tp(current, current-
> > > > >__state, state_value); 
> > >
> > > And this should not be using the internal macros of a trace
> > > point. It should be:
> > >
> > > trace_sched_set_state_tp(current, state_value);
> > >
> > > (I removed the current->__state as mentioned above). 
> >
> > But the static branch is already in the caller, no point
> > duplicating
> > that.
>
> Regardless, you should not be using internals of the tracepoints.
> That can
> change at any time and is not reliable (as the kernel test robot
> pointed out).
>
> It's a static branch, who cares if it's added twice? One is used to
> jump to
> the function, the other is for the tracepoint logic itself.
>
> There's several places that do this.
>
> Perhaps, in the future we could create a normal API that will always
> call
> the tracepoint, but until then, let's not use code that wasn't meant
> for
> that purpose.
>
> -- Steve
>

Mmh I get your point. We definitely don't want this piece of code to
break whenever something changes in the tracepoint API..

I will revert the change and prepare a V3

Thanks,
Gabriele