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

[Changwoo Min <changwoo@igalia.com>]

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