linux-kernel - Re: [PATCH] sched_ext: Add trace point to track sched

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <9ce1f9f6-f69c-4b14-be79-f46542d7316f@igalia.com>
Date: Thu, 27 Feb 2025 23:21:23 +0900
From: Changwoo Min <changwoo@...lia.com>
To: Andrea Righi <arighi@...dia.com>
Cc: tj@...nel.org, void@...ifault.com, kernel-dev@...lia.com,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sched_ext: Add trace point to track sched_ext core events



On 25. 2. 27. 19:55, Andrea Righi wrote:
> 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.

Sure.

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

I agree.

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

To me, sched_ext_event sounds better than others as it is simple.

Regards,
Changwoo Min