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

Linus Torvalds
2024-06-20T22:43:28Z

On Thu, 20 Jun 2024 at 15:15, Tejun Heo <tj@kernel.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