linux-kernel - Re: [PATCH V4 2/6] genirq/affinity: Pass affinity managed mask array to irq_build_affinity

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: <486ac5fc-9b9e-8591-0dd0-0336dde72c34@oracle.com>
Date:   Wed, 11 Jan 2023 09:46:55 +0000
From:   John Garry <john.g.garry@...cle.com>
To:     Ming Lei <ming.lei@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jens Axboe <axboe@...nel.dk>
Cc:     linux-kernel@...r.kernel.org, linux-block@...r.kernel.org,
        Christoph Hellwig <hch@....de>,
        John Garry <john.garry@...wei.com>
Subject: Re: [PATCH V4 2/6] genirq/affinity: Pass affinity managed mask array
 to irq_build_affinity_masks

On 27/12/2022 02:29, Ming Lei wrote:
> Pass affinity managed mask array to irq_build_affinity_masks() so that
> index of the first affinity managed vector is always zero, then we can
> simplify the implementation a bit.
> 
> Reviewed-by: Christoph Hellwig <hch@....de>
> Signed-off-by: Ming Lei <ming.lei@...hat.com>

Apart from a couple of nits, FWIW:

Reviewed-by: John Garry <john.g.garry@...cle.com>

> ---
>   kernel/irq/affinity.c | 28 ++++++++++++----------------
>   1 file changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c
> index 3361e36ebaa1..da6379cd27fd 100644
> --- a/kernel/irq/affinity.c
> +++ b/kernel/irq/affinity.c
> @@ -246,14 +246,13 @@ static void alloc_nodes_vectors(unsigned int numvecs,
>   
>   static int __irq_build_affinity_masks(unsigned int startvec,
>   				      unsigned int numvecs,
> -				      unsigned int firstvec,
>   				      cpumask_var_t *node_to_cpumask,
>   				      const struct cpumask *cpu_mask,
>   				      struct cpumask *nmsk,
>   				      struct irq_affinity_desc *masks)
>   {
>   	unsigned int i, n, nodes, cpus_per_vec, extra_vecs, done = 0;
> -	unsigned int last_affv = firstvec + numvecs;
> +	unsigned int last_affv = numvecs;
>   	unsigned int curvec = startvec;
>   	nodemask_t nodemsk = NODE_MASK_NONE;
>   	struct node_vectors *node_vectors;
> @@ -273,7 +272,7 @@ static int __irq_build_affinity_masks(unsigned int startvec,
>   			cpumask_and(nmsk, cpu_mask, node_to_cpumask[n]);
>   			cpumask_or(&masks[curvec].mask, &masks[curvec].mask, nmsk);
>   			if (++curvec == last_affv)
> -				curvec = firstvec;
> +				curvec = 0;
>   		}
>   		return numvecs;
>   	}
> @@ -321,7 +320,7 @@ static int __irq_build_affinity_masks(unsigned int startvec,
>   			 * may start anywhere
>   			 */
>   			if (curvec >= last_affv)
> -				curvec = firstvec;
> +				curvec = 0;
>   			irq_spread_init_one(&masks[curvec].mask, nmsk,
>   						cpus_per_vec);
>   		}
> @@ -336,11 +335,10 @@ static int __irq_build_affinity_masks(unsigned int startvec,
>    *	1) spread present CPU on these vectors
>    *	2) spread other possible CPUs on these vectors
>    */
> -static int irq_build_affinity_masks(unsigned int startvec, unsigned int numvecs,
> +static int irq_build_affinity_masks(unsigned int numvecs,
>   				    struct irq_affinity_desc *masks)
>   {
> -	unsigned int curvec = startvec, nr_present = 0, nr_others = 0;
> -	unsigned int firstvec = startvec;
> +	unsigned int curvec = 0

nit: curvec is used only as irq_build_affinity_masks() startvec param, 
so not sure why you use a different name in this function

> , nr_present = 0, nr_others = 0;
>   	cpumask_var_t *node_to_cpumask;
>   	cpumask_var_t nmsk, npresmsk;
>   	int ret = -ENOMEM;
> @@ -360,9 +358,8 @@ static int irq_build_affinity_masks(unsigned int startvec, unsigned int numvecs,
>   	build_node_to_cpumask(node_to_cpumask);
>   
>   	/* Spread on present CPUs starting from affd->pre_vectors */
> -	ret = __irq_build_affinity_masks(curvec, numvecs, firstvec,
> -					 node_to_cpumask, cpu_present_mask,
> -					 nmsk, masks);
> +	ret = __irq_build_affinity_masks(curvec, numvecs, node_to_cpumask,

nit: maybe you can /s/curvec/0/ to be more explicit in intention

> +					 cpu_present_mask, nmsk, masks);
>   	if (ret < 0)
>   		goto fail_build_affinity;
>   	nr_present = ret;
> @@ -374,13 +371,12 @@ static int irq_build_affinity_masks(unsigned int startvec, unsigned int numvecs,
>   	 * out vectors.
>   	 */
>   	if (nr_present >= numvecs)
> -		curvec = firstvec;
> +		curvec = 0;
>   	else
> -		curvec = firstvec + nr_present;
> +		curvec = nr_present;
>   	cpumask_andnot(npresmsk, cpu_possible_mask, cpu_present_mask);
> -	ret = __irq_build_affinity_masks(curvec, numvecs, firstvec,
> -					 node_to_cpumask, npresmsk, nmsk,
> -					 masks);
> +	ret = __irq_build_affinity_masks(curvec, numvecs, node_to_cpumask,
> +					 npresmsk, nmsk, masks);
>   	if (ret >= 0)
>   		nr_others = ret;
>   
> @@ -463,7 +459,7 @@ irq_create_affinity_masks(unsigned int nvecs, struct irq_affinity *affd)
>   		unsigned int this_vecs = affd->set_size[i];
>   		int ret;
>   
> -		ret = irq_build_affinity_masks(curvec, this_vecs, masks);
> +		ret = irq_build_affinity_masks(this_vecs, &masks[curvec]);
>   		if (ret) {
>   			kfree(masks);
>   			return NULL;