| Message ID | 20191219115436.10354-10-elibr@mellanox.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | netdev datapath actions offload | expand |
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
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. >
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. >>
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.
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
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.
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(-)