On Tue, Dec 24, 2024 at 09:37:35AM +0100, Andrea Righi wrote: > On Mon, Dec 23, 2024 at 07:53:21PM -0800, Yury Norov wrote: > > On Mon, Dec 23, 2024 at 06:48:48PM -0800, Yury Norov wrote: > > > On Fri, Dec 20, 2024 at 04:11:40PM +0100, Andrea Righi wrote: > > > > Introduce a flag to restrict the selection of an idle CPU to a specific > > > > NUMA node. > > > > > > > > Signed-off-by: Andrea Righi <arighi@xxxxxxxxxx> > > > > --- > > > > kernel/sched/ext.c | 1 + > > > > kernel/sched/ext_idle.c | 11 +++++++++-- > > > > 2 files changed, 10 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/kernel/sched/ext.c b/kernel/sched/ext.c > > > > index 143938e935f1..da5c15bd3c56 100644 > > > > --- a/kernel/sched/ext.c > > > > +++ b/kernel/sched/ext.c > > > > @@ -773,6 +773,7 @@ enum scx_deq_flags { > > > > > > > > enum scx_pick_idle_cpu_flags { > > > > SCX_PICK_IDLE_CORE = 1LLU << 0, /* pick a CPU whose SMT siblings are also idle */ > > > > + SCX_PICK_IDLE_NODE = 1LLU << 1, /* pick a CPU in the same target NUMA node */ > > > > > > SCX_FORCE_NODE or SCX_FIX_NODE? > > > > > > > }; > > > > > > > > enum scx_kick_flags { > > > > diff --git a/kernel/sched/ext_idle.c b/kernel/sched/ext_idle.c > > > > index 444f2a15f1d4..013deaa08f12 100644 > > > > --- a/kernel/sched/ext_idle.c > > > > +++ b/kernel/sched/ext_idle.c > > > > @@ -199,6 +199,12 @@ static s32 scx_pick_idle_cpu(const struct cpumask *cpus_allowed, int node, u64 f > > > > This function begins with: > > > > static s32 scx_pick_idle_cpu(const struct cpumask *cpus_allowed, int node, u64 flags) > > { > > nodemask_t hop_nodes = NODE_MASK_NONE; > > s32 cpu = -EBUSY; > > > > if (!static_branch_maybe(CONFIG_NUMA, &scx_builtin_idle_per_node)) > > return pick_idle_cpu_from_node(cpus_allowed, NUMA_FLAT_NODE, flags); > > > > ... > > > > So if I disable scx_builtin_idle_per_node and then call: > > > > scx_pick_idle_cpu(some_cpus, numa_node_id(), SCX_PICK_IDLE_NODE) > > > > I may get a CPU from any non-local node, right? I think we need to honor user's > > request: > > > > if (!static_branch_maybe(CONFIG_NUMA, &scx_builtin_idle_per_node)) > > return pick_idle_cpu_from_node(cpus_allowed, > > flags & SCX_PICK_IDLE_NODE ? node : NUMA_FLAT_NODE, flags); > > > > That way the code will be coherent: if you enable idle cpumasks, you > > will be able to follow all the NUMA hierarchy. If you disable them, at > > least you honor user's request to return a CPU from a given node, if > > he's very explicit about his intention. > > > > You can be even nicer: > > > > if (!static_branch_maybe(CONFIG_NUMA, &scx_builtin_idle_per_node)) { > > node = pick_idle_cpu_from_node(cpus, node, flags); > > if (node == MAX_NUM_NODES && flags & SCX_PICK_IDLE_NODE == 0) > > node = pick_idle_cpu_from_node(cpus, NUMA_FLAT_NODE, flags); > > > > return node; > > } > > > > Sorry, I'm not following, if scx_builtin_idle_per_node is disabled, we’re > only tracking idle CPUs in a single NUMA_FLAT_NODE (which is node 0). All > the other cpumasks are just empty, and we would always return -EBUSY if we > honor the user request. You're right. We can still do that like this: if (!static_branch_maybe(CONFIG_NUMA, &scx_builtin_idle_per_node)) { cpumask_and(tmp, cpus, cpumask_of_node(node)); node = pick_idle_cpu_from_node(tmp, NUMA_FLAT_NODE, flags); if (node == MAX_NUM_NODES && flags & SCX_PICK_IDLE_NODE == 0) node = pick_idle_cpu_from_node(cpus, NUMA_FLAT_NODE, flags); return node; } But I'm not sure we need this complication. Maybe later... > > Maybe we should just return an error if scx_builtin_idle_per_node is > disabled and the user is requesting an idle CPU in a specific node? The problem is that NUMA_FLAT_NODE is 0, and you can't distinguish it from node #0. You can drop NUMA_FLAT_NODE and ask users to always provide NUMA_NO_NODE if idle_per_node is disabled, or you can ignore the node entirely. You just need to describe it explicitly.