The two-stage spread is done on same irq vectors, and we just need that
either one stage covers all vector, not two stage work together to cover
all vectors.
So enhance the warning check to make sure all vectors are spread.
Cc: Christoph Hellwig <[email protected]>
Cc: Keith Busch <[email protected]>
Cc: [email protected],
Cc: Jon Derrick <[email protected]>
Cc: Jens Axboe <[email protected]>
Fixes: 6da4b3ab9a6 ("genirq/affinity: Add support for allocating interrupt sets")
Signed-off-by: Ming Lei <[email protected]>
---
kernel/irq/affinity.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c
index 6fef48033f96..265b3076f16b 100644
--- a/kernel/irq/affinity.c
+++ b/kernel/irq/affinity.c
@@ -215,8 +215,7 @@ static int irq_build_affinity_masks(unsigned int startvec, unsigned int numvecs,
npresmsk, nmsk, masks);
put_online_cpus();
- if (nr_present < numvecs)
- WARN_ON(nr_present + nr_others < numvecs);
+ WARN_ON(max(nr_present, nr_others) < numvecs);
free_node_to_cpumask(node_to_cpumask);
--
2.20.1
Hi Ming,
On Tue, 2019-08-13 at 16:14 +0800, Ming Lei wrote:
> The two-stage spread is done on same irq vectors, and we just need that
> either one stage covers all vector, not two stage work together to cover
> all vectors.
>
> So enhance the warning check to make sure all vectors are spread.
>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Keith Busch <[email protected]>
> Cc: [email protected],
> Cc: Jon Derrick <[email protected]>
> Cc: Jens Axboe <[email protected]>
> Fixes: 6da4b3ab9a6 ("genirq/affinity: Add support for allocating interrupt sets")
> Signed-off-by: Ming Lei <[email protected]>
> ---
> kernel/irq/affinity.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c
> index 6fef48033f96..265b3076f16b 100644
> --- a/kernel/irq/affinity.c
> +++ b/kernel/irq/affinity.c
> @@ -215,8 +215,7 @@ static int irq_build_affinity_masks(unsigned int startvec, unsigned int numvecs,
> npresmsk, nmsk, masks);
> put_online_cpus();
>
> - if (nr_present < numvecs)
> - WARN_ON(nr_present + nr_others < numvecs);
> + WARN_ON(max(nr_present, nr_others) < numvecs);
I think the patch description assumes the first condition
"The two-stage spread is done on same irq vectors"
/*
* Spread on non present CPUs starting from the next vector to be
* handled. If the spreading of present CPUs already exhausted the
* vector space, assign the non present CPUs to the already spread
* out vectors.
*/
if (nr_present >= numvecs)
curvec = firstvec;
But doesn't following condition imply nr_others spread is potentionally
different vector set?
else
curvec = firstvec + nr_present;
>
> free_node_to_cpumask(node_to_cpumask);
>
On Tue, Aug 13, 2019 at 07:31:39PM +0000, Derrick, Jonathan wrote:
> Hi Ming,
>
> On Tue, 2019-08-13 at 16:14 +0800, Ming Lei wrote:
> > The two-stage spread is done on same irq vectors, and we just need that
> > either one stage covers all vector, not two stage work together to cover
> > all vectors.
> >
> > So enhance the warning check to make sure all vectors are spread.
> >
> > Cc: Christoph Hellwig <[email protected]>
> > Cc: Keith Busch <[email protected]>
> > Cc: [email protected],
> > Cc: Jon Derrick <[email protected]>
> > Cc: Jens Axboe <[email protected]>
> > Fixes: 6da4b3ab9a6 ("genirq/affinity: Add support for allocating interrupt sets")
> > Signed-off-by: Ming Lei <[email protected]>
> > ---
> > kernel/irq/affinity.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c
> > index 6fef48033f96..265b3076f16b 100644
> > --- a/kernel/irq/affinity.c
> > +++ b/kernel/irq/affinity.c
> > @@ -215,8 +215,7 @@ static int irq_build_affinity_masks(unsigned int startvec, unsigned int numvecs,
> > npresmsk, nmsk, masks);
> > put_online_cpus();
> >
> > - if (nr_present < numvecs)
> > - WARN_ON(nr_present + nr_others < numvecs);
> > + WARN_ON(max(nr_present, nr_others) < numvecs);
>
> I think the patch description assumes the first condition
> "The two-stage spread is done on same irq vectors"
>
> /*
> * Spread on non present CPUs starting from the next vector to be
> * handled. If the spreading of present CPUs already exhausted the
> * vector space, assign the non present CPUs to the already spread
> * out vectors.
> */
> if (nr_present >= numvecs)
> curvec = firstvec;
>
> But doesn't following condition imply nr_others spread is potentionally
> different vector set?
>
> else
> curvec = firstvec + nr_present;
>
Most times, __irq_build_affinity_masks() returns numvecs.
However, each stage may expose less CPU number than num_vecs, then less
vectors than 'numvecs' can be spread.
So this patch is actually wrong.
Thank,
Ming