linux-kernel - Re: [PATCH sched_ext/for-6.11] sched, sched_ext: Replace scx_next_task_picked() with sched_class->switch

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] [thread-next>] [day] [month] [year] [list]

Date: Thu, 20 Jun 2024 15:42:48 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Tejun Heo <tj@...nel.org>
Cc: Thomas Gleixner <tglx@...utronix.de>, mingo@...hat.com, peterz@...radead.org, 
	juri.lelli@...hat.com, vincent.guittot@...aro.org, dietmar.eggemann@....com, 
	rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de, bristot@...hat.com, 
	vschneid@...hat.com, ast@...nel.org, daniel@...earbox.net, andrii@...nel.org, 
	martin.lau@...nel.org, joshdon@...gle.com, brho@...gle.com, pjt@...gle.com, 
	derkling@...gle.com, haoluo@...gle.com, dvernet@...a.com, 
	dschatzberg@...a.com, dskarlat@...cmu.edu, riel@...riel.com, 
	changwoo@...lia.com, himadrics@...ia.fr, memxor@...il.com, 
	andrea.righi@...onical.com, joel@...lfernandes.org, 
	linux-kernel@...r.kernel.org, bpf@...r.kernel.org, kernel-team@...a.com
Subject: Re: [PATCH sched_ext/for-6.11] sched, sched_ext: Replace
 scx_next_task_picked() with sched_class->switch_class()

On Thu, 20 Jun 2024 at 15:15, Tejun Heo <tj@...nel.org> wrote:
>
> The changes are straightforward and the code looks better afterwards.
> However, when !CONFIG_SCHED_CLASS_EXT, this just ends up adding an unused
> hook which is unlikely to be useful to other sched_classes. We can #ifdef
> the op with CONFIG_SCHED_CLASS_EXT but then I'm not sure the code
> necessarily looks better afterwards.

So honestly, if people _really_ care about performance here, then I
think that in the long run the right thing to do is

 - expose all the DEFINE_SCHED_CLASS() definitions in a header file

 - rename for_each_class() to FOR_EACH_CLASS() and make it unroll the
whole damn loop statically

which would turn the indirect branches into actual direct branches,
and would statically just remove any "if (!class->zyz)" conditionals.

Pretty? No. But it probably wouldn't be hugely ugly either, and
honestly, looking at the existing for_each_class() uses (and the one
single "for_class_range()" one), they are so small and the number of
classes is so small that unrolling the loop entirely doesn't sound
bad.

It wouldn't help deal with *this* case (since it's a "call variable
class"), but considering that the current __pick_next_task()

 (a) special-cases one class as-is

 (b) does a "for_each_class()" and calls an indirect call for each
when that doesn't trigger

I would claim that people don't care enough about this that one test
for a NULL 'switch_class' function would be worth worrying about.

Btw, indirect calls are now expensive enough that when you have only a
handful of choices, instead of a variable

        class->some_callback(some_arguments);

you might literally be better off with a macro that does

       #define call_sched_fn(class, name, arg...) switch (class) { \
        case &fair_name_class: fair_name_class.name(arg); break; \
        ... unroll them all here..

which then just generates a (very small) tree of if-statements.

Again, this is entirely too ugly to do unless people *really* care.
But for situations where you have a small handful of cases known at
compile-time, it's not out of the question, and it probably does
generate better code.

NOTE NOTE NOTE! This is a comp[letely independent aside, and has
nothing to do with sched_ext except for the very obvious indirect fact
that sched_ext would be one of the classes in this kind of code.

And yes, I suspect it is too ugly to actually do this.

            Linus