Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: fix bug in x86 cmpset

Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: fix bug in x86 cmpset

[Christian Ehrhardt]

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


-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: fix bug in x86 cmpset

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

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

Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: fix bug in x86 cmpset

[Thomas Monjalon]

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.