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

[Andrea Righi <arighi@nvidia.com>]

On Fri, Jan 24, 2025 at 09:51:18AM -1000, Tejun Heo wrote:
> 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*?

Ok.

> 
> > 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?

Correct, no need to move this chunk.

> 
> ...
> > @@ -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?

Oh I was missing this aspect. Yeah, moving them to a separate block and
registering it separately is probably a cleaner approach and we can have
all the idle-related kfuncs grouped together (without breaking anything).
Will do this.

> 
> ...
> > 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?

Ok, I'll apply all these changes and send a new patch.

Thanks for looking at this!
-Andrea