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

Eli Britstein elibr at nvidia.com
Mon Oct 12 11:13:15 UTC 2020


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?
>>> ---
>>>    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);

>>> +        *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