diff mbox series

[ovs-dev,V2,1/1] netdev-offload-dpdk: Preserve HW statistics for modified flows

Message ID 20201012142735.5304-1-elibr@nvidia.com
State Accepted
Headers show
Series [ovs-dev,V2,1/1] netdev-offload-dpdk: Preserve HW statistics for modified flows | expand

Commit Message

Eli Britstein Oct. 12, 2020, 2:27 p.m. UTC
In case of a flow modification, preserve the HW statistics of the old HW
flow to the new one.

Fixes: 3c7330ebf036 ("netdev-offload-dpdk: Support offload of output action.")
Signed-off-by: Eli Britstein <elibr@nvidia.com>
Reviewed-by: Gaetan Rivet <gaetanr@nvidia.com>
---
 lib/netdev-offload-dpdk.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

Comments

Sriharsha Basavapatna Oct. 14, 2020, 11:16 a.m. UTC | #1
On Mon, Oct 12, 2020 at 7:57 PM Eli Britstein <elibr@nvidia.com> wrote:
>
> In case of a flow modification, preserve the HW statistics of the old HW
> flow to the new one.
>
> Fixes: 3c7330ebf036 ("netdev-offload-dpdk: Support offload of output action.")
> Signed-off-by: Eli Britstein <elibr@nvidia.com>
> Reviewed-by: Gaetan Rivet <gaetanr@nvidia.com>

Acked-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>

> ---
>  lib/netdev-offload-dpdk.c | 33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> index 5b632bac4..dadd8f253 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
> @@ -78,7 +78,7 @@ ufid_to_rte_flow_data_find(const ovs_u128 *ufid)
>      return NULL;
>  }
>
> -static inline void
> +static inline struct ufid_to_rte_flow_data *
>  ufid_to_rte_flow_associate(const ovs_u128 *ufid,
>                             struct rte_flow *rte_flow, bool actions_offloaded)
>  {
> @@ -103,6 +103,7 @@ ufid_to_rte_flow_associate(const ovs_u128 *ufid,
>
>      cmap_insert(&ufid_to_rte_flow,
>                  CONST_CAST(struct cmap_node *, &data->node), hash);
> +    return data;
>  }
>
>  static inline void
> @@ -1420,7 +1421,7 @@ out:
>      return flow;
>  }
>
> -static int
> +static struct ufid_to_rte_flow_data *
>  netdev_offload_dpdk_add_flow(struct netdev *netdev,
>                               struct match *match,
>                               struct nlattr *nl_actions,
> @@ -1429,12 +1430,11 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev,
>                               struct offload_info *info)
>  {
>      struct flow_patterns patterns = { .items = NULL, .cnt = 0 };
> +    struct ufid_to_rte_flow_data *flows_data = NULL;
>      bool actions_offloaded = true;
>      struct rte_flow *flow;
> -    int ret = 0;
>
> -    ret = parse_flow_match(&patterns, match);
> -    if (ret) {
> +    if (parse_flow_match(&patterns, match)) {
>          VLOG_DBG_RL(&rl, "%s: matches of ufid "UUID_FMT" are not supported",
>                      netdev_get_name(netdev), UUID_ARGS((struct uuid *) ufid));
>          goto out;
> @@ -1452,16 +1452,15 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev,
>      }
>
>      if (!flow) {
> -        ret = -1;
>          goto out;
>      }
> -    ufid_to_rte_flow_associate(ufid, flow, actions_offloaded);
> +    flows_data = ufid_to_rte_flow_associate(ufid, flow, actions_offloaded);
>      VLOG_DBG("%s: installed flow %p by ufid "UUID_FMT,
>               netdev_get_name(netdev), flow, UUID_ARGS((struct uuid *)ufid));
>
>  out:
>      free_flow_patterns(&patterns);
> -    return ret;
> +    return flows_data;
>  }
>
>  static int
> @@ -1495,14 +1494,19 @@ netdev_offload_dpdk_flow_put(struct netdev *netdev, struct match *match,
>                               struct dpif_flow_stats *stats)
>  {
>      struct ufid_to_rte_flow_data *rte_flow_data;
> +    struct dpif_flow_stats old_stats;
> +    bool modification = false;
>      int ret;
>
>      /*
>       * If an old rte_flow exists, it means it's a flow modification.
>       * Here destroy the old rte flow first before adding a new one.
> +     * Keep the stats for the newly created rule.
>       */
>      rte_flow_data = ufid_to_rte_flow_data_find(ufid);
>      if (rte_flow_data && rte_flow_data->rte_flow) {
> +        old_stats = rte_flow_data->stats;
> +        modification = true;
>          ret = netdev_offload_dpdk_destroy_flow(netdev, ufid,
>                                                 rte_flow_data->rte_flow);
>          if (ret < 0) {
> @@ -1510,11 +1514,18 @@ netdev_offload_dpdk_flow_put(struct netdev *netdev, struct match *match,
>          }
>      }
>
> +    rte_flow_data = netdev_offload_dpdk_add_flow(netdev, match, actions,
> +                                                 actions_len, ufid, info);
> +    if (!rte_flow_data) {
> +        return -1;
> +    }
> +    if (modification) {
> +        rte_flow_data->stats = old_stats;
> +    }
>      if (stats) {
> -        memset(stats, 0, sizeof *stats);
> +        *stats = rte_flow_data->stats;
>      }
> -    return netdev_offload_dpdk_add_flow(netdev, match, actions,
> -                                        actions_len, ufid, info);
> +    return 0;
>  }
>
>  static int
> --
> 2.26.2.1730.g385c171
>
Finn, Emma Oct. 15, 2020, 11:46 a.m. UTC | #2
> -----Original Message-----
> From: dev <ovs-dev-bounces@openvswitch.org> On Behalf Of Sriharsha
> Basavapatna via dev
> Sent: Wednesday 14 October 2020 12:17
> To: Eli Britstein <elibr@nvidia.com>
> Cc: ovs dev <dev@openvswitch.org>; Gaetan Rivet <gaetanr@nvidia.com>; Ilya
> Maximets <i.maximets@ovn.org>
> Subject: Re: [ovs-dev] [PATCH V2 1/1] netdev-offload-dpdk: Preserve HW
> statistics for modified flows
> 
> On Mon, Oct 12, 2020 at 7:57 PM Eli Britstein <elibr@nvidia.com> wrote:
> >
> > In case of a flow modification, preserve the HW statistics of the old
> > HW flow to the new one.
> >
> > Fixes: 3c7330ebf036 ("netdev-offload-dpdk: Support offload of output
> > action.")
> > Signed-off-by: Eli Britstein <elibr@nvidia.com>
> > Reviewed-by: Gaetan Rivet <gaetanr@nvidia.com>
> 
> Acked-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
> 

Tested with X710 devices. 
Will do further testing with E810 devices when the patches become available in DPDK. 
Tested-by: Emma Finn <emma.finn@intel.com>

> > ---
> >  lib/netdev-offload-dpdk.c | 33 ++++++++++++++++++++++-----------
> >  1 file changed, 22 insertions(+), 11 deletions(-)
> >
> > diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> > index 5b632bac4..dadd8f253 100644
> > --- a/lib/netdev-offload-dpdk.c
> > +++ b/lib/netdev-offload-dpdk.c
> > @@ -78,7 +78,7 @@ ufid_to_rte_flow_data_find(const ovs_u128 *ufid)
> >      return NULL;
> >  }
> >
> > -static inline void
> > +static inline struct ufid_to_rte_flow_data *
> >  ufid_to_rte_flow_associate(const ovs_u128 *ufid,
> >                             struct rte_flow *rte_flow, bool
> > actions_offloaded)  { @@ -103,6 +103,7 @@
> > ufid_to_rte_flow_associate(const ovs_u128 *ufid,
> >
> >      cmap_insert(&ufid_to_rte_flow,
> >                  CONST_CAST(struct cmap_node *, &data->node), hash);
> > +    return data;
> >  }
> >
> >  static inline void
> > @@ -1420,7 +1421,7 @@ out:
> >      return flow;
> >  }
> >
> > -static int
> > +static struct ufid_to_rte_flow_data *
> >  netdev_offload_dpdk_add_flow(struct netdev *netdev,
> >                               struct match *match,
> >                               struct nlattr *nl_actions, @@ -1429,12
> > +1430,11 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev,
> >                               struct offload_info *info)  {
> >      struct flow_patterns patterns = { .items = NULL, .cnt = 0 };
> > +    struct ufid_to_rte_flow_data *flows_data = NULL;
> >      bool actions_offloaded = true;
> >      struct rte_flow *flow;
> > -    int ret = 0;
> >
> > -    ret = parse_flow_match(&patterns, match);
> > -    if (ret) {
> > +    if (parse_flow_match(&patterns, match)) {
> >          VLOG_DBG_RL(&rl, "%s: matches of ufid "UUID_FMT" are not
> supported",
> >                      netdev_get_name(netdev), UUID_ARGS((struct uuid *) ufid));
> >          goto out;
> > @@ -1452,16 +1452,15 @@ netdev_offload_dpdk_add_flow(struct netdev
> *netdev,
> >      }
> >
> >      if (!flow) {
> > -        ret = -1;
> >          goto out;
> >      }
> > -    ufid_to_rte_flow_associate(ufid, flow, actions_offloaded);
> > +    flows_data = ufid_to_rte_flow_associate(ufid, flow,
> > + actions_offloaded);
> >      VLOG_DBG("%s: installed flow %p by ufid "UUID_FMT,
> >               netdev_get_name(netdev), flow, UUID_ARGS((struct uuid
> > *)ufid));
> >
> >  out:
> >      free_flow_patterns(&patterns);
> > -    return ret;
> > +    return flows_data;
> >  }
> >
> >  static int
> > @@ -1495,14 +1494,19 @@ netdev_offload_dpdk_flow_put(struct netdev
> *netdev, struct match *match,
> >                               struct dpif_flow_stats *stats)  {
> >      struct ufid_to_rte_flow_data *rte_flow_data;
> > +    struct dpif_flow_stats old_stats;
> > +    bool modification = false;
> >      int ret;
> >
> >      /*
> >       * If an old rte_flow exists, it means it's a flow modification.
> >       * Here destroy the old rte flow first before adding a new one.
> > +     * Keep the stats for the newly created rule.
> >       */
> >      rte_flow_data = ufid_to_rte_flow_data_find(ufid);
> >      if (rte_flow_data && rte_flow_data->rte_flow) {
> > +        old_stats = rte_flow_data->stats;
> > +        modification = true;
> >          ret = netdev_offload_dpdk_destroy_flow(netdev, ufid,
> >                                                 rte_flow_data->rte_flow);
> >          if (ret < 0) {
> > @@ -1510,11 +1514,18 @@ netdev_offload_dpdk_flow_put(struct netdev
> *netdev, struct match *match,
> >          }
> >      }
> >
> > +    rte_flow_data = netdev_offload_dpdk_add_flow(netdev, match, actions,
> > +                                                 actions_len, ufid, info);
> > +    if (!rte_flow_data) {
> > +        return -1;
> > +    }
> > +    if (modification) {
> > +        rte_flow_data->stats = old_stats;
> > +    }
> >      if (stats) {
> > -        memset(stats, 0, sizeof *stats);
> > +        *stats = rte_flow_data->stats;
> >      }
> > -    return netdev_offload_dpdk_add_flow(netdev, match, actions,
> > -                                        actions_len, ufid, info);
> > +    return 0;
> >  }
> >
> >  static int
> > --
> > 2.26.2.1730.g385c171
> >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ilya Maximets Nov. 10, 2020, 9:24 a.m. UTC | #3
On 10/15/20 1:46 PM, Finn, Emma wrote:
>> -----Original Message-----
>> From: dev <ovs-dev-bounces@openvswitch.org> On Behalf Of Sriharsha
>> Basavapatna via dev
>> Sent: Wednesday 14 October 2020 12:17
>> To: Eli Britstein <elibr@nvidia.com>
>> Cc: ovs dev <dev@openvswitch.org>; Gaetan Rivet <gaetanr@nvidia.com>; Ilya
>> Maximets <i.maximets@ovn.org>
>> Subject: Re: [ovs-dev] [PATCH V2 1/1] netdev-offload-dpdk: Preserve HW
>> statistics for modified flows
>>
>> On Mon, Oct 12, 2020 at 7:57 PM Eli Britstein <elibr@nvidia.com> wrote:
>>>
>>> In case of a flow modification, preserve the HW statistics of the old
>>> HW flow to the new one.
>>>
>>> Fixes: 3c7330ebf036 ("netdev-offload-dpdk: Support offload of output
>>> action.")
>>> Signed-off-by: Eli Britstein <elibr@nvidia.com>
>>> Reviewed-by: Gaetan Rivet <gaetanr@nvidia.com>
>>
>> Acked-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
>>
> 
> Tested with X710 devices. 
> Will do further testing with E810 devices when the patches become available in DPDK. 
> Tested-by: Emma Finn <emma.finn@intel.com>

Thanks!
Applied to master and backported down to 2.13.

While looking at the patch I noticed that we always return zero stats while
removing the flow.  That doesn't look right.  We should, probably, fix that too.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index 5b632bac4..dadd8f253 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -78,7 +78,7 @@  ufid_to_rte_flow_data_find(const ovs_u128 *ufid)
     return NULL;
 }
 
-static inline void
+static inline struct ufid_to_rte_flow_data *
 ufid_to_rte_flow_associate(const ovs_u128 *ufid,
                            struct rte_flow *rte_flow, bool actions_offloaded)
 {
@@ -103,6 +103,7 @@  ufid_to_rte_flow_associate(const ovs_u128 *ufid,
 
     cmap_insert(&ufid_to_rte_flow,
                 CONST_CAST(struct cmap_node *, &data->node), hash);
+    return data;
 }
 
 static inline void
@@ -1420,7 +1421,7 @@  out:
     return flow;
 }
 
-static int
+static struct ufid_to_rte_flow_data *
 netdev_offload_dpdk_add_flow(struct netdev *netdev,
                              struct match *match,
                              struct nlattr *nl_actions,
@@ -1429,12 +1430,11 @@  netdev_offload_dpdk_add_flow(struct netdev *netdev,
                              struct offload_info *info)
 {
     struct flow_patterns patterns = { .items = NULL, .cnt = 0 };
+    struct ufid_to_rte_flow_data *flows_data = NULL;
     bool actions_offloaded = true;
     struct rte_flow *flow;
-    int ret = 0;
 
-    ret = parse_flow_match(&patterns, match);
-    if (ret) {
+    if (parse_flow_match(&patterns, match)) {
         VLOG_DBG_RL(&rl, "%s: matches of ufid "UUID_FMT" are not supported",
                     netdev_get_name(netdev), UUID_ARGS((struct uuid *) ufid));
         goto out;
@@ -1452,16 +1452,15 @@  netdev_offload_dpdk_add_flow(struct netdev *netdev,
     }
 
     if (!flow) {
-        ret = -1;
         goto out;
     }
-    ufid_to_rte_flow_associate(ufid, flow, actions_offloaded);
+    flows_data = ufid_to_rte_flow_associate(ufid, flow, actions_offloaded);
     VLOG_DBG("%s: installed flow %p by ufid "UUID_FMT,
              netdev_get_name(netdev), flow, UUID_ARGS((struct uuid *)ufid));
 
 out:
     free_flow_patterns(&patterns);
-    return ret;
+    return flows_data;
 }
 
 static int
@@ -1495,14 +1494,19 @@  netdev_offload_dpdk_flow_put(struct netdev *netdev, struct match *match,
                              struct dpif_flow_stats *stats)
 {
     struct ufid_to_rte_flow_data *rte_flow_data;
+    struct dpif_flow_stats old_stats;
+    bool modification = false;
     int ret;
 
     /*
      * If an old rte_flow exists, it means it's a flow modification.
      * Here destroy the old rte flow first before adding a new one.
+     * Keep the stats for the newly created rule.
      */
     rte_flow_data = ufid_to_rte_flow_data_find(ufid);
     if (rte_flow_data && rte_flow_data->rte_flow) {
+        old_stats = rte_flow_data->stats;
+        modification = true;
         ret = netdev_offload_dpdk_destroy_flow(netdev, ufid,
                                                rte_flow_data->rte_flow);
         if (ret < 0) {
@@ -1510,11 +1514,18 @@  netdev_offload_dpdk_flow_put(struct netdev *netdev, struct match *match,
         }
     }
 
+    rte_flow_data = netdev_offload_dpdk_add_flow(netdev, match, actions,
+                                                 actions_len, ufid, info);
+    if (!rte_flow_data) {
+        return -1;
+    }
+    if (modification) {
+        rte_flow_data->stats = old_stats;
+    }
     if (stats) {
-        memset(stats, 0, sizeof *stats);
+        *stats = rte_flow_data->stats;
     }
-    return netdev_offload_dpdk_add_flow(netdev, match, actions,
-                                        actions_len, ufid, info);
+    return 0;
 }
 
 static int