[PATCH] irq/affinity: Fix extra vecs calculation

[PATCH] irq/affinity: Fix extra vecs calculation

[Keith Busch]

This fixes a math error calculating the extra_vecs. The error assumed
only 1 cpu per vector, but the value needs to account for the actual
number of cpus per vector in order to get the correct remainder for
extra CPU assignment.

Fixes: 7bf8222b9bd0 ("irq/affinity: Fix CPU spread for unbalanced nodes")
Reported-by: Xiaolong Ye <xiaolong.ye@intel.com>
Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 kernel/irq/affinity.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c
index dc52911..d052947 100644
--- a/kernel/irq/affinity.c
+++ b/kernel/irq/affinity.c
@@ -108,7 +108,7 @@ irq_create_affinity_masks(int nvecs, const struct irq_affinity *affd)
 		vecs_to_assign = min(vecs_per_node, ncpus);
 
 		/* Account for rounding errors */
-		extra_vecs = ncpus - vecs_to_assign;
+		extra_vecs = ncpus - vecs_to_assign * (ncpus / vecs_to_assign);
 
 		for (v = 0; curvec < last_affv && v < vecs_to_assign;
 		     curvec++, v++) {
-- 
2.7.2

[tip:irq/urgent] irq/affinity: Fix extra vecs calculation

[tip-bot for Keith Busch]

Commit-ID:  3412386b531244f24a27c79ee003506a52a00848
Gitweb:     http://git.kernel.org/tip/3412386b531244f24a27c79ee003506a52a00848
Author:     Keith Busch <keith.busch@intel.com>
AuthorDate: Thu, 13 Apr 2017 13:28:12 -0400
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 13 Apr 2017 23:41:00 +0200

irq/affinity: Fix extra vecs calculation

This fixes a math error calculating the extra_vecs. The error assumed
only 1 cpu per vector, but the value needs to account for the actual
number of cpus per vector in order to get the correct remainder for
extra CPU assignment.

Fixes: 7bf8222b9bd0 ("irq/affinity: Fix CPU spread for unbalanced nodes")
Reported-by: Xiaolong Ye <xiaolong.ye@intel.com>
Signed-off-by: Keith Busch <keith.busch@intel.com>
Link: http://lkml.kernel.org/r/1492104492-19943-1-git-send-email-keith.busch@intel.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 kernel/irq/affinity.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c
index dc52911..d052947 100644
--- a/kernel/irq/affinity.c
+++ b/kernel/irq/affinity.c
@@ -108,7 +108,7 @@ irq_create_affinity_masks(int nvecs, const struct irq_affinity *affd)
 		vecs_to_assign = min(vecs_per_node, ncpus);
 
 		/* Account for rounding errors */
-		extra_vecs = ncpus - vecs_to_assign;
+		extra_vecs = ncpus - vecs_to_assign * (ncpus / vecs_to_assign);
 
 		for (v = 0; curvec < last_affv && v < vecs_to_assign;
 		     curvec++, v++) {

Re: irq/affinity: Fix extra vecs calculation

[Andrei Vagin]

Hi,

Something is wrong with this patch. We run CRIU tests for upstream kernels.
And we found that a kernel with this patch can't be booted.

https://travis-ci.org/avagin/linux/builds/223557750

We don't have access to console logs and I can't reproduce this issue on
my nodes. I tired to revert this patch and everything works as expected.

https://travis-ci.org/avagin/linux/builds/223594172

Here is another report about this patch
https://lkml.org/lkml/2017/4/16/344

Thanks,
Andrei

On Thu, Apr 13, 2017 at 01:28:12PM -0400, Keith Busch wrote:
> This fixes a math error calculating the extra_vecs. The error assumed
> only 1 cpu per vector, but the value needs to account for the actual
> number of cpus per vector in order to get the correct remainder for
> extra CPU assignment.
> 
> Fixes: 7bf8222b9bd0 ("irq/affinity: Fix CPU spread for unbalanced nodes")
> Reported-by: Xiaolong Ye <xiaolong.ye@intel.com>
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  kernel/irq/affinity.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c
> index dc52911..d052947 100644
> --- a/kernel/irq/affinity.c
> +++ b/kernel/irq/affinity.c
> @@ -108,7 +108,7 @@ irq_create_affinity_masks(int nvecs, const struct irq_affinity *affd)
>  		vecs_to_assign = min(vecs_per_node, ncpus);
>  
>  		/* Account for rounding errors */
> -		extra_vecs = ncpus - vecs_to_assign;
> +		extra_vecs = ncpus - vecs_to_assign * (ncpus / vecs_to_assign);
>  
>  		for (v = 0; curvec < last_affv && v < vecs_to_assign;
>  		     curvec++, v++) {

Re: irq/affinity: Fix extra vecs calculation

[Keith Busch]

On Wed, Apr 19, 2017 at 09:20:27AM -0700, Andrei Vagin wrote:
> Hi,
> 
> Something is wrong with this patch. We run CRIU tests for upstream kernels.
> And we found that a kernel with this patch can't be booted.
> 
> https://travis-ci.org/avagin/linux/builds/223557750
> 
> We don't have access to console logs and I can't reproduce this issue on
> my nodes. I tired to revert this patch and everything works as expected.
> 
> https://travis-ci.org/avagin/linux/builds/223594172
> 
> Here is another report about this patch
> https://lkml.org/lkml/2017/4/16/344

Yikes, okay, I've made a mistake somewhere. Sorry about that, I will
look into this ASAP.

If it's a divide by 0 as your last link indicates, that must mean there
are possible nodes, but have no CPUs, and those should be skipped. If
that's the case, the following should fix it, but I'm going to do some
more qemu testing with various CPU topologies to confirm.

---
diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c
index d052947..80c45d0 100644
--- a/kernel/irq/affinity.c
+++ b/kernel/irq/affinity.c
@@ -105,6 +105,9 @@ irq_create_affinity_masks(int nvecs, const struct irq_affinity *affd)
 
 		/* Calculate the number of cpus per vector */
 		ncpus = cpumask_weight(nmsk);
+		if (!ncpus)
+			continue;
+
 		vecs_to_assign = min(vecs_per_node, ncpus);
 
 		/* Account for rounding errors */
--

Re: irq/affinity: Fix extra vecs calculation

[Andrei Vagin]

On Wed, Apr 19, 2017 at 01:03:59PM -0400, Keith Busch wrote:
> On Wed, Apr 19, 2017 at 09:20:27AM -0700, Andrei Vagin wrote:
> > Hi,
> > 
> > Something is wrong with this patch. We run CRIU tests for upstream kernels.
> > And we found that a kernel with this patch can't be booted.
> > 
> > https://travis-ci.org/avagin/linux/builds/223557750
> > 
> > We don't have access to console logs and I can't reproduce this issue on
> > my nodes. I tired to revert this patch and everything works as expected.
> > 
> > https://travis-ci.org/avagin/linux/builds/223594172
> > 
> > Here is another report about this patch
> > https://lkml.org/lkml/2017/4/16/344
> 
> Yikes, okay, I've made a mistake somewhere. Sorry about that, I will
> look into this ASAP.

Thank you

> 
> If it's a divide by 0 as your last link indicates, that must mean there
> are possible nodes, but have no CPUs, and those should be skipped. If
> that's the case, the following should fix it, but I'm going to do some
> more qemu testing with various CPU topologies to confirm.

This patch doesn't fix my problem
https://travis-ci.org/avagin/linux/builds/223674690

> 
> ---
> diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c
> index d052947..80c45d0 100644
> --- a/kernel/irq/affinity.c
> +++ b/kernel/irq/affinity.c
> @@ -105,6 +105,9 @@ irq_create_affinity_masks(int nvecs, const struct irq_affinity *affd)
>  
>  		/* Calculate the number of cpus per vector */
>  		ncpus = cpumask_weight(nmsk);
> +		if (!ncpus)
> +			continue;
> +
>  		vecs_to_assign = min(vecs_per_node, ncpus);
>  
>  		/* Account for rounding errors */
> --

Re: irq/affinity: Fix extra vecs calculation

[Andrei Vagin]

On Wed, Apr 19, 2017 at 01:03:59PM -0400, Keith Busch wrote:
> On Wed, Apr 19, 2017 at 09:20:27AM -0700, Andrei Vagin wrote:
> > Hi,
> > 
> > Something is wrong with this patch. We run CRIU tests for upstream kernels.
> > And we found that a kernel with this patch can't be booted.
> > 
> > https://travis-ci.org/avagin/linux/builds/223557750
> > 
> > We don't have access to console logs and I can't reproduce this issue on
> > my nodes. I tired to revert this patch and everything works as expected.
> > 
> > https://travis-ci.org/avagin/linux/builds/223594172
> > 
> > Here is another report about this patch
> > https://lkml.org/lkml/2017/4/16/344
> 
> Yikes, okay, I've made a mistake somewhere. Sorry about that, I will
> look into this ASAP.
> 
> If it's a divide by 0 as your last link indicates, that must mean there
> are possible nodes, but have no CPUs, and those should be skipped. If
> that's the case, the following should fix it, but I'm going to do some
> more qemu testing with various CPU topologies to confirm.

I printed variables from my test host, I think this can help to
investigate the issue:

irq_create_affinity_masks:116: vecs_to_assign 0 ncpus 2 extra_vecs 2 vecs_per_node 0 affv 2 curvec 2 nodes 1

and here is a patch which I use to print them:

diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c
index a073a6e..c43c85d 100644
--- a/kernel/irq/affinity.c
+++ b/kernel/irq/affinity.c
@@ -110,7 +110,10 @@ irq_create_affinity_masks(int nvecs, const struct irq_affinity *affd)
                vecs_to_assign = min(vecs_per_node, ncpus);
 
                /* Account for rounding errors */
-               extra_vecs = ncpus - vecs_to_assign * (ncpus / vecs_to_assign);
+//             extra_vecs = ncpus - vecs_to_assign * (ncpus / vecs_to_assign);
+               extra_vecs = ncpus - vecs_to_assign;
+               printk("%s:%d: vecs_to_assign %d ncpus %d extra_vecs %d vecs_per_node %d affv %d curvec %d nodes %d\n",
+                       __func__, __LINE__, vecs_to_assign, ncpus, extra_vecs, vecs_per_node, affv, curvec, nodes);
 
                for (v = 0; curvec < last_affv && v < vecs_to_assign;
                     curvec++, v++) {


> 
> ---
> diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c
> index d052947..80c45d0 100644
> --- a/kernel/irq/affinity.c
> +++ b/kernel/irq/affinity.c
> @@ -105,6 +105,9 @@ irq_create_affinity_masks(int nvecs, const struct irq_affinity *affd)
>  
>  		/* Calculate the number of cpus per vector */
>  		ncpus = cpumask_weight(nmsk);
> +		if (!ncpus)
> +			continue;
> +
>  		vecs_to_assign = min(vecs_per_node, ncpus);
>  
>  		/* Account for rounding errors */
> --

Re: irq/affinity: Fix extra vecs calculation

[Keith Busch]

On Wed, Apr 19, 2017 at 12:53:44PM -0700, Andrei Vagin wrote:
> On Wed, Apr 19, 2017 at 01:03:59PM -0400, Keith Busch wrote:
> > If it's a divide by 0 as your last link indicates, that must mean there
> > are possible nodes, but have no CPUs, and those should be skipped. If
> > that's the case, the following should fix it, but I'm going to do some
> > more qemu testing with various CPU topologies to confirm.
> 
> I printed variables from my test host, I think this can help to
> investigate the issue:
> 
> irq_create_affinity_masks:116: vecs_to_assign 0 ncpus 2 extra_vecs 2 vecs_per_node 0 affv 2 curvec 2 nodes 1

That explains a lot. This setup wants 2 "pre_vectors", but I didn't
know that was even a thing. This should fix it:

---
diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c
index d052947..eb8b689 100644
--- a/kernel/irq/affinity.c
+++ b/kernel/irq/affinity.c
@@ -98,13 +98,16 @@ irq_create_affinity_masks(int nvecs, const struct irq_affinity *affd)
 		int ncpus, v, vecs_to_assign, vecs_per_node;
 
 		/* Spread the vectors per node */
-		vecs_per_node = (affv - curvec) / nodes;
+		vecs_per_node = (affv - (curvec - affd->pre_vectors)) / nodes;
 
 		/* Get the cpus on this node which are in the mask */
 		cpumask_and(nmsk, cpu_online_mask, cpumask_of_node(n));
--

Re: irq/affinity: Fix extra vecs calculation

[Andrei Vagin]

On Wed, Apr 19, 2017 at 05:53:09PM -0400, Keith Busch wrote:
> On Wed, Apr 19, 2017 at 12:53:44PM -0700, Andrei Vagin wrote:
> > On Wed, Apr 19, 2017 at 01:03:59PM -0400, Keith Busch wrote:
> > > If it's a divide by 0 as your last link indicates, that must mean there
> > > are possible nodes, but have no CPUs, and those should be skipped. If
> > > that's the case, the following should fix it, but I'm going to do some
> > > more qemu testing with various CPU topologies to confirm.
> > 
> > I printed variables from my test host, I think this can help to
> > investigate the issue:
> > 
> > irq_create_affinity_masks:116: vecs_to_assign 0 ncpus 2 extra_vecs 2 vecs_per_node 0 affv 2 curvec 2 nodes 1
> 
> That explains a lot. This setup wants 2 "pre_vectors", but I didn't
> know that was even a thing. This should fix it:

This patch works for me.
> 
> ---
> diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c
> index d052947..eb8b689 100644
> --- a/kernel/irq/affinity.c
> +++ b/kernel/irq/affinity.c
> @@ -98,13 +98,16 @@ irq_create_affinity_masks(int nvecs, const struct irq_affinity *affd)
>  		int ncpus, v, vecs_to_assign, vecs_per_node;
>  
>  		/* Spread the vectors per node */
> -		vecs_per_node = (affv - curvec) / nodes;
> +		vecs_per_node = (affv - (curvec - affd->pre_vectors)) / nodes;
>  
>  		/* Get the cpus on this node which are in the mask */
>  		cpumask_and(nmsk, cpu_online_mask, cpumask_of_node(n));
> --

Re: irq/affinity: Fix extra vecs calculation

[Keith Busch]

On Wed, Apr 19, 2017 at 03:32:06PM -0700, Andrei Vagin wrote:
> This patch works for me.

Awesome, thank you much for confirming, and again, sorry for the
breakage. I see virtio-scsi is one reliable way to have reproduced this,
so I'll incorporate that into tests before posting future kernel core
patches.

I'll send this fix with change log shortly.

> > ---
> > diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c
> > index d052947..eb8b689 100644
> > --- a/kernel/irq/affinity.c
> > +++ b/kernel/irq/affinity.c
> > @@ -98,13 +98,16 @@ irq_create_affinity_masks(int nvecs, const struct irq_affinity *affd)
> >  		int ncpus, v, vecs_to_assign, vecs_per_node;
> >  
> >  		/* Spread the vectors per node */
> > -		vecs_per_node = (affv - curvec) / nodes;
> > +		vecs_per_node = (affv - (curvec - affd->pre_vectors)) / nodes;
> >  
> >  		/* Get the cpus on this node which are in the mask */
> >  		cpumask_and(nmsk, cpu_online_mask, cpumask_of_node(n));
> > --