@@ -516,7 +516,8 @@ static int xive_pick_irq_target(struct irq_data *d,free_cpumask_var(mask);if(cpu>=0)returncpu;-fuzz--;+if(fuzz)+fuzz--;}/* No chip IDs, fallback to using the affinity mask */
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-08-02 22:49:45
On Wed, 2017-08-02 at 18:43 +0200, Cédric Le Goater wrote:
If xive_find_target_in_mask() fails to find a cpu, the fuzz value used
in xive_pick_irq_target() is decremented and reused in the last
returning call to xive_find_target_in_mask(). This can result in such
WARNINGs if the initial fuzz value is zero :
Ah indeed ... would have worked better if "fuzz" had been unsigned.
@@ -516,7 +516,8 @@ static int xive_pick_irq_target(struct irq_data *d,free_cpumask_var(mask);if(cpu>=0)returncpu;-fuzz--;+if(fuzz)+fuzz--;}/* No chip IDs, fallback to using the affinity mask */
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-08-03 00:01:11
Benjamin Herrenschmidt [off-list ref] writes:
On Wed, 2017-08-02 at 18:43 +0200, C=C3=A9dric Le Goater wrote:
quoted
If xive_find_target_in_mask() fails to find a cpu, the fuzz value used
in xive_pick_irq_target() is decremented and reused in the last
returning call to xive_find_target_in_mask(). This can result in such
WARNINGs if the initial fuzz value is zero :
Ah indeed ... would have worked better if "fuzz" had been unsigned.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-08-03 01:51:28
On Thu, 2017-08-03 at 10:01 +1000, Michael Ellerman wrote:
Benjamin Herrenschmidt [off-list ref] writes:
quoted
On Wed, 2017-08-02 at 18:43 +0200, Cédric Le Goater wrote:
quoted
If xive_find_target_in_mask() fails to find a cpu, the fuzz value used
in xive_pick_irq_target() is decremented and reused in the last
returning call to xive_find_target_in_mask(). This can result in such
WARNINGs if the initial fuzz value is zero :
Ah indeed ... would have worked better if "fuzz" had been unsigned.
Is that an ack or a changes requested?
Either ;-) The original code would have been fine with an unsigned, I
didn't realize it was signed and going negative. That said, cedric
patch is fine.
Cheers,
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-08-03 08:03:03
On Thu, 2017-08-03 at 09:45 +0200, Cédric Le Goater wrote:
On 08/02/2017 11:57 PM, Benjamin Herrenschmidt wrote:
quoted
On Wed, 2017-08-02 at 18:43 +0200, Cédric Le Goater wrote:
quoted
If xive_find_target_in_mask() fails to find a cpu, the fuzz value used
in xive_pick_irq_target() is decremented and reused in the last
returning call to xive_find_target_in_mask(). This can result in such
WARNINGs if the initial fuzz value is zero :
Ah indeed ... would have worked better if "fuzz" had been unsigned.
but 'fuzz' is unsigned !
Haha right.
With a -1, unsigned or not, the 'first' cpu becomes out of range for
the calculation below :
/* Pick up a starting point CPU in the mask based on fuzz */
num = cpumask_weight(mask);
first = fuzz % num;
How can it ? fuzz % num should then return something that's
0 <= first < num
Regardless of the value of fuzz.
Which means we should be able to locate it. The only case I could think
of that would fail would be if num is 0
/* Locate it */
cpu = cpumask_first(mask);
for (i = 0; i < first && cpu < nr_cpu_ids; i++)
cpu = cpumask_next(cpu, mask);
May be there is a better fix ?
Also, I am not sure of :
num = cpumask_weight(mask);
shouldn't we be using :
num = nr_cpu_ids;
In that case, 'first' would have been in the cpu range.
No that's the whole point. If we did that, then we would go out of the
mask.
The basic idea is that the mask contains "num" bits set, and we want to
pick one of them. But those bits can be bit 0, 5, 12 ... while
nr_cpu_ids can be 96 for example.
So for example, if the bits set as above, and fuzz is 5, we have
num is 3 (3 bits set in the mask)
first will then be 5 % 3 which is 2. That means that we want to pick
the "2th 0-based" ie the 3rd bit in the mask as our tentative target.
So the loop will iterate all the bits in the mask until i reaches
first, which is 2. So it will start with cpu = 0 i = 0, then cpu = 5 i
= 1, then cpu = 12 i = 2 and will exit then.
Ben.
From: Cédric Le Goater <clg@kaod.org> Date: 2017-08-03 08:22:10
On 08/02/2017 11:57 PM, Benjamin Herrenschmidt wrote:
On Wed, 2017-08-02 at 18:43 +0200, Cédric Le Goater wrote:
quoted
If xive_find_target_in_mask() fails to find a cpu, the fuzz value used
in xive_pick_irq_target() is decremented and reused in the last
returning call to xive_find_target_in_mask(). This can result in such
WARNINGs if the initial fuzz value is zero :
Ah indeed ... would have worked better if "fuzz" had been unsigned.
but 'fuzz' is unsigned !
With a -1, unsigned or not, the 'first' cpu becomes out of range for
the calculation below :
/* Pick up a starting point CPU in the mask based on fuzz */
num = cpumask_weight(mask);
first = fuzz % num;
/* Locate it */
cpu = cpumask_first(mask);
for (i = 0; i < first && cpu < nr_cpu_ids; i++)
cpu = cpumask_next(cpu, mask);
May be there is a better fix ?
Also, I am not sure of :
num = cpumask_weight(mask);
shouldn't we be using :
num = nr_cpu_ids;
In that case, 'first' would have been in the cpu range.
Cheers,
C.
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-08-03 09:52:12
Benjamin Herrenschmidt [off-list ref] writes:
On Thu, 2017-08-03 at 09:45 +0200, C=C3=A9dric Le Goater wrote:
quoted
On 08/02/2017 11:57 PM, Benjamin Herrenschmidt wrote:
quoted
On Wed, 2017-08-02 at 18:43 +0200, C=C3=A9dric Le Goater wrote:
quoted
If xive_find_target_in_mask() fails to find a cpu, the fuzz value us=
ed
quoted
quoted
quoted
in xive_pick_irq_target() is decremented and reused in the last
returning call to xive_find_target_in_mask(). This can result in such
WARNINGs if the initial fuzz value is zero :
=20
Ah indeed ... would have worked better if "fuzz" had been unsigned.
=20
but 'fuzz' is unsigned !=20
Haha right.
quoted
With a -1, unsigned or not, the 'first' cpu becomes out of range for
the calculation below :
=20
/* Pick up a starting point CPU in the mask based on fuzz */
num =3D cpumask_weight(mask);
first =3D fuzz % num;
How can it ? fuzz % num should then return something that's
0 <=3D first < num
Regardless of the value of fuzz.
What if num is 0?
Which it would be in the fallback case, if the affinity mask is empty,
AFAICS.
cheers
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-08-03 11:52:36
On Thu, 2017-08-03 at 19:52 +1000, Michael Ellerman wrote:
What if num is 0?
Which it would be in the fallback case, if the affinity mask is empty,
AFAICS.
How can the mask be empty though ? But yes, as I noted there is a
problem if the value is 0. Not sure what to do if we are given an empty
mask though.
Ben.
From: Cédric Le Goater <clg@kaod.org> Date: 2017-08-03 17:42:10
On 08/03/2017 01:52 PM, Benjamin Herrenschmidt wrote:
On Thu, 2017-08-03 at 19:52 +1000, Michael Ellerman wrote:
quoted
What if num is 0?
Which it would be in the fallback case, if the affinity mask is empty,
AFAICS.
How can the mask be empty though ? But yes, as I noted there is a
problem if the value is 0. Not sure what to do if we are given an empty
mask though.
I am seeing different problems.
The fuzz value was one but that was due to the fact that the spapr
backend did not set the chip_id of the xive_irq_data. I should have
fix that now. But in some other situations, the weight of the
'affinity' mask is 2048 (and 'nr_cpu_ids') and that also breaks
the 'first' calculation.
I am currently tracking this with the xive support for spapr. I will
send a fix with the patchset.
Thanks,
C.