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