[PATCH] correct mis-conversion set_bit() -> __cpumask_set_cpu() by 4aaca0e9cd

[PATCH] correct mis-conversion set_bit() -> __cpumask_set_cpu() by 4aaca0e9cd

[Jan Beulich]

[-- 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

Re: [PATCH] correct mis-conversion set_bit() -> __cpumask_set_cpu() by 4aaca0e9cd

[Sander Eikelenboom]

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

Re: [PATCH] correct mis-conversion set_bit() -> __cpumask_set_cpu() by 4aaca0e9cd

[Andrew Cooper]

[-- 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

Re: [PATCH] correct mis-conversion set_bit() -> __cpumask_set_cpu() by 4aaca0e9cd

[Jan Beulich]

>>> 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

Re: [PATCH] correct mis-conversion set_bit() -> __cpumask_set_cpu() by 4aaca0e9cd

[Sander Eikelenboom]

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

Re: [PATCH] correct mis-conversion set_bit() -> __cpumask_set_cpu() by 4aaca0e9cd

[Jan Beulich]

>>> 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

Re: [PATCH] correct mis-conversion set_bit() -> __cpumask_set_cpu() by 4aaca0e9cd

[Ian Campbell]

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>