| 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
| ||
|
Date: Mon, 8 Aug 2022 17:39:04 +0300
From: Tariq Toukan <ttoukan.linux@...il.com>
To: Valentin Schneider <vschneid@...hat.com>,
Tariq Toukan <tariqt@...dia.com>,
"David S. Miller" <davem@...emloft.net>,
Saeed Mahameed <saeedm@...dia.com>,
Jakub Kicinski <kuba@...nel.org>,
Ingo Molnar <mingo@...hat.com>,
Peter Zijlstra <peterz@...radead.org>,
Juri Lelli <juri.lelli@...hat.com>
Cc: Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
Gal Pressman <gal@...dia.com>,
Vincent Guittot <vincent.guittot@...aro.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next V4 1/3] sched/topology: Add NUMA-based CPUs
spread API
On 8/4/2022 8:28 PM, Valentin Schneider wrote:
> On 28/07/22 22:12, Tariq Toukan wrote:
>> Implement and expose API that sets the spread of CPUs based on distance,
>> given a NUMA node. Fallback to legacy logic that uses
>> cpumask_local_spread.
>>
>> This logic can be used by device drivers to prefer some remote cpus over
>> others.
>>
>> Reviewed-by: Gal Pressman <gal@...dia.com>
>> Signed-off-by: Tariq Toukan <tariqt@...dia.com>
>
> IIUC you want a multi-CPU version of sched_numa_find_closest(). I'm OK with
> the need (and you have the numbers to back it up), but I have some qualms
> with the implementation, see more below.
>
I want a sorted multi-CPU version.
>> ---
>> include/linux/sched/topology.h | 5 ++++
>> kernel/sched/topology.c | 49 ++++++++++++++++++++++++++++++++++
>> 2 files changed, 54 insertions(+)
>>
>> diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
>> index 56cffe42abbc..a49167c2a0e5 100644
>> --- a/include/linux/sched/topology.h
>> +++ b/include/linux/sched/topology.h
>> @@ -210,6 +210,7 @@ extern void set_sched_topology(struct sched_domain_topology_level *tl);
>> # define SD_INIT_NAME(type)
>> #endif
>>
>> +void sched_cpus_set_spread(int node, u16 *cpus, int ncpus);
>> #else /* CONFIG_SMP */
>>
>> struct sched_domain_attr;
>> @@ -231,6 +232,10 @@ static inline bool cpus_share_cache(int this_cpu, int that_cpu)
>> return true;
>> }
>>
>> +static inline void sched_cpus_set_spread(int node, u16 *cpus, int ncpus)
>> +{
>> + memset(cpus, 0, ncpus * sizeof(*cpus));
>> +}
>> #endif /* !CONFIG_SMP */
>>
>> #if defined(CONFIG_ENERGY_MODEL) && defined(CONFIG_CPU_FREQ_GOV_SCHEDUTIL)
>> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
>> index 05b6c2ad90b9..157aef862c04 100644
>> --- a/kernel/sched/topology.c
>> +++ b/kernel/sched/topology.c
>> @@ -2067,8 +2067,57 @@ int sched_numa_find_closest(const struct cpumask *cpus, int cpu)
>> return found;
>> }
>>
>> +static bool sched_cpus_spread_by_distance(int node, u16 *cpus, int ncpus)
> ^^^^^^^^^
> That should be a struct *cpumask.
With cpumask, we'll lose the order.
>
>> +{
>> + cpumask_var_t cpumask;
>> + int first, i;
>> +
>> + if (!zalloc_cpumask_var(&cpumask, GFP_KERNEL))
>> + return false;
>> +
>> + cpumask_copy(cpumask, cpu_online_mask);
>> +
>> + first = cpumask_first(cpumask_of_node(node));
>> +
>> + for (i = 0; i < ncpus; i++) {
>> + int cpu;
>> +
>> + cpu = sched_numa_find_closest(cpumask, first);
>> + if (cpu >= nr_cpu_ids) {
>> + free_cpumask_var(cpumask);
>> + return false;
>> + }
>> + cpus[i] = cpu;
>> + __cpumask_clear_cpu(cpu, cpumask);
>> + }
>> +
>> + free_cpumask_var(cpumask);
>> + return true;
>> +}
>
> This will fail if ncpus > nr_cpu_ids, which shouldn't be a problem. It
> would make more sense to set *up to* ncpus, the calling code can then
> decide if getting fewer than requested is OK or not.
>
> I also don't get the fallback to cpumask_local_spread(), is that if the
> NUMA topology hasn't been initialized yet? It feels like most users of this
> would invoke it late enough (i.e. anything after early initcalls) to have
> the backing data available.
I don't expect this to fail, as we invoke it late enough. Fallback is
there just in case, to preserve the old behavior instead of getting
totally broken.
>
> Finally, I think iterating only once per NUMA level would make more sense.
Agree, although it's just a setup stage.
I'll check if it can work for me, based on the reference code below.
>
> I've scribbled something together from those thoughts, see below. This has
> just the mlx5 bits touched to show what I mean, but that's just compile
> tested.
My function returns a *sorted* list of the N closest cpus.
That is important. In many cases, drivers do not need all N irqs, but
only a portion of it, so it wants to use the closest subset of cpus.
IIUC, the code below relaxes this and returns the set of N closest cpus,
unsorted.
> ---
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eq.c b/drivers/net/ethernet/mellanox/mlx5/core/eq.c
> index 229728c80233..2d010d8d670c 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eq.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eq.c
> @@ -810,7 +810,7 @@ static int comp_irqs_request(struct mlx5_core_dev *dev)
> {
> struct mlx5_eq_table *table = dev->priv.eq_table;
> int ncomp_eqs = table->num_comp_eqs;
> - u16 *cpus;
> + cpumask_var_t cpus;
> int ret;
> int i;
>
> @@ -825,15 +825,14 @@ static int comp_irqs_request(struct mlx5_core_dev *dev)
> return ret;
> }
>
> - cpus = kcalloc(ncomp_eqs, sizeof(*cpus), GFP_KERNEL);
> - if (!cpus) {
> + if (!zalloc_cpumask_var(&cpus, GFP_KERNEL)) {
> ret = -ENOMEM;
> goto free_irqs;
> }
> - for (i = 0; i < ncomp_eqs; i++)
> - cpus[i] = cpumask_local_spread(i, dev->priv.numa_node);
> +
> + sched_numa_find_n_closest(cpus, dev->piv.numa_node, ncomp_eqs);
> ret = mlx5_irqs_request_vectors(dev, cpus, ncomp_eqs, table->comp_irqs);
> - kfree(cpus);
> + free_cpumask_var(cpus);
> if (ret < 0)
> goto free_irqs;
> return ret;
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c b/drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c
> index 662f1d55e30e..2330f81aeede 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c
> @@ -448,7 +448,7 @@ void mlx5_irqs_release_vectors(struct mlx5_irq **irqs, int nirqs)
> /**
> * mlx5_irqs_request_vectors - request one or more IRQs for mlx5 device.
> * @dev: mlx5 device that is requesting the IRQs.
> - * @cpus: CPUs array for binding the IRQs
> + * @cpus: cpumask for binding the IRQs
> * @nirqs: number of IRQs to request.
> * @irqs: an output array of IRQs pointers.
> *
> @@ -458,25 +458,22 @@ void mlx5_irqs_release_vectors(struct mlx5_irq **irqs, int nirqs)
> * This function returns the number of IRQs requested, (which might be smaller than
> * @nirqs), if successful, or a negative error code in case of an error.
> */
> -int mlx5_irqs_request_vectors(struct mlx5_core_dev *dev, u16 *cpus, int nirqs,
> +int mlx5_irqs_request_vectors(struct mlx5_core_dev *dev,
> + const struct cpumask *cpus,
> + int nirqs,
> struct mlx5_irq **irqs)
> {
> - cpumask_var_t req_mask;
> + int cpu = cpumask_first(cpus);
> struct mlx5_irq *irq;
> - int i;
>
> - if (!zalloc_cpumask_var(&req_mask, GFP_KERNEL))
> - return -ENOMEM;
> - for (i = 0; i < nirqs; i++) {
> - cpumask_set_cpu(cpus[i], req_mask);
> - irq = mlx5_irq_request(dev, i, req_mask);
> + for (i = 0; i < nirqs && cpu < nr_cpu_ids; i++) {
> + irq = mlx5_irq_request(dev, i, cpumask_of(cpu));
> if (IS_ERR(irq))
> break;
> - cpumask_clear(req_mask);
> irqs[i] = irq;
> + cpu = cpumask_next(cpu, cpus);
> }
>
> - free_cpumask_var(req_mask);
> return i ? i : PTR_ERR(irq);
> }
>
> diff --git a/include/linux/topology.h b/include/linux/topology.h
> index 4564faafd0e1..bdc9c5df84cd 100644
> --- a/include/linux/topology.h
> +++ b/include/linux/topology.h
> @@ -245,5 +245,13 @@ static inline const struct cpumask *cpu_cpu_mask(int cpu)
> return cpumask_of_node(cpu_to_node(cpu));
> }
>
> +#ifdef CONFIG_NUMA
> +extern int sched_numa_find_n_closest(struct cpumask *cpus, int node, int ncpus);
> +#else
> +static inline int sched_numa_find_n_closest(struct cpumask *cpus, int node, int ncpus)
> +{
> + return -ENOTSUPP;
> +}
> +#endif
>
> #endif /* _LINUX_TOPOLOGY_H */
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 8739c2a5a54e..499f6ef611fa 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -2067,6 +2067,56 @@ int sched_numa_find_closest(const struct cpumask *cpus, int cpu)
> return found;
> }
>
> +/**
> + * sched_numa_find_n_closest - Find the 'n' closest cpus to a given node
> + * @cpus: The cpumask to fill in with CPUs
> + * @ncpus: How many CPUs to look for
> + * @node: The node to start the search from
> + *
> + * This will fill *up to* @ncpus in @cpus, using the closest (in NUMA distance)
> + * first and expanding outside the node if more CPUs are required.
> + *
> + * Return: Number of found CPUs, negative value on error.
> + */
> +int sched_numa_find_n_closest(struct cpumask *cpus, int node, int ncpus)
> +{
> + struct cpumask ***masks;
> + int cpu, lvl, ntofind = ncpus;
> +
> + if (node >= nr_node_ids)
> + return -EINVAL;
> +
> + rcu_read_lock();
> +
> + masks = rcu_dereference(sched_domains_numa_masks);
> + if (!masks)
> + goto unlock;
> +
> + /*
> + * Walk up the level masks; the first mask should be CPUs LOCAL_DISTANCE
> + * away (aka the local node), and we incrementally grow the search
> + * beyond that.
> + */
> + for (lvl = 0; lvl < sched_domains_numa_levels; lvl++) {
> + if (!masks[lvl][node])
> + goto unlock;
> +
> + /* XXX: could be neater with for_each_cpu_andnot() */
> + for_each_cpu(cpu, masks[lvl][node]) {
> + if (cpumask_test_cpu(cpu, cpus))
> + continue;
> +
> + __cpumask_set_cpu(cpu, cpus);
> + if (--ntofind == 0)
> + goto unlock;
> + }
> + }
> +unlock:
> + rcu_read_unlock();
> + return ncpus - ntofind;
> +}
> +EXPORT_SYMBOL_GPL(sched_numa_find_n_closest);
> +
> #endif /* CONFIG_NUMA */
>
> static int __sdt_alloc(const struct cpumask *cpu_map)
>
Powered by blists - more mailing lists