On Tue, Nov 29, 2022 at 10:22:56PM -1000, Tejun Heo wrote:
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index d06ada2341cb..cfbfc47692eb 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -131,6 +131,7 @@
> *(__dl_sched_class) \
> *(__rt_sched_class) \
> *(__fair_sched_class) \
> + *(__ext_sched_class) \
> *(__idle_sched_class) \
> __sched_class_lowest = .;
>
> @@ -9654,8 +9675,13 @@ void __init sched_init(void)
> int i;
>
> /* Make sure the linker didn't screw up */
> - BUG_ON(&idle_sched_class != &fair_sched_class + 1 ||
> - &fair_sched_class != &rt_sched_class + 1 ||
> +#ifdef CONFIG_SCHED_CLASS_EXT
> + BUG_ON(&idle_sched_class != &ext_sched_class + 1 ||
> + &ext_sched_class != &fair_sched_class + 1);
> +#else
> + BUG_ON(&idle_sched_class != &fair_sched_class + 1);
> +#endif
> + BUG_ON(&fair_sched_class != &rt_sched_class + 1 ||
> &rt_sched_class != &dl_sched_class + 1);
> #ifdef CONFIG_SMP
> BUG_ON(&dl_sched_class != &stop_sched_class + 1);
Perhaps the saner way to write this is:
#ifdef CONFIG_SMP
BUG_ON(!sched_class_above(&stop_sched_class, &dl_sched_class));
#endif
BUG_ON(!sched_class_above(&dl_sched_class, &rt_sched_class));
BUG_ON(!sched_class_above(&rt_sched_class, &fair_sched_class));
BUG_ON(!sched_class_above(&fair_sched_class, &idle_sched_class));
#ifdef CONFIG_...
BUG_ON(!sched_class_above(&fair_sched_class, &ext_sched_class));
BUG_ON(!sched_class_above(&ext_sched_class, &idle_sched_class));
#endif
> +static inline const struct sched_class *next_active_class(const struct sched_class *class)
> +{
> + class++;
> + if (!scx_enabled() && class == &ext_sched_class)
> + class++;
> + return class;
> +}
> +
> +#define for_active_class_range(class, _from, _to) \
> + for (class = (_from); class != (_to); class = next_active_class(class))
> +
> +#define for_each_active_class(class) \
> + for_active_class_range(class, __sched_class_highest, __sched_class_lowest)
> +
> +/*
> + * SCX requires a balance() call before every pick_next_task() call including
> + * when waking up from idle.
> + */
> +#define for_balance_class_range(class, prev_class, end_class) \
> + for_active_class_range(class, (prev_class) > &ext_sched_class ? \
> + &ext_sched_class : (prev_class), (end_class))
> +
This seems quite insane; why not simply make the ext methods effective
no-ops? Both balance and pick explicitly support that already, no?
> @@ -5800,10 +5812,13 @@ static void put_prev_task_balance(struct rq *rq, struct task_struct *prev,
> * We can terminate the balance pass as soon as we know there is
> * a runnable task of @class priority or higher.
> */
> - for_class_range(class, prev->sched_class, &idle_sched_class) {
> + for_balance_class_range(class, prev->sched_class, &idle_sched_class) {
> if (class->balance(rq, prev, rf))
> break;
> }
> +#else
> + /* SCX needs the balance call even in UP, call it explicitly */
This, *WHY* !?!
> + balance_scx_on_up(rq, prev, rf);
> #endif
>
> put_prev_task(rq, prev);
> @@ -5818,6 +5833,9 @@ __pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
> const struct sched_class *class;
> struct task_struct *p;
>
> + if (scx_enabled())
> + goto restart;
> +
> /*
> * Optimization: we know that if all tasks are in the fair class we can
> * call that function directly, but only if the @prev task wasn't of a
> @@ -5843,7 +5861,7 @@ __pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
> restart:
> put_prev_task_balance(rq, prev, rf);
>
> - for_each_class(class) {
> + for_each_active_class(class) {
> p = class->pick_next_task(rq);
> if (p)
> return p;
> @@ -5876,7 +5894,7 @@ static inline struct task_struct *pick_task(struct rq *rq)
> const struct sched_class *class;
> struct task_struct *p;
>
> - for_each_class(class) {
> + for_each_active_class(class) {
> p = class->pick_task(rq);
> if (p)
> return p;
But this.. afaict that means that:
- the whole EXT thing is incompatible with SCHED_CORE
- the whole EXT thing can be trivially starved by the presence of a
single CFS/BATCH/IDLE task.
Both seems like deal breakers.
