Re: [PATCH sched_ext/for-6.14] sched_ext: Move built-in idle CPU selection policy to a separate file

[Tejun Heo <tj@kernel.org>]

Hello,

On Thu, Jan 23, 2025 at 11:02:02PM +0100, Andrea Righi wrote:
...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 023df277737d..cd3d5b139a11 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20935,6 +20935,8 @@ T:	git://git.kernel.org/pub/scm/linux/kernel/git/tj/sched_ext.git
>  F:	include/linux/sched/ext.h
>  F:	kernel/sched/ext.h
>  F:	kernel/sched/ext.c
> +F:	kernel/sched/ext_idle.c
> +F:	kernel/sched/ext_idle.h

Maybe kernel/sched/ext*?

> diff --git a/kernel/sched/ext.c b/kernel/sched/ext.c
> index 564f250e7689..a24d48cebfb7 100644
> --- a/kernel/sched/ext.c
> +++ b/kernel/sched/ext.c
...
> @@ -896,6 +893,17 @@ static struct static_key_false scx_has_op[SCX_OPI_END] =
>  static atomic_t scx_exit_kind = ATOMIC_INIT(SCX_EXIT_DONE);
>  static struct scx_exit_info *scx_exit_info;
>  
> +#define scx_ops_error_kind(err, fmt, args...)					\
> +	scx_ops_exit_kind((err), 0, fmt, ##args)
> +
> +#define scx_ops_exit(code, fmt, args...)					\
> +	scx_ops_exit_kind(SCX_EXIT_UNREG_KERN, (code), fmt, ##args)
> +
> +#define scx_ops_error(fmt, args...)						\
> +	scx_ops_error_kind(SCX_EXIT_ERROR, fmt, ##args)
> +
> +#define SCX_HAS_OP(op)	static_branch_likely(&scx_has_op[SCX_OP_IDX(op)])
> +

This chunk is no longer necessary, right?

...
> @@ -7750,12 +7037,6 @@ BTF_ID_FLAGS(func, scx_bpf_nr_cpu_ids)
>  BTF_ID_FLAGS(func, scx_bpf_get_possible_cpumask, KF_ACQUIRE)
>  BTF_ID_FLAGS(func, scx_bpf_get_online_cpumask, KF_ACQUIRE)
>  BTF_ID_FLAGS(func, scx_bpf_put_cpumask, KF_RELEASE)
> -BTF_ID_FLAGS(func, scx_bpf_get_idle_cpumask, KF_ACQUIRE)
> -BTF_ID_FLAGS(func, scx_bpf_get_idle_smtmask, KF_ACQUIRE)
> -BTF_ID_FLAGS(func, scx_bpf_put_idle_cpumask, KF_RELEASE)
> -BTF_ID_FLAGS(func, scx_bpf_test_and_clear_cpu_idle)
> -BTF_ID_FLAGS(func, scx_bpf_pick_idle_cpu, KF_RCU)
> -BTF_ID_FLAGS(func, scx_bpf_pick_any_cpu, KF_RCU)

So, these were in ids_any and could be called from any BPF progs.

> diff --git a/kernel/sched/ext_idle.c b/kernel/sched/ext_idle.c
> new file mode 100644
> index 000000000000..ca99fc58af91
> --- /dev/null
> +++ b/kernel/sched/ext_idle.c
...
> +BTF_KFUNCS_START(scx_kfunc_ids_select_cpu)
> +BTF_ID_FLAGS(func, scx_bpf_select_cpu_dfl, KF_RCU)
> +BTF_ID_FLAGS(func, scx_bpf_get_idle_cpumask, KF_ACQUIRE)
> +BTF_ID_FLAGS(func, scx_bpf_get_idle_smtmask, KF_ACQUIRE)
> +BTF_ID_FLAGS(func, scx_bpf_put_idle_cpumask, KF_RELEASE)
> +BTF_ID_FLAGS(func, scx_bpf_test_and_clear_cpu_idle)
> +BTF_ID_FLAGS(func, scx_bpf_pick_idle_cpu, KF_RCU)
> +BTF_ID_FLAGS(func, scx_bpf_pick_any_cpu, KF_RCU)
> +BTF_KFUNCS_END(scx_kfunc_ids_select_cpu)

But they get moved into ids_select_cpu and can only be called from
struct_ops. Also, we currently don't use kfunc id sets directly to decide
which kfunc may be called from which ops but that may change in the future
too. Just create a separate ids and register directly from an init function?

...
> diff --git a/kernel/sched/ext_idle.h b/kernel/sched/ext_idle.h
> new file mode 100644
> index 000000000000..c1385af1ceeb
> --- /dev/null
> +++ b/kernel/sched/ext_idle.h
...
> +#ifdef CONFIG_SMP
> +static DEFINE_STATIC_KEY_FALSE(scx_selcpu_topo_llc);
> +static DEFINE_STATIC_KEY_FALSE(scx_selcpu_topo_numa);
> +
> +static void update_selcpu_topology(void);
> +static void reset_idle_masks(void);
> +static void init_idle_masks(void);
> +static bool test_and_clear_cpu_idle(int cpu);
> +
> +static s32 scx_pick_idle_cpu(const struct cpumask *cpus_allowed, u64 flags);
> +static s32 scx_select_cpu_dfl(struct task_struct *p,
> +			      s32 prev_cpu, u64 wake_flags, bool *found);

The way scheudler code is built is weird but let's keep it as close to usual
h/c splits - make them extern prototypes and prefix them with scx_. Also,
maybe name them to explicitly show that they deal with bulitin idle
handling?

Thanks.

-- 
tejun