* [PATCH] correct mis-conversion set_bit() -> __cpumask_set_cpu() by 4aaca0e9cd
@ 2015-02-23 11:06 Jan Beulich
2015-02-23 11:17 ` Sander Eikelenboom
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Jan Beulich @ 2015-02-23 11:06 UTC (permalink / raw)
To: xen-devel
Cc: Ian Campbell, Sander Eikelenboom, Keir Fraser, Ian Jackson, Tim Deegan
[-- Attachment #1: Type: text/plain, Size: 708 bytes --]
I have no idea how I came to use __cpumask_set_cpu() there, the
conversion should have been set_bit() -> __set_bit(). The wrong
construct results in problems on systems with relatively few CPUs.
Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/softirq.c
+++ b/xen/common/softirq.c
@@ -106,7 +106,7 @@ void cpu_raise_softirq(unsigned int cpu,
if ( !per_cpu(batching, this_cpu) || in_irq() )
smp_send_event_check_cpu(cpu);
else
- __cpumask_set_cpu(nr, &per_cpu(batch_mask, this_cpu));
+ __set_bit(nr, &per_cpu(batch_mask, this_cpu));
}
void cpu_raise_softirq_batch_begin(void)
[-- Attachment #2: cpumask-relax-fix.patch --]
[-- Type: text/plain, Size: 775 bytes --]
correct mis-conversion set_bit() -> __cpumask_set_cpu() by 4aaca0e9cd
I have no idea how I came to use __cpumask_set_cpu() there, the
conversion should have been set_bit() -> __set_bit(). The wrong
construct results in problems on systems with relatively few CPUs.
Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/softirq.c
+++ b/xen/common/softirq.c
@@ -106,7 +106,7 @@ void cpu_raise_softirq(unsigned int cpu,
if ( !per_cpu(batching, this_cpu) || in_irq() )
smp_send_event_check_cpu(cpu);
else
- __cpumask_set_cpu(nr, &per_cpu(batch_mask, this_cpu));
+ __set_bit(nr, &per_cpu(batch_mask, this_cpu));
}
void cpu_raise_softirq_batch_begin(void)
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] correct mis-conversion set_bit() -> __cpumask_set_cpu() by 4aaca0e9cd
2015-02-23 11:06 [PATCH] correct mis-conversion set_bit() -> __cpumask_set_cpu() by 4aaca0e9cd Jan Beulich
@ 2015-02-23 11:17 ` Sander Eikelenboom
2015-02-23 12:01 ` Andrew Cooper
2015-02-26 16:53 ` Sander Eikelenboom
2 siblings, 0 replies; 7+ messages in thread
From: Sander Eikelenboom @ 2015-02-23 11:17 UTC (permalink / raw)
To: Jan Beulich; +Cc: Ian Campbell, xen-devel, Keir Fraser, Ian Jackson, Tim Deegan
Monday, February 23, 2015, 12:06:00 PM, you wrote:
> I have no idea how I came to use __cpumask_set_cpu() there, the
> conversion should have been set_bit() -> __set_bit(). The wrong
> construct results in problems on systems with relatively few CPUs.
> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> --- a/xen/common/softirq.c
> +++ b/xen/common/softirq.c
> @@ -106,7 +106,7 @@ void cpu_raise_softirq(unsigned int cpu,
> if ( !per_cpu(batching, this_cpu) || in_irq() )
> smp_send_event_check_cpu(cpu);
> else
> - __cpumask_set_cpu(nr, &per_cpu(batch_mask, this_cpu));
> + __set_bit(nr, &per_cpu(batch_mask, this_cpu));
> }
>
> void cpu_raise_softirq_batch_begin(void)
Thanks !
--
Sander
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] correct mis-conversion set_bit() -> __cpumask_set_cpu() by 4aaca0e9cd
2015-02-23 11:06 [PATCH] correct mis-conversion set_bit() -> __cpumask_set_cpu() by 4aaca0e9cd Jan Beulich
2015-02-23 11:17 ` Sander Eikelenboom
@ 2015-02-23 12:01 ` Andrew Cooper
2015-02-23 13:47 ` Jan Beulich
2015-02-26 16:53 ` Sander Eikelenboom
2 siblings, 1 reply; 7+ messages in thread
From: Andrew Cooper @ 2015-02-23 12:01 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Ian Campbell, Sander Eikelenboom, Keir Fraser, Ian Jackson, Tim Deegan
[-- Attachment #1.1: Type: text/plain, Size: 1284 bytes --]
On 23/02/15 11:06, Jan Beulich wrote:
> I have no idea how I came to use __cpumask_set_cpu() there, the
> conversion should have been set_bit() -> __set_bit(). The wrong
> construct results in problems on systems with relatively few CPUs.
>
> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Insofar as this clearly corrects the identified regression,
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
However, I am still not convinced that the resulting code is actually
correct.
batch_mask is a cpumask_t and used properly as a cpumask in
cpumask_raise_softirq(). It is wrong to be putting softirq indices into
it here.
>
> --- a/xen/common/softirq.c
> +++ b/xen/common/softirq.c
> @@ -106,7 +106,7 @@ void cpu_raise_softirq(unsigned int cpu,
> if ( !per_cpu(batching, this_cpu) || in_irq() )
> smp_send_event_check_cpu(cpu);
> else
> - __cpumask_set_cpu(nr, &per_cpu(batch_mask, this_cpu));
> + __set_bit(nr, &per_cpu(batch_mask, this_cpu));
> }
>
> void cpu_raise_softirq_batch_begin(void)
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
[-- Attachment #1.2: Type: text/html, Size: 2341 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] correct mis-conversion set_bit() -> __cpumask_set_cpu() by 4aaca0e9cd
2015-02-23 12:01 ` Andrew Cooper
@ 2015-02-23 13:47 ` Jan Beulich
0 siblings, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2015-02-23 13:47 UTC (permalink / raw)
To: Andrew Cooper
Cc: Keir Fraser, Ian Jackson, Tim Deegan, Ian Campbell,
Sander Eikelenboom, xen-devel
>>> On 23.02.15 at 13:01, <andrew.cooper3@citrix.com> wrote:
> On 23/02/15 11:06, Jan Beulich wrote:
>> I have no idea how I came to use __cpumask_set_cpu() there, the
>> conversion should have been set_bit() -> __set_bit(). The wrong
>> construct results in problems on systems with relatively few CPUs.
>>
>> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Insofar as this clearly corrects the identified regression,
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> However, I am still not convinced that the resulting code is actually
> correct.
>
> batch_mask is a cpumask_t and used properly as a cpumask in
> cpumask_raise_softirq(). It is wrong to be putting softirq indices into
> it here.
Ah, now I see (somehow I didn't pay close enough attention to
what you said in the morning) - the code was wrong even before
that change. In that case - yes, let's not fix it the wrong way.
Jan
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] correct mis-conversion set_bit() -> __cpumask_set_cpu() by 4aaca0e9cd
2015-02-23 11:06 [PATCH] correct mis-conversion set_bit() -> __cpumask_set_cpu() by 4aaca0e9cd Jan Beulich
2015-02-23 11:17 ` Sander Eikelenboom
2015-02-23 12:01 ` Andrew Cooper
@ 2015-02-26 16:53 ` Sander Eikelenboom
2015-02-27 7:33 ` Jan Beulich
2 siblings, 1 reply; 7+ messages in thread
From: Sander Eikelenboom @ 2015-02-26 16:53 UTC (permalink / raw)
To: Jan Beulich; +Cc: Ian Campbell, xen-devel, Keir Fraser, Ian Jackson, Tim Deegan
Monday, February 23, 2015, 12:06:00 PM, you wrote:
> I have no idea how I came to use __cpumask_set_cpu() there, the
> conversion should have been set_bit() -> __set_bit(). The wrong
> construct results in problems on systems with relatively few CPUs.
> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> --- a/xen/common/softirq.c
> +++ b/xen/common/softirq.c
> @@ -106,7 +106,7 @@ void cpu_raise_softirq(unsigned int cpu,
> if ( !per_cpu(batching, this_cpu) || in_irq() )
> smp_send_event_check_cpu(cpu);
> else
> - __cpumask_set_cpu(nr, &per_cpu(batch_mask, this_cpu));
> + __set_bit(nr, &per_cpu(batch_mask, this_cpu));
> }
>
> void cpu_raise_softirq_batch_begin(void)
Hi Jan,
Any reason this wasn't applied to staging yet ?
--
Sander
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] correct mis-conversion set_bit() -> __cpumask_set_cpu() by 4aaca0e9cd
2015-02-26 16:53 ` Sander Eikelenboom
@ 2015-02-27 7:33 ` Jan Beulich
2015-02-27 10:34 ` Ian Campbell
0 siblings, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2015-02-27 7:33 UTC (permalink / raw)
To: Sander Eikelenboom
Cc: Ian Campbell, xen-devel, Keir Fraser, Ian Jackson, Tim Deegan
>>> On 26.02.15 at 17:53, <linux@eikelenboom.it> wrote:
> Monday, February 23, 2015, 12:06:00 PM, you wrote:
>
>> I have no idea how I came to use __cpumask_set_cpu() there, the
>> conversion should have been set_bit() -> __set_bit(). The wrong
>> construct results in problems on systems with relatively few CPUs.
>
>> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
>> --- a/xen/common/softirq.c
>> +++ b/xen/common/softirq.c
>> @@ -106,7 +106,7 @@ void cpu_raise_softirq(unsigned int cpu,
>> if ( !per_cpu(batching, this_cpu) || in_irq() )
>> smp_send_event_check_cpu(cpu);
>> else
>> - __cpumask_set_cpu(nr, &per_cpu(batch_mask, this_cpu));
>> + __set_bit(nr, &per_cpu(batch_mask, this_cpu));
>> }
>>
>> void cpu_raise_softirq_batch_begin(void)
>
> Hi Jan,
>
> Any reason this wasn't applied to staging yet ?
It didn't get ack-ed so far (and it was a little too early still to
ping it - I try to allow a week before doing so).
Jan
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] correct mis-conversion set_bit() -> __cpumask_set_cpu() by 4aaca0e9cd
2015-02-27 7:33 ` Jan Beulich
@ 2015-02-27 10:34 ` Ian Campbell
0 siblings, 0 replies; 7+ messages in thread
From: Ian Campbell @ 2015-02-27 10:34 UTC (permalink / raw)
To: Jan Beulich
Cc: Sander Eikelenboom, Tim Deegan, Keir Fraser, Ian Jackson, xen-devel
On Fri, 2015-02-27 at 07:33 +0000, Jan Beulich wrote:
> >>> On 26.02.15 at 17:53, <linux@eikelenboom.it> wrote:
>
> > Monday, February 23, 2015, 12:06:00 PM, you wrote:
> >
> >> I have no idea how I came to use __cpumask_set_cpu() there, the
> >> conversion should have been set_bit() -> __set_bit(). The wrong
> >> construct results in problems on systems with relatively few CPUs.
> >
> >> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >
> >> --- a/xen/common/softirq.c
> >> +++ b/xen/common/softirq.c
> >> @@ -106,7 +106,7 @@ void cpu_raise_softirq(unsigned int cpu,
> >> if ( !per_cpu(batching, this_cpu) || in_irq() )
> >> smp_send_event_check_cpu(cpu);
> >> else
> >> - __cpumask_set_cpu(nr, &per_cpu(batch_mask, this_cpu));
> >> + __set_bit(nr, &per_cpu(batch_mask, this_cpu));
> >> }
> >>
> >> void cpu_raise_softirq_batch_begin(void)
> >
> > Hi Jan,
> >
> > Any reason this wasn't applied to staging yet ?
>
> It didn't get ack-ed
Sorry, I thought this was an x86 patch for some reason and therefore
that Andrew's ack was sufficient.
For v2 of the patch (<54EB3D880200007800062834@mail.emea.novell.com>,
using __cpumask_set_cpu(cpu, ...):
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-02-27 10:35 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-23 11:06 [PATCH] correct mis-conversion set_bit() -> __cpumask_set_cpu() by 4aaca0e9cd Jan Beulich
2015-02-23 11:17 ` Sander Eikelenboom
2015-02-23 12:01 ` Andrew Cooper
2015-02-23 13:47 ` Jan Beulich
2015-02-26 16:53 ` Sander Eikelenboom
2015-02-27 7:33 ` Jan Beulich
2015-02-27 10:34 ` Ian Campbell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.