[ovs-dev] [PATCH V6 09/18] dpctl: Support dump-flows filters "dpdk" and "partially-offloaded"
Sriharsha Basavapatna
sriharsha.basavapatna at broadcom.com
Thu Jan 9 08:58:14 UTC 2020
On Wed, Jan 8, 2020 at 11:34 PM Ilya Maximets <i.maximets at 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 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.
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
More information about the dev
mailing list