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

[Changwoo Min]

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