linux-kernel - Re: [PATCH 3/4] sched_ext: idle: Allow scx_bpf_select_cpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aCl1nq0hUJ7IdtC5@gpd3>
Date: Sun, 18 May 2025 07:52:30 +0200
From: Andrea Righi <arighi@...dia.com>
To: David Vernet <void@...ifault.com>
Cc: Tejun Heo <tj@...nel.org>, Changwoo Min <changwoo@...lia.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] sched_ext: idle: Allow scx_bpf_select_cpu_and() from
 unlocked context

On Sat, May 17, 2025 at 07:40:22PM -0500, David Vernet wrote:
> On Sun, May 18, 2025 at 12:50:29AM +0200, Andrea Righi wrote:
> 
> Hey Andrea,
> 
> [...]
> 
> > > I wonder if we should just bring this into scx_select_cpu_dfl()? It seems like
> > > it would makes sense to do this optimization whether we're looking at
> > > cpus_allowed here, or p->cpus_ptr in scx_select_cpu_dfl(). I seem to recall us
> > > having this in there before so there may be a reason we removed it, but I've
> > > been out of the game for a while so I'm not sure.
> > 
> > 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

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

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