[ovs-dev] [PATCH V6 09/18] dpctl: Support dump-flows filters "dpdk" and "partially-offloaded"

Ilya Maximets i.maximets at ovn.org
Wed Jan 8 18:04:12 UTC 2020


On 08.01.2020 18:25, Eli Britstein wrote:
> 
> On 1/8/2020 2:17 PM, Ilya Maximets wrote:
>> On 19.12.2019 12:54, Eli Britstein wrote:
>>> Flows that are offloaded via DPDK can be partially offloaded (matches
>>> only) or fully offloaded (matches and actions). Set partially offloaded
>>> display to (offloaded=partial, dp:ovs), and fully offloaded to
>>> (offloaded=yes, dp:dpdk). Also support filter types "dpdk" and
>>> "partially-offloaded".
>>>
>>> Signed-off-by: Eli Britstein <elibr at mellanox.com>
>>> ---
>>>   NEWS          |  3 ++
>>>   lib/dpctl.c   | 97 ++++++++++++++++++++++++++++++++++++++++++++---------------
>>>   lib/dpctl.man |  2 ++
>>>   3 files changed, 78 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/NEWS b/NEWS
>>> index 2acde3fe8..85c4a4812 100644
>>> --- a/NEWS
>>> +++ b/NEWS
>>> @@ -26,6 +26,9 @@ Post-v2.12.0
>>>        * DPDK ring ports (dpdkr) are deprecated and will be removed in next
>>>          releases.
>>>        * Add support for DPDK 19.11.
>>> +   - 'ovs-appctl dpctl/dump-flows' can now show offloaded=partial for
>>> +     partially offloaded flows, dp:dpdk for fully offloaded by dpdk, and
>>> +     type filter supports new filters: "dpdk" and "partially-offloaded".
>>>   
>>>   v2.12.0 - 03 Sep 2019
>>>   ---------------------
>>> diff --git a/lib/dpctl.c b/lib/dpctl.c
>>> index 0ee64718c..387f61ed4 100644
>>> --- a/lib/dpctl.c
>>> +++ b/lib/dpctl.c
>>> @@ -818,7 +818,11 @@ format_dpif_flow(struct ds *ds, const struct dpif_flow *f, struct hmap *ports,
>>>   
>>>       dpif_flow_stats_format(&f->stats, ds);
>>>       if (dpctl_p->verbosity && f->attrs.offloaded) {
>>> -        ds_put_cstr(ds, ", offloaded:yes");
>>> +        if (f->attrs.dp_layer && !strcmp(f->attrs.dp_layer, "ovs")) {
>>> +            ds_put_cstr(ds, ", offloaded:partial");
>>> +        } else {
>>> +            ds_put_cstr(ds, ", offloaded:yes");
>>> +        }
>>>       }
>>>       if (dpctl_p->verbosity && f->attrs.dp_layer) {
>>>           ds_put_format(ds, ", dp:%s", f->attrs.dp_layer);
>>> @@ -827,20 +831,30 @@ format_dpif_flow(struct ds *ds, const struct dpif_flow *f, struct hmap *ports,
>>>       format_odp_actions(ds, f->actions, f->actions_len, ports);
>>>   }
>>>   
>>> +enum dp_type {
>>> +    DP_TYPE_ANY,
>>> +    DP_TYPE_OVS,
>>> +    DP_TYPE_TC,
>>> +    DP_TYPE_DPDK,
>>> +};
>>> +
>>> +enum ol_type {
>>> +    OL_TYPE_ANY,
>>> +    OL_TYPE_NO,
>>> +    OL_TYPE_YES,
>>> +    OL_TYPE_PARTIAL,
>>> +};
>>> +
>>>   struct dump_types {
>>> -    bool ovs;
>>> -    bool tc;
>>> -    bool offloaded;
>>> -    bool non_offloaded;
>>> +    enum dp_type dp_type;
>>> +    enum ol_type ol_type;
>>>   };
>>>   
>>>   static void
>>>   enable_all_dump_types(struct dump_types *dump_types)
>>>   {
>>> -    dump_types->ovs = true;
>>> -    dump_types->tc = true;
>>> -    dump_types->offloaded = true;
>>> -    dump_types->non_offloaded = true;
>>> +    dump_types->dp_type = DP_TYPE_ANY;
>>> +    dump_types->ol_type = OL_TYPE_ANY;
>>>   }
>>>   
>>>   static int
>>> @@ -862,13 +876,17 @@ populate_dump_types(char *types_list, struct dump_types *dump_types,
>>>           current_type[type_len] = '\0';
>>>   
>>>           if (!strcmp(current_type, "ovs")) {
>>> -            dump_types->ovs = true;
>>> +            dump_types->dp_type = DP_TYPE_OVS;
>>>           } else if (!strcmp(current_type, "tc")) {
>>> -            dump_types->tc = true;
>>> +            dump_types->dp_type = DP_TYPE_TC;
>>> +        } else if (!strcmp(current_type, "dpdk")) {
>>> +            dump_types->dp_type = DP_TYPE_DPDK;
>>>           } else if (!strcmp(current_type, "offloaded")) {
>>> -            dump_types->offloaded = true;
>>> +            dump_types->ol_type = OL_TYPE_YES;
>>>           } else if (!strcmp(current_type, "non-offloaded")) {
>>> -            dump_types->non_offloaded = true;
>>> +            dump_types->ol_type = OL_TYPE_NO;
>>> +        } else if (!strcmp(current_type, "partially-offloaded")) {
>>> +            dump_types->ol_type = OL_TYPE_PARTIAL;
>>>           } else if (!strcmp(current_type, "all")) {
>>>               enable_all_dump_types(dump_types);
>>>           } else {
>>> @@ -884,30 +902,61 @@ static void
>>>   determine_dpif_flow_dump_types(struct dump_types *dump_types,
>>>                                  struct dpif_flow_dump_types *dpif_dump_types)
>>>   {
>>> -    dpif_dump_types->ovs_flows = dump_types->ovs || dump_types->non_offloaded;
>>> -    dpif_dump_types->netdev_flows = dump_types->tc || dump_types->offloaded
>>> -                                    || dump_types->non_offloaded;
>>> +    dpif_dump_types->ovs_flows = dump_types->dp_type == DP_TYPE_OVS;
>>> +    dpif_dump_types->netdev_flows = (dump_types->dp_type == DP_TYPE_TC ||
>>> +                                     dump_types->dp_type == DP_TYPE_DPDK);
>>>   }
>> Above code doesn't handle DP_TYPE_ANY.  This mostly breaks dump-flows
>> command if no type specified.
> Correct. I already fixed it today. That was the first issue with make 
> check-offloads.
>>
>> Some more issues:  I think this patch will not handle multiple flow
>> types correctly.  For example, "dump-flows --type=tc,dpdk" should
>> dump "flows that are in TC, but not in HW" + "flows that are in HW via
>> DPDK". So, it should not dump flows that handled purely by OVS or
>> offloaded to HW via TC.  I believe, this case will not work correctly
>> with current implementation of this patch.
> 
> My understanding of this "type" parameter was that it is a kind of a 
> filter, that should narrow down the flows dumped, so the condition is 
> *AND* and not *OR*.
> 
> With "--type=tc,dpdk", it won't dump anything as a flow can't be both.
> 
> The second issue with make check-offloads was that it checked 
> "tc,offloaded", and got nothing, as those flows were "tc", but not 
> offloaded.
> 
> I think that's more correct behavior, so I changed the test.
> 
> Please comment your thoughts before I submit v7.

The test is correct.  And condition should be *OR*.  Most of the
options are mutually exclusive, so *AND* doesn't make much sense here.

'tc' stands for flows that handled by TC, i.e. not offloaded to HW.
'offloaded' stands for flows offloaded to HW.
So, 'tc,offloaded' - flows that are not in OVS and handled by TC or HW.

Best regards, Ilya Maximets.


More information about the dev mailing list