Message-ID: <Z1n6WgfTBBBIH2of@yury-ThinkPad>
Date: Wed, 11 Dec 2024 12:47:22 -0800
From: Yury Norov <yury.norov@...il.com>
To: Andrea Righi <arighi@...dia.com>
Cc: Tejun Heo <tj@...nel.org>, David Vernet <void@...ifault.com>,
	Changwoo Min <changwoo@...lia.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] sched_ext: Introduce NUMA aware idle cpu kfunc
 helpers

On Wed, Dec 11, 2024 at 09:20:56PM +0100, Andrea Righi wrote:
> On Wed, Dec 11, 2024 at 09:43:26AM -0800, Yury Norov wrote:
> > On Mon, Dec 09, 2024 at 11:40:58AM +0100, Andrea Righi wrote:
> > > Add the following kfunc's to provide scx schedulers direct access to
> > > per-node idle cpumasks information:
> > > 
> > >  const struct cpumask *scx_bpf_get_idle_cpumask_node(int node)
> > >  const struct cpumask *scx_bpf_get_idle_smtmask_node(int node)
> > >  s32 scx_bpf_pick_idle_cpu_node(int node,
> > >                                 const cpumask_t *cpus_allowed, u64 flags)
> > >  int scx_bpf_cpu_to_node(s32 cpu)
> > > 
> > > Signed-off-by: Andrea Righi <arighi@...dia.com>
> > > ---
> > >  kernel/sched/ext.c                       | 96 +++++++++++++++++++++++-
> > >  tools/sched_ext/include/scx/common.bpf.h |  4 +
> > >  tools/sched_ext/include/scx/compat.bpf.h | 19 +++++
> > >  3 files changed, 117 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/kernel/sched/ext.c b/kernel/sched/ext.c
> > > index d0d57323bcfc..ea7cc481782c 100644
> > > --- a/kernel/sched/ext.c
> > > +++ b/kernel/sched/ext.c
> > > @@ -433,6 +433,7 @@ struct sched_ext_ops {
> > >  	 * - scx_bpf_select_cpu_dfl()
> > >  	 * - scx_bpf_test_and_clear_cpu_idle()
> > >  	 * - scx_bpf_pick_idle_cpu()
> > > +	 * - scx_bpf_pick_idle_cpu_node()
> > >  	 *
> > >  	 * The user also must implement ops.select_cpu() as the default
> > >  	 * implementation relies on scx_bpf_select_cpu_dfl().
> > > @@ -955,6 +956,8 @@ static struct cpumask *get_idle_cpumask_node(int node)
> > >  	if (!static_branch_maybe(CONFIG_NUMA, &scx_builtin_idle_per_node))
> > >  		return idle_masks[0]->cpu;
> > >  
> > > +	if (node < 0 || node >= num_possible_nodes())
> > > +		return NULL;
> > 
> > 1. This sanity should go before the check above.
> > 2. In-kernel users don't need to do sanity checks. BPF users should,
> >    but for them you need to move it in BPF wrapper.
> > 3. -1 is a valid parameter, means NUMA_NO_NODE. 
> 
> Ok, but what would you return with NUMA_NO_NODE, in theory we should return
> a global system-wide cpumask, that doesn't exist with the per-node
> cpumasks. Maybe just return cpu_none_mask? That's what I've done in the
> next version, that seems safer than returning NULL.
 
To begin with, you can just disallow NUMA_NO_NODE for this interface.
Put a corresponding comment or warning, and you're done.

On the other hand, you can treat it as 'I don't care' hint and return
a cpumask for any node that has idle CPUs.

Returning cpu_none_mask?.. OK, it's possible, but what does that
bring? User will have to traverse empty mask just to find nothing.
I'd rather disallow NUMA_NO_NODE than returning something useless.

> > >  	return idle_masks[node]->cpu;
> > >  }
> > >  
> > > @@ -963,6 +966,8 @@ static struct cpumask *get_idle_smtmask_node(int node)
> > >  	if (!static_branch_maybe(CONFIG_NUMA, &scx_builtin_idle_per_node))
> > >  		return idle_masks[0]->smt;
> > >  
> > > +	if (node < 0 || node >= num_possible_nodes())
> > > +		return NULL;
> > >  	return idle_masks[node]->smt;
> > >  }
> > >  
> > > @@ -7469,6 +7474,16 @@ __bpf_kfunc u32 scx_bpf_nr_cpu_ids(void)
> > >  	return nr_cpu_ids;
> > >  }
> > >  
> > > +/**
> > > + * scx_bpf_cpu_to_node - Return the NUMA node the given @cpu belongs to
> > > + */
> > > +__bpf_kfunc int scx_bpf_cpu_to_node(s32 cpu)
> > > +{
> > > +	if (cpu < 0 || cpu >= nr_cpu_ids)
> > > +		return -EINVAL;
> > > +	return cpu_to_node(cpu);
> > > +}
> > 
> > I believe this wrapper should be declared somewhere in
> > kernel/sched/topology.c, and better be a separate patch.
> 
> Maybe kernel/bpf/helpers.c? And name it bpf_cpu_to_node()?

Sure, even better
 
> > > +
> > >  /**
> > >   * scx_bpf_get_possible_cpumask - Get a referenced kptr to cpu_possible_mask
> > >   */
> > > @@ -7499,11 +7514,32 @@ __bpf_kfunc void scx_bpf_put_cpumask(const struct cpumask *cpumask)
> > >  	 */
> > >  }
> > >  
> > > +/**
> > > + * scx_bpf_get_idle_cpumask_node - Get a referenced kptr to the idle-tracking
> > > + * per-CPU cpumask of a target NUMA node.
> > > + *
> > > + * Returns an empty cpumask if idle tracking is not enabled, if @node is not
> > > + * valid, or running on a UP kernel.
> > > + */
> > > +__bpf_kfunc const struct cpumask *scx_bpf_get_idle_cpumask_node(int node)
> > > +{
> > > +	if (!static_branch_likely(&scx_builtin_idle_enabled)) {
> > > +		scx_ops_error("built-in idle tracking is disabled");
> > > +		return cpu_none_mask;
> > > +	}
> > > +	if (!static_branch_likely(&scx_builtin_idle_per_node)) {
> > > +		scx_ops_error("per-node idle tracking is disabled");
> > > +		return cpu_none_mask;
> > > +	}
> > 
> > Nub question: is it possible that scx_builtin_idle_per_node is enable,
> > but scx_builtin_idle_enabled not? From my naive perspective, we can't
> > enable per-node idle masks without enabling general idle masks. Or I
> > mislead it?
> 
> In theory a BPF scheduler could set SCX_OPS_BUILTIN_IDLE_PER_NODE (without
> SCX_OPS_KEEP_BUILTIN_IDLE) in .flags while implementing ops.update_idle().
> 
> In this way we would have scx_builtin_idle_enabled==false and
> scx_builtin_idle_per_node==true, which doesn't make much sense, so we
> should probably handle this case in validate_ops() and trigger an error.
> 
> Good catch!
> 
> > 
> > > +
> > > +	return get_idle_cpumask_node(node) ? : cpu_none_mask;
> > > +}
> > >  /**
> > >   * scx_bpf_get_idle_cpumask - Get a referenced kptr to the idle-tracking
> > >   * per-CPU cpumask of the current NUMA node.
> > >   *
> > > - * Returns NULL if idle tracking is not enabled, or running on a UP kernel.
> > > + * Returns an emtpy cpumask if idle tracking is not enabled, or running on a UP
> > > + * kernel.
> > >   */
> > >  __bpf_kfunc const struct cpumask *scx_bpf_get_idle_cpumask(void)
> > >  {
> > > @@ -7515,12 +7551,35 @@ __bpf_kfunc const struct cpumask *scx_bpf_get_idle_cpumask(void)
> > >  	return get_curr_idle_cpumask();
> > >  }
> > >  
> > > +/**
> > > + * scx_bpf_get_idle_smtmask_node - Get a referenced kptr to the idle-tracking,
> > > + * per-physical-core cpumask of a target NUMA node. Can be used to determine
> > > + * if an entire physical core is free.
> > > + *
> > > + * Returns an empty cpumask if idle tracking is not enabled, if @node is not
> > > + * valid, or running on a UP kernel.
> > > + */
> > > +__bpf_kfunc const struct cpumask *scx_bpf_get_idle_smtmask_node(int node)
> > > +{
> > > +	if (!static_branch_likely(&scx_builtin_idle_enabled)) {
> > > +		scx_ops_error("built-in idle tracking is disabled");
> > > +		return cpu_none_mask;
> > > +	}
> > > +	if (!static_branch_likely(&scx_builtin_idle_per_node)) {
> > > +		scx_ops_error("per-node idle tracking is disabled");
> > > +		return cpu_none_mask;
> > > +	}
> > 
> > Can you add vertical spacing between blocks?
> 
> You mean a blank between the two blocks, right?

Yes

> Anyway, ...
> 
> > 
> > Also, because you use this construction more than once, I think it
> > makes sense to make it a helper.
> 
> With a proper error check in validate_ops() we can just get rid of the
> scx_builtin_idle_enabled block and simply check scx_builtin_idle_per_node.

But still, having a helper is better than opencoding the same 4-lines
pattern again and again

> > > +
> > > +	return get_idle_smtmask_node(node) ? : cpu_none_mask;
> > > +}
> > > +
> > >  /**
> > >   * scx_bpf_get_idle_smtmask - Get a referenced kptr to the idle-tracking,
> > >   * per-physical-core cpumask of the current NUMA node. Can be used to determine
> > >   * if an entire physical core is free.
> > >   *
> > > - * Returns NULL if idle tracking is not enabled, or running on a UP kernel.
> > > + * Returns an empty cumask if idle tracking is not enabled, or running on a UP
> > > + * kernel.
> > >   */
> > >  __bpf_kfunc const struct cpumask *scx_bpf_get_idle_smtmask(void)
> > >  {
> > > @@ -7569,6 +7628,35 @@ __bpf_kfunc bool scx_bpf_test_and_clear_cpu_idle(s32 cpu)
> > >  		return false;
> > >  }
> > >  
> > > +/**
> > > + * scx_bpf_pick_idle_cpu_node - Pick and claim an idle cpu from a NUMA node
> > > + * @node: target NUMA node
> > > + * @cpus_allowed: Allowed cpumask
> > > + * @flags: %SCX_PICK_IDLE_CPU_* flags
> > > + *
> > > + * Pick and claim an idle cpu in @cpus_allowed from the NUMA node @node.
> > > + * Returns the picked idle cpu number on success. -%EBUSY if no matching cpu
> > > + * was found.
> > > + *
> > > + * Unavailable if ops.update_idle() is implemented and
> > > + * %SCX_OPS_KEEP_BUILTIN_IDLE is not set or if %SCX_OPS_KEEP_BUILTIN_IDLE is
> > > + * not set.
> > > + */
> > > +__bpf_kfunc s32 scx_bpf_pick_idle_cpu_node(int node, const struct cpumask *cpus_allowed,
> > > +				      u64 flags)
> > > +{
> > 
> > Sanity checks here?
> 
> Indeed, thanks!
> 
> -Andrea

Powered by blists - more mailing lists