Re: [PATCH v4] sched_ext: Introduce NUMA awareness to the default idle selection policy

[Andrea Righi]

On Mon, Oct 28, 2024 at 08:11:55AM -1000, Tejun Heo wrote:
> External email: Use caution opening links or attachments
> 
> 
> Hello,
> 
> On Mon, Oct 28, 2024 at 12:33:38PM +0100, Andrea Righi wrote:
> ...
> > +static void update_selcpu_topology(void)
> >  {
> > +     bool enable_llc = false, enable_numa = false;
> > +     struct sched_domain *sd;
> > +     const struct cpumask *cpus;
> > +     s32 cpu = cpumask_first(cpu_possible_mask);
> 
> On x86, the first CPU is guaranteed to be online but there are archs that
> allow the first CPU to go down in which case the topo information might not
> be available. Use cpumask_first(cpu_online_mask) instead?

Ok, I agree, cpu_online_mask is probably more reliable.

> 
> ...
> > @@ -3383,6 +3479,10 @@ static void handle_hotplug(struct rq *rq, bool online)
> >
> >       atomic_long_inc(&scx_hotplug_seq);
> >
> > +     if ((SCX_HAS_OP(cpu_online) || SCX_HAS_OP(cpu_offline)) &&
> > +         static_branch_likely(&scx_builtin_idle_enabled))
> > +             update_selcpu_topology();
> 
> Hmm... this feels a bit too complicated. Just gate it with scx_enabled()?

Ok, update_selcpu_topology() is not that expensive, so we can probably
just check scx_enabled() here.

> 
> >       if (online && SCX_HAS_OP(cpu_online))
> >               SCX_CALL_OP(SCX_KF_UNLOCKED, cpu_online, cpu);
> >       else if (!online && SCX_HAS_OP(cpu_offline))
> > @@ -5202,6 +5302,10 @@ static int scx_ops_enable(struct sched_ext_ops *ops, struct bpf_link *link)
> >                       static_branch_enable_cpuslocked(&scx_has_op[i]);
> >
> >       check_hotplug_seq(ops);
> > +#ifdef CONFIG_SMP
> > +     if (!ops->update_idle || (ops->flags & SCX_OPS_KEEP_BUILTIN_IDLE))
> > +             update_selcpu_topology();
> > +#endif
> 
> And always update here?

Ok.

Thanks,
-Andrea