Re: [PATCH] sched_ext: Add trace point to track sched_ext core events

[Andrea Righi <arighi@nvidia.com>]

On Thu, Feb 27, 2025 at 07:23:23PM +0900, Changwoo Min wrote:
> On 25. 2. 27. 17:19, Andrea Righi wrote:
> > On Thu, Feb 27, 2025 at 05:05:54PM +0900, Changwoo Min wrote:
> > Otherwise there's the risk to break potential users of this tracepoint that
...
> > Maybe we can call it @id or @event_id or similar and guarantee its
> > portability? What do you think?
> 
> Now I think dropping @offset would be better in the long run
> because we can maintain scx_event_stats clean and do not create
> a source of confusion. Regarding the ease of using @name, adding
> an code example in the commit message will suffice, something
> like this:
> 
> struct tp_add_event {
> 	struct trace_entry ent;
> 	u32 __data_loc_name;
> 	u64 delta;
> };
> 
> SEC("tracepoint/sched_ext/sched_ext_add_event")
> int tp_add_event(struct tp_add_event *ctx)
> {
> 	char event_name[128];
> 	unsigned short offset = ctx->__data_loc_name & 0xFFFF;
>         bpf_probe_read_str((void *)event_name, 128, (char *)ctx + offset);
> 
> 	bpf_printk("name %s   delta %llu", event_name, ctx->delta);
> 	return 0;
> }

We can definitely add a BPF code example, but keep in mind that tracepoints
can be used also outside of BPF, like:

 $ sudo perf trace -e sched_ext:sched_ext_add_event

In this case I think just having the name is totally fine.

> 
> The downside of not having a numerical ID (@offset or @event_id)
> is the cost of string comparison to distinguish an event type. If
> we assume the probing the event is rare, it will be okay.
> 
> @Tejun, @Andrea -- What do you think? Should we provide
> a portability-guaranteed @event_id after dropping @offset? Or
> would it be more than sufficient to have a string-type event name?

I think a tracepoint should be used mostly for tracing purposes, not in
critical hot paths. So, under this assumption, the overhead of the string
comparison is probably acceptable and it allows us to not worry too much
about breaking compatibility.

Also, perf trace allows to use filters based on strings, so in our case we
can do something like this for example:

 $ sudo perf trace -e sched_ext:sched_ext_add_event --filter 'name == "SCX_EV_ENQ_SLICE_DFL"'

While at it, what do you think about renaming this tracepoint
sched_ext_event or maybe sched_ext_core_event?

Thanks,
-Andrea