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: <y7bwaqjt43ejjrw4qscabr2ywd7wsang27fl27inkserg2czuv@ocovdxuhdi3k>
Date: Sun, 18 May 2025 08:49:31 -0500
From: David Vernet <void@...ifault.com>
To: Andrea Righi <arighi@...dia.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 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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)