[ovs-dev] [PATCH 1/1] netdev-offload-dpdk: Preserve HW statistics for modified flows
Sriharsha Basavapatna
sriharsha.basavapatna at broadcom.com
Mon Oct 12 15:26:55 UTC 2020
On Mon, Oct 12, 2020 at 4:43 PM Eli Britstein <elibr at nvidia.com> wrote:
>
>
> On 10/12/2020 11:45 AM, Sriharsha Basavapatna wrote:
> > On Sun, Sep 6, 2020 at 5:52 PM Eli Britstein <elibr at nvidia.com> wrote:
> >> ping
> >>
> >> On 7/30/2020 4:52 PM, Eli Britstein wrote:
> >>> In case of a flow modification, preserve the HW statistics of the old HW
> >>> flow to the new one.
> >>>
> >>> Signed-off-by: Eli Britstein <elibr at mellanox.com>
> >>> Reviewed-by: Gaetan Rivet <gaetanr at mellanox.com>
> > Update fixes: tag
> I'll put 3c7330ebf036 netdev-offload-dpdk: Support offload of output action.
> As it's the first commit that does full offload. Before that there are
> no HW rules that count. What do you think?
That's fine.
> >>> ---
> >>> lib/netdev-offload-dpdk.c | 28 +++++++++++++++++++++-------
> >>> 1 file changed, 21 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> >>> index de6101e4d..960acb2da 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
> >>> @@ -1407,7 +1408,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,
> >>> @@ -1416,6 +1417,7 @@ 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;
> > We can eliminate 'ret' with this change, since it is only being used
> > to catch the return value of parse_flow_match().
> Ack
> >>> @@ -1442,13 +1444,13 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev,
> >>> ret = -1;
> > Relates to the above comment.
> >>> 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
> >>> @@ -1482,14 +1484,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) {
> >>> @@ -1497,11 +1504,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);
> > What if it is a new flow, don't we need to clear the stats ?
>
> It is already cleared before. Allocation is xZalloc. See in
> ufid_to_rte_flow_associate:
>
> struct ufid_to_rte_flow_data *data = xzalloc(sizeof *data);
Ok, thanks.
>
> >>> + *stats = rte_flow_data->stats;
> >>> }
> >>> - return netdev_offload_dpdk_add_flow(netdev, match, actions,
> >>> - actions_len, ufid, info);
> >>> + return 0;
> >>> }
> >>>
> >>> static int
> >> _______________________________________________
> >> dev mailing list
> >> dev at openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list