[ovs-dev] dpif-netdev: Avoid side-effect in argument of atomic_store_relaxed().

Some of the implementations of atomic_store_relaxed() evaluate their
first argument more than once, so arguments with side effects cause
strange behavior.  This fixes a problem observed on 64-bit Windows.

Reported-by: Alin Serdean <aserdean@ovn.org>
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 lib/dpif-netdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Thanks a lot for fixing this!

Acked-by: Alin Serdean <aserdean@ovn.org>

> -----Original Message-----
> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
> bounces@openvswitch.org] On Behalf Of Ben Pfaff
> Sent: Wednesday, September 6, 2017 6:58 PM
> To: dev@openvswitch.org
> Cc: Ben Pfaff <blp@ovn.org>; Alin Serdean <aserdean@ovn.org>
> Subject: [ovs-dev] [PATCH] dpif-netdev: Avoid side-effect in argument of
> atomic_store_relaxed().
> 
> Some of the implementations of atomic_store_relaxed() evaluate their first
> argument more than once, so arguments with side effects cause strange
> behavior.  This fixes a problem observed on 64-bit Windows.
> 
> Reported-by: Alin Serdean <aserdean@ovn.org>
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
>  lib/dpif-netdev.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
> 071ec141f1d1..0ceef9d82914 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -3201,8 +3201,8 @@ static void
>  dp_netdev_rxq_set_intrvl_cycles(struct dp_netdev_rxq *rx,
>                                  unsigned long long cycles)  {
> -   atomic_store_relaxed(&rx->cycles_intrvl[rx->intrvl_idx++
> -                                           % PMD_RXQ_INTERVAL_MAX],
cycles);
> +    unsigned int idx = rx->intrvl_idx++ % PMD_RXQ_INTERVAL_MAX;
> +    atomic_store_relaxed(&rx->cycles_intrvl[idx], cycles);
>  }
> 
>  static uint64_t
> --
> 2.10.2
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On 9/6/17, 8:57 AM, "ovs-dev-bounces@openvswitch.org on behalf of Ben Pfaff" <ovs-dev-bounces@openvswitch.org on behalf of blp@ovn.org> wrote:

    Some of the implementations of atomic_store_relaxed() evaluate their
    first argument more than once, so arguments with side effects cause
    strange behavior.  This fixes a problem observed on 64-bit Windows.

Acked-by: Darrell Ball <dlu998@gmail.com>
    
    Reported-by: Alin Serdean <aserdean@ovn.org>
    Signed-off-by: Ben Pfaff <blp@ovn.org>
    ---
     lib/dpif-netdev.c | 4 ++--
     1 file changed, 2 insertions(+), 2 deletions(-)
    
    diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
    index 071ec141f1d1..0ceef9d82914 100644
    --- a/lib/dpif-netdev.c
    +++ b/lib/dpif-netdev.c
    @@ -3201,8 +3201,8 @@ static void
     dp_netdev_rxq_set_intrvl_cycles(struct dp_netdev_rxq *rx,
                                     unsigned long long cycles)
     {
    -   atomic_store_relaxed(&rx->cycles_intrvl[rx->intrvl_idx++
    -                                           % PMD_RXQ_INTERVAL_MAX], cycles);
    +    unsigned int idx = rx->intrvl_idx++ % PMD_RXQ_INTERVAL_MAX;
    +    atomic_store_relaxed(&rx->cycles_intrvl[idx], cycles);
     }
     
     static uint64_t
    -- 
    2.10.2
    
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=dbPiJUxk4mmv_rFKdPUGAFvYi0T-dlJbVYyuRzb0gtQ&s=pwkvuvnWKzBBebzMnL_KG-Hkl2YICM8ZeyhU_f984Sg&e=

Thanks Alin and Darrell, I applied this to master.  I don't think it
needs any backports.

On Wed, Sep 06, 2017 at 10:41:25PM +0300, aserdean@ovn.org wrote:
> Thanks a lot for fixing this!
> 
> Acked-by: Alin Serdean <aserdean@ovn.org>
> 
> > -----Original Message-----
> > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
> > bounces@openvswitch.org] On Behalf Of Ben Pfaff
> > Sent: Wednesday, September 6, 2017 6:58 PM
> > To: dev@openvswitch.org
> > Cc: Ben Pfaff <blp@ovn.org>; Alin Serdean <aserdean@ovn.org>
> > Subject: [ovs-dev] [PATCH] dpif-netdev: Avoid side-effect in argument of
> > atomic_store_relaxed().
> > 
> > Some of the implementations of atomic_store_relaxed() evaluate their first
> > argument more than once, so arguments with side effects cause strange
> > behavior.  This fixes a problem observed on 64-bit Windows.
> > 
> > Reported-by: Alin Serdean <aserdean@ovn.org>
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> > ---
> >  lib/dpif-netdev.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
> > 071ec141f1d1..0ceef9d82914 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -3201,8 +3201,8 @@ static void
> >  dp_netdev_rxq_set_intrvl_cycles(struct dp_netdev_rxq *rx,
> >                                  unsigned long long cycles)  {
> > -   atomic_store_relaxed(&rx->cycles_intrvl[rx->intrvl_idx++
> > -                                           % PMD_RXQ_INTERVAL_MAX],
> cycles);
> > +    unsigned int idx = rx->intrvl_idx++ % PMD_RXQ_INTERVAL_MAX;
> > +    atomic_store_relaxed(&rx->cycles_intrvl[idx], cycles);
> >  }
> > 
> >  static uint64_t
> > --
> > 2.10.2
> > 
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>