Message ID | 1475184293-18298-1-git-send-email-nikhil.rao@intel.com (mailing list archive) |
---|---|
State | Accepted, archived |
Delegated to: | Thomas Monjalon |
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 7A4F756A1; Thu, 29 Sep 2016 14:54:31 +0200 (CEST) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 958A3568A for <dev@dpdk.org>; Thu, 29 Sep 2016 14:54:29 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga105.jf.intel.com with ESMTP; 29 Sep 2016 05:54:28 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos; i="5.30,415,1470726000"; d="scan'208"; a="1038136311" Received: from unknown (HELO localhost.iind.intel.com) ([10.224.122.216]) by orsmga001.jf.intel.com with ESMTP; 29 Sep 2016 05:54:27 -0700 From: Nikhil Rao <nikhil.rao@intel.com> To: dev@dpdk.org Cc: Nikhil Rao <nikhil.rao@intel.com> Date: Fri, 30 Sep 2016 02:54:53 +0530 Message-Id: <1475184293-18298-1-git-send-email-nikhil.rao@intel.com> X-Mailer: git-send-email 2.7.4 Subject: [dpdk-dev] [PATCH] eal: fix bug in x86 cmpset X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Rao, Nikhil
Sept. 29, 2016, 9:24 p.m. UTC
The original code used movl instead of xchgl, this caused rte_atomic64_cmpset to use ebx as the lower dword of the source to cmpxchg8b instead of the lower dword of function argument "src". Reported-by: Job Abraham <job.abraham@intel.com> Tested-by: Job Abraham <job.abraham@intel.com> Signed-off-by: Nikhil Rao <nikhil.rao@intel.com> --- lib/librte_eal/common/include/arch/x86/rte_atomic_32.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Comments
The patch misses a fixed: line which it should get I think. But in general If applied -> stable for this one? On Thu, Sep 29, 2016 at 11:24 PM, Nikhil Rao <nikhil.rao@intel.com> wrote: > The original code used movl instead of xchgl, this caused > rte_atomic64_cmpset to use ebx as the lower dword of the source > to cmpxchg8b instead of the lower dword of function argument "src". > > Reported-by: Job Abraham <job.abraham@intel.com> > Tested-by: Job Abraham <job.abraham@intel.com> > Signed-off-by: Nikhil Rao <nikhil.rao@intel.com> > --- > lib/librte_eal/common/include/arch/x86/rte_atomic_32.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/librte_eal/common/include/arch/x86/rte_atomic_32.h > b/lib/librte_eal/common/include/arch/x86/rte_atomic_32.h > index 2e04c75..fb3abf1 100644 > --- a/lib/librte_eal/common/include/arch/x86/rte_atomic_32.h > +++ b/lib/librte_eal/common/include/arch/x86/rte_atomic_32.h > @@ -81,7 +81,7 @@ rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t > exp, uint64_t src) > : "memory" ); /* no-clobber list */ > #else > asm volatile ( > - "mov %%ebx, %%edi\n" > + "xchgl %%ebx, %%edi;\n" > MPLOCKED > "cmpxchg8b (%[dst]);" > "setz %[res];" > -- > 2.7.4 > >
On 9/29/2016 6:35 PM, Christian Ehrhardt wrote: > The patch misses a fixed: line which it should get I think. The bug has existed from the day the DPDK was open-sourced, i.e, there wasn't a specific commit that introduced this feature/bug, hence wasn't sure if it needed the fixes tag. > > But in general If applied -> stable for this one? > Yes. > > On Thu, Sep 29, 2016 at 11:24 PM, Nikhil Rao <nikhil.rao@intel.com <mailto:nikhil.rao@intel.com>> wrote: > > The original code used movl instead of xchgl, this caused > rte_atomic64_cmpset to use ebx as the lower dword of the source > to cmpxchg8b instead of the lower dword of function argument "src". > > Reported-by: Job Abraham <job.abraham@intel.com <mailto:job.abraham@intel.com>> > Tested-by: Job Abraham <job.abraham@intel.com <mailto:job.abraham@intel.com>> > Signed-off-by: Nikhil Rao <nikhil.rao@intel.com <mailto:nikhil.rao@intel.com>> > --- > lib/librte_eal/common/include/arch/x86/rte_atomic_32.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/librte_eal/common/include/arch/x86/rte_atomic_32.h b/lib/librte_eal/common/include/arch/x86/rte_atomic_32.h > index 2e04c75..fb3abf1 100644 > --- a/lib/librte_eal/common/include/arch/x86/rte_atomic_32.h > +++ b/lib/librte_eal/common/include/arch/x86/rte_atomic_32.h > @@ -81,7 +81,7 @@ rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src) > : "memory" ); /* no-clobber list */ > #else > asm volatile ( > - "mov %%ebx, %%edi\n" > + "xchgl %%ebx, %%edi;\n" > MPLOCKED > "cmpxchg8b (%[dst]);" > "setz %[res];" > -- > 2.7.4 > > > > > -- > Christian Ehrhardt > Software Engineer, Ubuntu Server > Canonical Ltd
2016-09-29 18:46, Rao, Nikhil: > > On 9/29/2016 6:35 PM, Christian Ehrhardt wrote: > > The patch misses a fixed: line which it should get I think. > > The bug has existed from the day the DPDK was open-sourced, i.e, there wasn't a specific > commit that introduced this feature/bug, hence wasn't sure if it needed the fixes tag. In this case, we use the first commit: af75078 first public release It means it can be backported everywhere.
2016-09-30 02:54, Nikhil Rao: > The original code used movl instead of xchgl, this caused > rte_atomic64_cmpset to use ebx as the lower dword of the source > to cmpxchg8b instead of the lower dword of function argument "src". Could you please start the explanation with a statement of what is wrong from an user point of view? It could help to understand how severe it is.
2016-09-29 18:34, Thomas Monjalon: > 2016-09-30 02:54, Nikhil Rao: > > The original code used movl instead of xchgl, this caused > > rte_atomic64_cmpset to use ebx as the lower dword of the source > > to cmpxchg8b instead of the lower dword of function argument "src". > > Could you please start the explanation with a statement of > what is wrong from an user point of view? > It could help to understand how severe it is. Please, we need a clear explanation of the bug, and an acknowledgement.
2016-11-06 22:09, Thomas Monjalon: > 2016-09-29 18:34, Thomas Monjalon: > > 2016-09-30 02:54, Nikhil Rao: > > > The original code used movl instead of xchgl, this caused > > > rte_atomic64_cmpset to use ebx as the lower dword of the source > > > to cmpxchg8b instead of the lower dword of function argument "src". > > > > Could you please start the explanation with a statement of > > what is wrong from an user point of view? > > It could help to understand how severe it is. > > Please, we need a clear explanation of the bug, and an acknowledgement. Should we close this bug?
On 9/2/2017 4:53 PM, Thomas Monjalon wrote: > 2016-11-06 22:09, Thomas Monjalon: >> 2016-09-29 18:34, Thomas Monjalon: >>> 2016-09-30 02:54, Nikhil Rao: >>>> The original code used movl instead of xchgl, this caused >>>> rte_atomic64_cmpset to use ebx as the lower dword of the source >>>> to cmpxchg8b instead of the lower dword of function argument "src". >>> Could you please start the explanation with a statement of >>> what is wrong from an user point of view? >>> It could help to understand how severe it is. >> Please, we need a clear explanation of the bug, and an acknowledgement. > Should we close this bug? I took a few minutes to look at this, and the issue can easily be reproduced with a small snippet of code. With the 'mov', the lower dword of the result is incorrect. This is resolved by using 'xchgl'. void main() { uint64_t a = 0xff000000ff; rte_atomic64_cmpset( &a, 0xff000000ff, 0xfa000000fa); printf("0x%lx\n", a); } When using 'mov', the result is 0xfa00000000 When using 'xchgl', the result is 0xfa000000fa, as expected. Rgds, Dave.
2017-02-10 10:39, Hunt, David: > > On 9/2/2017 4:53 PM, Thomas Monjalon wrote: > > 2016-11-06 22:09, Thomas Monjalon: > >> 2016-09-29 18:34, Thomas Monjalon: > >>> 2016-09-30 02:54, Nikhil Rao: > >>>> The original code used movl instead of xchgl, this caused > >>>> rte_atomic64_cmpset to use ebx as the lower dword of the source > >>>> to cmpxchg8b instead of the lower dword of function argument "src". > >>> Could you please start the explanation with a statement of > >>> what is wrong from an user point of view? > >>> It could help to understand how severe it is. > >> Please, we need a clear explanation of the bug, and an acknowledgement. > > Should we close this bug? > > I took a few minutes to look at this, and the issue can easily be > reproduced with a small snippet of code. > With the 'mov', the lower dword of the result is incorrect. This is > resolved by using 'xchgl'. > > void main() > { > uint64_t a = 0xff000000ff; > > rte_atomic64_cmpset( &a, 0xff000000ff, 0xfa000000fa); > printf("0x%lx\n", a); > } > > When using 'mov', the result is 0xfa00000000 > When using 'xchgl', the result is 0xfa000000fa, as expected. This operation is used a lot in drivers for link status. I think we need to clearly explain what was the consequence of this bug.
On 10/2/2017 10:53 AM, Thomas Monjalon wrote: > 2017-02-10 10:39, Hunt, David: >> On 9/2/2017 4:53 PM, Thomas Monjalon wrote: >>> 2016-11-06 22:09, Thomas Monjalon: >>>> 2016-09-29 18:34, Thomas Monjalon: >>>>> 2016-09-30 02:54, Nikhil Rao: >>>>>> The original code used movl instead of xchgl, this caused >>>>>> rte_atomic64_cmpset to use ebx as the lower dword of the source >>>>>> to cmpxchg8b instead of the lower dword of function argument "src". >>>>> Could you please start the explanation with a statement of >>>>> what is wrong from an user point of view? >>>>> It could help to understand how severe it is. >>>> Please, we need a clear explanation of the bug, and an acknowledgement. >>> Should we close this bug? >> I took a few minutes to look at this, and the issue can easily be >> reproduced with a small snippet of code. >> With the 'mov', the lower dword of the result is incorrect. This is >> resolved by using 'xchgl'. >> >> void main() >> { >> uint64_t a = 0xff000000ff; >> >> rte_atomic64_cmpset( &a, 0xff000000ff, 0xfa000000fa); >> printf("0x%lx\n", a); >> } >> >> When using 'mov', the result is 0xfa00000000 >> When using 'xchgl', the result is 0xfa000000fa, as expected. > This operation is used a lot in drivers for link status. > > I think we need to clearly explain what was the consequence of this bug. Agreed. It's probably also worth noting that its only on the __PIC__ enabled codepath so would have more of an affect on the distros.
On Fri, 10 Feb 2017 11:53:06 +0100 Thomas Monjalon <thomas.monjalon@6wind.com> wrote: > 2017-02-10 10:39, Hunt, David: > > > > On 9/2/2017 4:53 PM, Thomas Monjalon wrote: > > > 2016-11-06 22:09, Thomas Monjalon: > > >> 2016-09-29 18:34, Thomas Monjalon: > > >>> 2016-09-30 02:54, Nikhil Rao: > > >>>> The original code used movl instead of xchgl, this caused > > >>>> rte_atomic64_cmpset to use ebx as the lower dword of the source > > >>>> to cmpxchg8b instead of the lower dword of function argument "src". > > >>> Could you please start the explanation with a statement of > > >>> what is wrong from an user point of view? > > >>> It could help to understand how severe it is. > > >> Please, we need a clear explanation of the bug, and an acknowledgement. > > > Should we close this bug? > > > > I took a few minutes to look at this, and the issue can easily be > > reproduced with a small snippet of code. > > With the 'mov', the lower dword of the result is incorrect. This is > > resolved by using 'xchgl'. > > > > void main() > > { > > uint64_t a = 0xff000000ff; > > > > rte_atomic64_cmpset( &a, 0xff000000ff, 0xfa000000fa); > > printf("0x%lx\n", a); > > } > > > > When using 'mov', the result is 0xfa00000000 > > When using 'xchgl', the result is 0xfa000000fa, as expected. > > This operation is used a lot in drivers for link status. > > I think we need to clearly explain what was the consequence of this bug. A bigger issue is why there are a huge number of copies of the same link code in drivers. Definitely should be common code. Also why is cmpset used here when a simple atomic_set would work as well for what was intended.
2017-02-10 08:46, Stephen Hemminger: > On Fri, 10 Feb 2017 11:53:06 +0100 > Thomas Monjalon <thomas.monjalon@6wind.com> wrote: > > > 2017-02-10 10:39, Hunt, David: > > > > > > On 9/2/2017 4:53 PM, Thomas Monjalon wrote: > > > > 2016-11-06 22:09, Thomas Monjalon: > > > >> 2016-09-29 18:34, Thomas Monjalon: > > > >>> 2016-09-30 02:54, Nikhil Rao: > > > >>>> The original code used movl instead of xchgl, this caused > > > >>>> rte_atomic64_cmpset to use ebx as the lower dword of the source > > > >>>> to cmpxchg8b instead of the lower dword of function argument "src". > > > >>> Could you please start the explanation with a statement of > > > >>> what is wrong from an user point of view? > > > >>> It could help to understand how severe it is. > > > >> Please, we need a clear explanation of the bug, and an acknowledgement. > > > > Should we close this bug? > > > > > > I took a few minutes to look at this, and the issue can easily be > > > reproduced with a small snippet of code. > > > With the 'mov', the lower dword of the result is incorrect. This is > > > resolved by using 'xchgl'. > > > > > > void main() > > > { > > > uint64_t a = 0xff000000ff; > > > > > > rte_atomic64_cmpset( &a, 0xff000000ff, 0xfa000000fa); > > > printf("0x%lx\n", a); > > > } > > > > > > When using 'mov', the result is 0xfa00000000 > > > When using 'xchgl', the result is 0xfa000000fa, as expected. > > > > This operation is used a lot in drivers for link status. > > > > I think we need to clearly explain what was the consequence of this bug. > > > A bigger issue is why there are a huge number of copies of the same link code > in drivers. Definitely should be common code. Also why is cmpset used here > when a simple atomic_set would work as well for what was intended. I'm surprised that there is no progress on this issue.
On Fri, Feb 10, 2017 at 11:53:06AM +0100, Thomas Monjalon wrote: > 2017-02-10 10:39, Hunt, David: > > > > On 9/2/2017 4:53 PM, Thomas Monjalon wrote: > > > 2016-11-06 22:09, Thomas Monjalon: > > >> 2016-09-29 18:34, Thomas Monjalon: > > >>> 2016-09-30 02:54, Nikhil Rao: > > >>>> The original code used movl instead of xchgl, this caused > > >>>> rte_atomic64_cmpset to use ebx as the lower dword of the source > > >>>> to cmpxchg8b instead of the lower dword of function argument "src". > > >>> Could you please start the explanation with a statement of > > >>> what is wrong from an user point of view? > > >>> It could help to understand how severe it is. > > >> Please, we need a clear explanation of the bug, and an acknowledgement. > > > Should we close this bug? > > > > I took a few minutes to look at this, and the issue can easily be > > reproduced with a small snippet of code. > > With the 'mov', the lower dword of the result is incorrect. This is > > resolved by using 'xchgl'. > > > > void main() > > { > > uint64_t a = 0xff000000ff; > > > > rte_atomic64_cmpset( &a, 0xff000000ff, 0xfa000000fa); > > printf("0x%lx\n", a); > > } > > > > When using 'mov', the result is 0xfa00000000 > > When using 'xchgl', the result is 0xfa000000fa, as expected. > > This operation is used a lot in drivers for link status. > > I think we need to clearly explain what was the consequence of this bug. Resurrecting this old thread, with my analysis. The issue is indeed as described above, the low dword of the result of the 64-bit cmpset is incorrect, if the exchange takes place. This is due to the incorrect source value not being placed in the ebx register. What is meant to happen is that, if the old value (from EDX:EAX) matches the value in the memory location, that memory location is written to by the new value from ECX:EBX. However, for PIC code, we can't use EBX register so the parameter is placed in EDI register instead. The first line is meant to be moving the EDI value to EBX, but instead is doing the opposite, of moving the current EBX value to EDI. This leads to the incorrect result. An alternative fix would be the following code: asm volatile ( "push %%ebx;" "mov %%edi, %%ebx;" MPLOCKED "cmpxchg8b (%[dst]);" "setz %[res];" "mov %%ebx, %%edi;" "pop %%ebx;" : [res] "=a" (res) /* result in eax */ : [dst] "S" (dst), /* esi */ "D" (_src.l32), /* edi, copied to ebx */ "c" (_src.h32), /* ecx */ "a" (_exp.l32), /* eax */ "d" (_exp.h32) /* edx */ : "memory" ); /* no-clobber list */ However, the xchg to swap the registers at the start and swap them back at the end is shorter. Couple of other comments on this code area that should be taken into account: 1. the indentation of the asm code looks wrong, and should probably be fixed to make it more readable. 2. the comment on the "D" register is wrong as it refers to ebx 3. the fact that we can't use ebx, and instead use edi and swap twice should be commented. For the fix itself: Acked-by: Bruce Richardson <bruce.richardson@intel.com> Regards, /Bruce
+Correct email for Thomas. On Mon, Sep 04, 2017 at 02:02:05PM +0100, Bruce Richardson wrote: > On Fri, Feb 10, 2017 at 11:53:06AM +0100, Thomas Monjalon wrote: > > 2017-02-10 10:39, Hunt, David: > > > > > > On 9/2/2017 4:53 PM, Thomas Monjalon wrote: > > > > 2016-11-06 22:09, Thomas Monjalon: > > > >> 2016-09-29 18:34, Thomas Monjalon: > > > >>> 2016-09-30 02:54, Nikhil Rao: > > > >>>> The original code used movl instead of xchgl, this caused > > > >>>> rte_atomic64_cmpset to use ebx as the lower dword of the source > > > >>>> to cmpxchg8b instead of the lower dword of function argument "src". > > > >>> Could you please start the explanation with a statement of > > > >>> what is wrong from an user point of view? > > > >>> It could help to understand how severe it is. > > > >> Please, we need a clear explanation of the bug, and an acknowledgement. > > > > Should we close this bug? > > > > > > I took a few minutes to look at this, and the issue can easily be > > > reproduced with a small snippet of code. > > > With the 'mov', the lower dword of the result is incorrect. This is > > > resolved by using 'xchgl'. > > > > > > void main() > > > { > > > uint64_t a = 0xff000000ff; > > > > > > rte_atomic64_cmpset( &a, 0xff000000ff, 0xfa000000fa); > > > printf("0x%lx\n", a); > > > } > > > > > > When using 'mov', the result is 0xfa00000000 > > > When using 'xchgl', the result is 0xfa000000fa, as expected. > > > > This operation is used a lot in drivers for link status. > > > > I think we need to clearly explain what was the consequence of this bug. > > Resurrecting this old thread, with my analysis. > > The issue is indeed as described above, the low dword of the result of > the 64-bit cmpset is incorrect, if the exchange takes place. This is due > to the incorrect source value not being placed in the ebx register. > > What is meant to happen is that, if the old value (from EDX:EAX) matches > the value in the memory location, that memory location is written to by > the new value from ECX:EBX. However, for PIC code, we can't use EBX > register so the parameter is placed in EDI register instead. The first > line is meant to be moving the EDI value to EBX, but instead is doing > the opposite, of moving the current EBX value to EDI. This leads to the > incorrect result. > > An alternative fix would be the following code: > > asm volatile ( > "push %%ebx;" > "mov %%edi, %%ebx;" > MPLOCKED "cmpxchg8b (%[dst]);" > "setz %[res];" > "mov %%ebx, %%edi;" > "pop %%ebx;" > : [res] "=a" (res) /* result in eax */ > : [dst] "S" (dst), /* esi */ > "D" (_src.l32), /* edi, copied to ebx */ > "c" (_src.h32), /* ecx */ > "a" (_exp.l32), /* eax */ > "d" (_exp.h32) /* edx */ > : "memory" ); /* no-clobber list */ > > However, the xchg to swap the registers at the start and swap them back > at the end is shorter. > > Couple of other comments on this code area that should be taken into > account: > 1. the indentation of the asm code looks wrong, and should probably be > fixed to make it more readable. > 2. the comment on the "D" register is wrong as it refers to ebx > 3. the fact that we can't use ebx, and instead use edi and swap twice > should be commented. > > For the fix itself: > > Acked-by: Bruce Richardson <bruce.richardson@intel.com> > > Regards, > /Bruce
04/09/2017 15:02, Bruce Richardson: > Couple of other comments on this code area that should be taken into > account: > 1. the indentation of the asm code looks wrong, and should probably be > fixed to make it more readable. > 2. the comment on the "D" register is wrong as it refers to ebx > 3. the fact that we can't use ebx, and instead use edi and swap twice > should be commented. A patch to fix all these comments would be welcome :) > For the fix itself: > > Acked-by: Bruce Richardson <bruce.richardson@intel.com> Applied (more than one year after the submission), thanks
diff --git a/lib/librte_eal/common/include/arch/x86/rte_atomic_32.h b/lib/librte_eal/common/include/arch/x86/rte_atomic_32.h index 2e04c75..fb3abf1 100644 --- a/lib/librte_eal/common/include/arch/x86/rte_atomic_32.h +++ b/lib/librte_eal/common/include/arch/x86/rte_atomic_32.h @@ -81,7 +81,7 @@ rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src) : "memory" ); /* no-clobber list */ #else asm volatile ( - "mov %%ebx, %%edi\n" + "xchgl %%ebx, %%edi;\n" MPLOCKED "cmpxchg8b (%[dst]);" "setz %[res];"