[ovs-dev,v2] netdev-tc-offloads: update stats properly on flow deletion

Currently, when an offloaded DP flow is deleted, the related stats
are unconditionally cleared. As a result the counters for the
originating open flow are corrupted.

This change addresses the issue updating the DP stats with the current
values provided by the flower APIs before deleting the tc filter, as
currently done by others DP providers.

Tested vs different OVS H/W offload devices, verifying that the open flow
stats are correct after the expiration of the related, H/W offloaded DP
flow.

Fixes: 30b6b047260b ("netdev-tc-offloads: Implement netdev flow del using tc interface")
CC: Paul Blakey <paulb@mellanox.com>
Reported-by: Kumar Sanghvi <kumaras@chelsio.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v1 -> v2:
 - ensure to always clear stats if tc_get_flower() fails for some reasons
---
 lib/netdev-tc-offloads.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

On Fri, 17 Nov 2017 14:36:22 +0100
Paolo Abeni <pabeni@redhat.com> wrote:
> Currently, when an offloaded DP flow is deleted, the related stats
> are unconditionally cleared. As a result the counters for the
> originating open flow are corrupted.
> 
> This change addresses the issue updating the DP stats with the current
> values provided by the flower APIs before deleting the tc filter, as
> currently done by others DP providers.
> 
> Tested vs different OVS H/W offload devices, verifying that the open flow
> stats are correct after the expiration of the related, H/W offloaded DP
> flow.
> 
> Fixes: 30b6b047260b ("netdev-tc-offloads: Implement netdev flow del using tc interface")
> CC: Paul Blakey <paulb@mellanox.com>
> Reported-by: Kumar Sanghvi <kumaras@chelsio.com>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> v1 -> v2:
>  - ensure to always clear stats if tc_get_flower() fails for some reasons
> ---
>  lib/netdev-tc-offloads.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> index f86f3e046..781d849e4 100644
> --- a/lib/netdev-tc-offloads.c
> +++ b/lib/netdev-tc-offloads.c
> @@ -1101,6 +1101,7 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
>                     const ovs_u128 *ufid,
>                     struct dpif_flow_stats *stats)
>  {
> +    struct tc_flower flower;
>      struct netdev *dev;
>      int prio = 0;
>      int ifindex;
> @@ -1120,14 +1121,20 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
>          return -ifindex;
>      }
>  
> +    if (stats) {
> +        memset(stats, 0, sizeof *stats);
> +        if (!tc_get_flower(ifindex, prio, handle, &flower)) {
> +            stats->n_packets = get_32aligned_u64(&flower.stats.n_packets);
> +            stats->n_bytes = get_32aligned_u64(&flower.stats.n_bytes);
> +            stats->used = flower.lastused;
> +        }
> +    }
> +
>      error = tc_del_filter(ifindex, prio, handle);
>      del_ufid_tc_mapping(ufid);
>  
>      netdev_close(dev);
>  
> -    if (stats) {
> -        memset(stats, 0, sizeof *stats);
> -    }
>      return error;
>  }
>  

Thanks for the V2 :-)
Acked-by: Flavio Leitner <fbl@sysclose.org>

On Fri, Nov 17, 2017 at 04:45:05PM -0200, Flavio Leitner wrote:
> On Fri, 17 Nov 2017 14:36:22 +0100
> Paolo Abeni <pabeni@redhat.com> wrote:
> > Currently, when an offloaded DP flow is deleted, the related stats
> > are unconditionally cleared. As a result the counters for the
> > originating open flow are corrupted.
> > 
> > This change addresses the issue updating the DP stats with the current
> > values provided by the flower APIs before deleting the tc filter, as
> > currently done by others DP providers.
> > 
> > Tested vs different OVS H/W offload devices, verifying that the open flow
> > stats are correct after the expiration of the related, H/W offloaded DP
> > flow.
> > 
> > Fixes: 30b6b047260b ("netdev-tc-offloads: Implement netdev flow del using tc interface")
> > CC: Paul Blakey <paulb@mellanox.com>
> > Reported-by: Kumar Sanghvi <kumaras@chelsio.com>
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > v1 -> v2:
> >  - ensure to always clear stats if tc_get_flower() fails for some reasons
> > ---
> >  lib/netdev-tc-offloads.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> > index f86f3e046..781d849e4 100644
> > --- a/lib/netdev-tc-offloads.c
> > +++ b/lib/netdev-tc-offloads.c
> > @@ -1101,6 +1101,7 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
> >                     const ovs_u128 *ufid,
> >                     struct dpif_flow_stats *stats)
> >  {
> > +    struct tc_flower flower;
> >      struct netdev *dev;
> >      int prio = 0;
> >      int ifindex;
> > @@ -1120,14 +1121,20 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
> >          return -ifindex;
> >      }
> >  
> > +    if (stats) {
> > +        memset(stats, 0, sizeof *stats);
> > +        if (!tc_get_flower(ifindex, prio, handle, &flower)) {
> > +            stats->n_packets = get_32aligned_u64(&flower.stats.n_packets);
> > +            stats->n_bytes = get_32aligned_u64(&flower.stats.n_bytes);
> > +            stats->used = flower.lastused;
> > +        }
> > +    }
> > +
> >      error = tc_del_filter(ifindex, prio, handle);
> >      del_ufid_tc_mapping(ufid);
> >  
> >      netdev_close(dev);
> >  
> > -    if (stats) {
> > -        memset(stats, 0, sizeof *stats);
> > -    }
> >      return error;
> >  }
> >  
> 
> Thanks for the V2 :-)
> Acked-by: Flavio Leitner <fbl@sysclose.org>

Thanks, applied to master and branch-2.8.