linux-kernel - Re: [PATCH 1/7] sched_ext: Implement event counter infrastructure and add an event

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: <89d98a0c-8d62-45c7-a213-b72052019ed8@igalia.com>
Date: Wed, 22 Jan 2025 10:45:46 +0900
From: Changwoo Min <changwoo@...lia.com>
To: Leonardo Temperanza <leonardo.temperanza@...il.com>, tj@...nel.org,
 void@...ifault.com, arighi@...dia.com
Cc: kernel-dev@...lia.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/7] sched_ext: Implement event counter infrastructure and
 add an event

Hello Leonardo,

Thanks for the review.

On 25. 1. 22. 00:00, Leonardo Temperanza wrote:
> scx_event_stat fields are required to be u64, otherwise the macros below 
> don't work correctly. Perhaps a comment could highlight this "hidden" 
> constraint? As well as a BUILD_BUG_ON to verify that this constraint is 
> verified at build time.

That's a good suggestion. I will add any hidden constratins in the comments.

> If an event is added to scx_event_kind but not scx_event_stat_str, the 
> array can possibly be accessed out of bounds (or into a NULL string 
> depending on the missing index). The GNU C extension could be used to 
> initialize all elements to the empty string "", like this:
> 
> static const char *scx_event_stat_str[SCX_EVENT_END] = {
>      [0 ... SCX_EVENT_END - 1] = "",
>      [SCX_EVENT_INVAL_SELECT_CPU] = "invalid_select_cpu",
> };
> 
> Alternatively a helper could be used:
> 
> static const char *scx_event_name(enum scx_event_kind kind)
> {
>      return scx_event_stat_str[kind] ? : "";
> }

Thanks for the suggestion. I am considering dropping event and event_str 
in the next version. That is because the only purpose of the event 
things is to print dump messages. At this point, the simpler the better, 
I think.


>> +/**
>> + * scx_read_event_kind - Read an event from 'e' with 'kind'
>> + * @e: a pointer to an event collected by scx_bpf_event_stat()
>> + * @kine: an event type defined in scx_event_kind
>> + */
>> +#define scx_read_event_kind(e, kind) ({                        \
>> +    u64 *__e64 = (u64 *)(e);                        \
>> +    __e64[kind];                                \
>> +})
>> +
> 
> 
> nit: typo, "@kine" instead of "@kind"

Good catch. Will fix it.

Regards,
Changwoo Min