On Wed, Jan 8, 2020 at 11:34 PM Ilya Maximets <[email protected]> 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 <[email protected]> > >>> --- > >>> 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
