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

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: <8ce4d887-464c-4d4d-825d-0d01dcafe400@igalia.com>
Date: Tue, 11 Feb 2025 09:57:08 +0900
From: Changwoo Min <changwoo@...lia.com>
To: Tejun Heo <tj@...nel.org>
Cc: void@...ifault.com, arighi@...dia.com, kernel-dev@...lia.com,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sched_ext: Provides a sysfs 'events' to expose core event
 counters

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