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

Message ID 20191219115436.10354-10-elibr@mellanox.com
State Changes Requested
Delegated to: Ilya Maximets
Headers show
Series
  • netdev datapath actions offload
Related show

Commit Message

Eli Britstein Dec. 19, 2019, 11:54 a.m. UTC
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@mellanox.com>
---
 NEWS          |  3 ++
 lib/dpctl.c   | 97 ++++++++++++++++++++++++++++++++++++++++++++---------------
 lib/dpctl.man |  2 ++
 3 files changed, 78 insertions(+), 24 deletions(-)

Comments

0-day Robot Dec. 19, 2019, 1:18 p.m. UTC | #1
Bleep bloop.  Greetings Eli Britstein, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 96 characters long (recommended limit is 79)
#195 FILE: lib/dpctl.man:130:
   \fBpartially-offloaded\fR - displays flows where only part of their proccessing is done in HW

Lines checked: 201, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Ilya Maximets Jan. 8, 2020, 12:17 p.m. UTC | #2
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@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.

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.

>  
>  static bool
> -flow_passes_type_filter(const struct dpif_flow *f,
> -                        struct dump_types *dump_types)
> +flow_passes_dp_filter(const struct dpif_flow *f,
> +                      struct dump_types *dump_types)
>  {
> -    if (dump_types->ovs && !strcmp(f->attrs.dp_layer, "ovs")) {
> +    if (dump_types->dp_type == DP_TYPE_ANY) {
>          return true;
>      }
> -    if (dump_types->tc && !strcmp(f->attrs.dp_layer, "tc")) {
> -        return true;
> +    if (dump_types->dp_type == DP_TYPE_OVS) {
> +        return !strcmp(f->attrs.dp_layer, "ovs");
>      }
> -    if (dump_types->offloaded && f->attrs.offloaded) {
> -        return true;
> +    if (dump_types->dp_type == DP_TYPE_TC) {
> +        return !strcmp(f->attrs.dp_layer, "tc");
>      }
> -    if (dump_types->non_offloaded && !(f->attrs.offloaded)) {
> +    if (dump_types->dp_type == DP_TYPE_DPDK) {
> +        return !strcmp(f->attrs.dp_layer, "dpdk");
> +    }
> +    /* should never get here. */
> +    return false;
> +}
> +
> +static bool
> +flow_passes_ol_filter(const struct dpif_flow *f,
> +                      struct dump_types *dump_types)
> +{
> +    if (dump_types->ol_type == OL_TYPE_ANY) {
>          return true;
>      }
> +    if (dump_types->ol_type == OL_TYPE_NO) {
> +        return !f->attrs.offloaded;
> +    }
> +    if (dump_types->ol_type == OL_TYPE_YES &&
> +        strcmp(f->attrs.dp_layer, "ovs")) {
> +        return f->attrs.offloaded;
> +    }
> +    if (dump_types->ol_type == OL_TYPE_PARTIAL &&
> +        !strcmp(f->attrs.dp_layer, "ovs")) {
> +        return f->attrs.offloaded;
> +    }
> +    /* should never get here. */
>      return false;
>  }
>  
> +static bool
> +flow_passes_type_filter(const struct dpif_flow *f,
> +                        struct dump_types *dump_types)
> +{
> +    return flow_passes_dp_filter(f, dump_types) &&
> +           flow_passes_ol_filter(f, dump_types);
> +}
> +
>  static struct hmap *
>  dpctl_get_portno_names(struct dpif *dpif, const struct dpctl_params *dpctl_p)
>  {
> diff --git a/lib/dpctl.man b/lib/dpctl.man
> index 090c3b960..727d1f7be 100644
> --- a/lib/dpctl.man
> +++ b/lib/dpctl.man
> @@ -124,8 +124,10 @@ This option supported only for \fBovs\-appctl dpctl/dump\-flows\fR.
>  .
>     \fBovs\fR - displays flows handled in the ovs dp
>     \fBtc\fR - displays flows handled in the tc dp
> +   \fBdpdk\fR - displays flows fully offloaded by dpdk
>     \fBoffloaded\fR - displays flows offloaded to the HW
>     \fBnon-offloaded\fR - displays flows not offloaded to the HW
> +   \fBpartially-offloaded\fR - displays flows where only part of their proccessing is done in HW
>     \fBall\fR - displays all the types of flows
>  .IP
>  By default all the types of flows are displayed.
>
Eli Britstein Jan. 8, 2020, 5:25 p.m. UTC | #3
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@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.

>
>>   
>>   static bool
>> -flow_passes_type_filter(const struct dpif_flow *f,
>> -                        struct dump_types *dump_types)
>> +flow_passes_dp_filter(const struct dpif_flow *f,
>> +                      struct dump_types *dump_types)
>>   {
>> -    if (dump_types->ovs && !strcmp(f->attrs.dp_layer, "ovs")) {
>> +    if (dump_types->dp_type == DP_TYPE_ANY) {
>>           return true;
>>       }
>> -    if (dump_types->tc && !strcmp(f->attrs.dp_layer, "tc")) {
>> -        return true;
>> +    if (dump_types->dp_type == DP_TYPE_OVS) {
>> +        return !strcmp(f->attrs.dp_layer, "ovs");
>>       }
>> -    if (dump_types->offloaded && f->attrs.offloaded) {
>> -        return true;
>> +    if (dump_types->dp_type == DP_TYPE_TC) {
>> +        return !strcmp(f->attrs.dp_layer, "tc");
>>       }
>> -    if (dump_types->non_offloaded && !(f->attrs.offloaded)) {
>> +    if (dump_types->dp_type == DP_TYPE_DPDK) {
>> +        return !strcmp(f->attrs.dp_layer, "dpdk");
>> +    }
>> +    /* should never get here. */
>> +    return false;
>> +}
>> +
>> +static bool
>> +flow_passes_ol_filter(const struct dpif_flow *f,
>> +                      struct dump_types *dump_types)
>> +{
>> +    if (dump_types->ol_type == OL_TYPE_ANY) {
>>           return true;
>>       }
>> +    if (dump_types->ol_type == OL_TYPE_NO) {
>> +        return !f->attrs.offloaded;
>> +    }
>> +    if (dump_types->ol_type == OL_TYPE_YES &&
>> +        strcmp(f->attrs.dp_layer, "ovs")) {
>> +        return f->attrs.offloaded;
>> +    }
>> +    if (dump_types->ol_type == OL_TYPE_PARTIAL &&
>> +        !strcmp(f->attrs.dp_layer, "ovs")) {
>> +        return f->attrs.offloaded;
>> +    }
>> +    /* should never get here. */
>>       return false;
>>   }
>>   
>> +static bool
>> +flow_passes_type_filter(const struct dpif_flow *f,
>> +                        struct dump_types *dump_types)
>> +{
>> +    return flow_passes_dp_filter(f, dump_types) &&
>> +           flow_passes_ol_filter(f, dump_types);
>> +}
>> +
>>   static struct hmap *
>>   dpctl_get_portno_names(struct dpif *dpif, const struct dpctl_params *dpctl_p)
>>   {
>> diff --git a/lib/dpctl.man b/lib/dpctl.man
>> index 090c3b960..727d1f7be 100644
>> --- a/lib/dpctl.man
>> +++ b/lib/dpctl.man
>> @@ -124,8 +124,10 @@ This option supported only for \fBovs\-appctl dpctl/dump\-flows\fR.
>>   .
>>      \fBovs\fR - displays flows handled in the ovs dp
>>      \fBtc\fR - displays flows handled in the tc dp
>> +   \fBdpdk\fR - displays flows fully offloaded by dpdk
>>      \fBoffloaded\fR - displays flows offloaded to the HW
>>      \fBnon-offloaded\fR - displays flows not offloaded to the HW
>> +   \fBpartially-offloaded\fR - displays flows where only part of their proccessing is done in HW
>>      \fBall\fR - displays all the types of flows
>>   .IP
>>   By default all the types of flows are displayed.
>>
Ilya Maximets Jan. 8, 2020, 6:04 p.m. UTC | #4
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@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.
Ginwala, Aliasgar via dev Jan. 9, 2020, 8:58 a.m. UTC | #5
On Wed, Jan 8, 2020 at 11:34 PM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> 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@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.

It'd help if ovs-dpctl manpage is updated to reflect the behavior
explained above (OR condition), may be even with a couple of examples
when multiple comma separated types are provided. Also, we now have 2
types for fully offloaded flows: "dpdk" and "offloaded"; this could be
confusing ?

Thanks,
-Harsha

Patch
diff mbox series

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);
 }
 
 static bool
-flow_passes_type_filter(const struct dpif_flow *f,
-                        struct dump_types *dump_types)
+flow_passes_dp_filter(const struct dpif_flow *f,
+                      struct dump_types *dump_types)
 {
-    if (dump_types->ovs && !strcmp(f->attrs.dp_layer, "ovs")) {
+    if (dump_types->dp_type == DP_TYPE_ANY) {
         return true;
     }
-    if (dump_types->tc && !strcmp(f->attrs.dp_layer, "tc")) {
-        return true;
+    if (dump_types->dp_type == DP_TYPE_OVS) {
+        return !strcmp(f->attrs.dp_layer, "ovs");
     }
-    if (dump_types->offloaded && f->attrs.offloaded) {
-        return true;
+    if (dump_types->dp_type == DP_TYPE_TC) {
+        return !strcmp(f->attrs.dp_layer, "tc");
     }
-    if (dump_types->non_offloaded && !(f->attrs.offloaded)) {
+    if (dump_types->dp_type == DP_TYPE_DPDK) {
+        return !strcmp(f->attrs.dp_layer, "dpdk");
+    }
+    /* should never get here. */
+    return false;
+}
+
+static bool
+flow_passes_ol_filter(const struct dpif_flow *f,
+                      struct dump_types *dump_types)
+{
+    if (dump_types->ol_type == OL_TYPE_ANY) {
         return true;
     }
+    if (dump_types->ol_type == OL_TYPE_NO) {
+        return !f->attrs.offloaded;
+    }
+    if (dump_types->ol_type == OL_TYPE_YES &&
+        strcmp(f->attrs.dp_layer, "ovs")) {
+        return f->attrs.offloaded;
+    }
+    if (dump_types->ol_type == OL_TYPE_PARTIAL &&
+        !strcmp(f->attrs.dp_layer, "ovs")) {
+        return f->attrs.offloaded;
+    }
+    /* should never get here. */
     return false;
 }
 
+static bool
+flow_passes_type_filter(const struct dpif_flow *f,
+                        struct dump_types *dump_types)
+{
+    return flow_passes_dp_filter(f, dump_types) &&
+           flow_passes_ol_filter(f, dump_types);
+}
+
 static struct hmap *
 dpctl_get_portno_names(struct dpif *dpif, const struct dpctl_params *dpctl_p)
 {
diff --git a/lib/dpctl.man b/lib/dpctl.man
index 090c3b960..727d1f7be 100644
--- a/lib/dpctl.man
+++ b/lib/dpctl.man
@@ -124,8 +124,10 @@  This option supported only for \fBovs\-appctl dpctl/dump\-flows\fR.
 .
    \fBovs\fR - displays flows handled in the ovs dp
    \fBtc\fR - displays flows handled in the tc dp
+   \fBdpdk\fR - displays flows fully offloaded by dpdk
    \fBoffloaded\fR - displays flows offloaded to the HW
    \fBnon-offloaded\fR - displays flows not offloaded to the HW
+   \fBpartially-offloaded\fR - displays flows where only part of their proccessing is done in HW
    \fBall\fR - displays all the types of flows
 .IP
 By default all the types of flows are displayed.