Re: [PATCH 3/4] sched_ext: idle: Allow scx_bpf_select_cpu_and() from unlocked context

[David Vernet]

On Sun, May 18, 2025 at 07:52:30AM +0200, Andrea Righi wrote:
> > > 
> > > Trying to remember... probably it was removed because ops.select_cpu() is
> > > never called for tasks that can only run on 1 CPU.
> > 
> > Hmmm, I think it is called even for pcpu tasks, no?
> 
> IIUC ops.select_cpu() is triggered by select_task_rq(), but only if
> p->nr_cpus_allowed > 1 and it's not a migration-disabled task, see:
> 
> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/sched/core.c#n3582

Ahh, right you are, I'd forgotten about that. Thanks for the link.

> > > > Anyways, if we could do this, then we could bring both scx_bpf_select_cpu_and()
> > > > and scx_select_cpu_dfl() into the scx_kfunc_ids_idle kfunc group and remove
> > > > scx_kfunc_ids_select_cpu.
> > > > 
> > > > What do you think?
> > > 
> > > Are you suggesting that scx_bpf_select_cpu_dfl() should also be allowed in
> > > the same contexts as scx_bpf_select_cpu_and()?
> > 
> > Yep that's what I was thinking.
> > 
> > > I did consider that, but was initially concerned about the potential
> > > overhead of handling different contexts, even though these extra checks to
> > > manage the contexts would likely be negligible in terms of overhead. And it
> > > would give the possibility to use scx_bpf_select_cpu_dfl() in ops.enqueue()
> > > as well, so overall I see more pros than cons.
> > 
> > Now that you mention it I don't see any reason that scx_bpf_select_cpu_dfl()
> > couldn't be called from ops.enqueue() even now, as we do hold the rq lock on
> > that path. But in general I think that if we want to make
> > scx_bpf_select_cpu_and() callable without the rq lock held, that we might as
> > well do it for both as I don't think there's any semantic difference between
> > the two to the user; it's just that one version also does an AND.
> 
> Semantically speaking, yes, like you say, they're the same, except that one
> also accepts an additional AND cpumask.
> 
> The only reason to keep them separate might be the slight overhead of
> managing contexts, but, again, that should be negligible and not worth
> preserving a different and potentially confusing semantic.

Yep, I think we're aligned.

> > As I mentioned in the other thread, I don't have a strong opinion either way,
> > but in my opinion it seems more consistent to move the extra context-handling
> > logic into scx_bpf_select_cpu_dfl() if we do decide to allow callers to use
> > this.
> 
> I think I agree, I'll send another patch on top to unify the two kfuncs.

Thanks, Andrea.

- David

Attachment: signature.asc
Description: PGP signature