From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] correct mis-conversion set_bit() -> __cpumask_set_cpu() by 4aaca0e9cd Date: Mon, 23 Feb 2015 12:01:20 +0000 Message-ID: <54EB1690.3040709@citrix.com> References: <54EB17A802000078000626F9@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7376680829131246583==" Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YPrhj-0000Bd-81 for xen-devel@lists.xenproject.org; Mon, 23 Feb 2015 12:01:39 +0000 In-Reply-To: <54EB17A802000078000626F9@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , xen-devel Cc: Ian Campbell , Sander Eikelenboom , Keir Fraser , Ian Jackson , Tim Deegan List-Id: xen-devel@lists.xenproject.org --===============7376680829131246583== Content-Type: multipart/alternative; boundary="------------040009010003020708040008" --------------040009010003020708040008 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: quoted-printable 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 > Signed-off-by: Jan Beulich Insofar as this clearly corrects the identified regression, Reviewed-by: Andrew Cooper 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)); > } > =20 > void cpu_raise_softirq_batch_begin(void) > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------040009010003020708040008 Content-Type: text/html; charset="windows-1252" Content-Length: 2396 Content-Transfer-Encoding: quoted-printable
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,=A0 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().=A0 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

--------------040009010003020708040008-- --===============7376680829131246583== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============7376680829131246583==--