Re: [PATCH] sched_ext: Provides a sysfs 'events' to expose core event counters

[Changwoo Min]

Hello,

Thank you for the review!

On 25. 2. 11. 02:28, Tejun Heo wrote:
> Hello,
>
> On Mon, Feb 10, 2025 at 11:36:43PM +0900, Changwoo Min wrote:
> ...
> > +#define scx_attr_event_show(buf, at, events, kind) ({				\
> > +	sysfs_emit_at(buf, at, "%40s: %16llu\n", #kind, (events)->kind);	\
> > +})
>
> It's nice to format things in tabular forms but things under /sys lean more
> towards simpler formatting, so maybe just do "%s %16llu\n"?

Sure, I will change it to the simplest form, "%s %llu\n".

> >   static struct attribute *scx_global_attrs[] = {
> >   	&scx_attr_state.attr,
> >   	&scx_attr_switch_all.attr,
> >   	&scx_attr_nr_rejected.attr,
> >   	&scx_attr_hotplug_seq.attr,
> >   	&scx_attr_enable_seq.attr,
> > +	&scx_attr_events.attr,
>
> This probably should belong to the root/ subdir as we'd probably want to
> keep the event counter separate per scheduler instance in the
> multi-scheduler future.

I feel this is a bit contradictory to the need to access the core
event counters even after an scx scheduler is unloaded. In the
current implementation, root/ subdir appears and disappears when
an scx scheduler is loaded and unloaded.

We may change the scx_ktype to something similar to
scx_global_attr_group in order to keep root/ subdir. We then show
an empty file for root/ops when no scx scheduler is loaded while
keep the root/events file intact. I am not sure if this is what
we want.

What do you think?

Regards,
Changwoo Min