Hi, Sorry it seems I missed the v3 actually! Please disregard this review. I will copy the still relevant comments there.
On 14/10/20 16:27 +0200, Gaëtan Rivet wrote: > Hello, > > On 24/07/20 15:33 +0800, [email protected] wrote: > > From: Tonghao Zhang <[email protected]> > > > > "ovs-appctl dpctl/dump-flows" added the option > > "pmd" which allow user to dump pmd specified. > > > > That option is useful to dump rules of pmd > > when we have a large number of rules in dp. > > > > Thanks for the patch! I think it can be useful. > A few comments below. > > > Signed-off-by: Tonghao Zhang <[email protected]> > > --- > > v2: > > * rebase the codes > > * add usage > > --- > > NEWS | 2 ++ > > lib/dpctl.c | 28 +++++++++++++++++++++++----- > > lib/dpctl.man | 6 +++++- > > lib/dpif-netdev.c | 25 +++++++++++++++++++++++++ > > lib/dpif-provider.h | 2 ++ > > 5 files changed, 57 insertions(+), 6 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index dceda95a381a..04a05914c72e 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -38,6 +38,8 @@ v2.14.0 - xx xxx xxxx > > * Add runtime CPU ISA detection to allow optimized ISA functions > > * Add support for dynamically changing DPCLS subtable lookup functions > > * Add ISA optimized DPCLS lookup function using AVX512 > > + * Command "ovs-appctl dpctl/dump-flows" added option "pmd" which allow > > + user to dump pmd specified. > > It would be better to follow the same format as other items in the list. > Single quotes should be used instead of double, past-tense should be > avoided. I can propose this instead: > > * Add the 'pmd' option to "ovs-appctl dpctl/dump-flows", which > restricts a flow dump to a single PMD threads if set. > > > - New configuration knob 'other_config:bond-primary' for AB bonds > > that specifies interface will be the preferred port if it is active. > > - Tunnels: TC Flower offload > > diff --git a/lib/dpctl.c b/lib/dpctl.c > > index 09ae97f25cf3..a76e3e520804 100644 > > --- a/lib/dpctl.c > > +++ b/lib/dpctl.c > > @@ -979,6 +979,7 @@ dpctl_dump_flows(int argc, const char *argv[], struct > > dpctl_params *dpctl_p) > > struct dpif_flow_dump_thread *flow_dump_thread; > > struct dpif_flow_dump *flow_dump; > > struct dpif_flow f; > > + int pmd_id_filter = PMD_ID_NULL; > > int pmd_id = PMD_ID_NULL; > > int lastargc = 0; > > int error; > > @@ -996,6 +997,12 @@ dpctl_dump_flows(int argc, const char *argv[], struct > > dpctl_params *dpctl_p) > > goto out_free; > > } > > types_list = xstrdup(argv[--argc] + 5); > > + } else if (pmd_id_filter == PMD_ID_NULL && > > + !strncmp(argv[argc - 1], "pmd=", 4)) { > > + if (!ovs_scan(argv[--argc], "pmd=%"SCNu32, &pmd_id_filter)) { > > The SCNu32 format is to scan a uint32_t value. > To stay consistent with 'pmd_id_filter' type as it follows PMD_ID_NULL > definition, > the scan format should be '%d' instead. > > It could also be useful for the user to be able to dump flows from the > main thread, but we cannot expect them to use NON_PMD_CORE_ID. Parsing > could recognize 'main' as well maybe? Then the filter would be set to > NON_PMD_CORE_ID. I'm not sure it would then work with > dpif_netdev_flow_dump_next(), have you tested it? > > > + error = EINVAL; > > + goto out_free; > > + } > > } > > } > > > > @@ -1044,7 +1051,13 @@ dpctl_dump_flows(int argc, const char *argv[], > > struct dpctl_params *dpctl_p) > > ds_init(&ds); > > memset(&f, 0, sizeof f); > > flow_dump = dpif_flow_dump_create(dpif, false, &dpif_dump_types); > > + flow_dump->pmd_id = pmd_id_filter; > > flow_dump_thread = dpif_flow_dump_thread_create(flow_dump); > > + if (!flow_dump_thread) { > > + error = ENOENT; > > + goto out_dump_destroy; > > + } > > + > > while (dpif_flow_dump_next(flow_dump_thread, &f, 1)) { > > if (filter) { > > struct flow flow; > > @@ -1085,11 +1098,16 @@ dpctl_dump_flows(int argc, const char *argv[], > > struct dpctl_params *dpctl_p) > > } > > } > > dpif_flow_dump_thread_destroy(flow_dump_thread); > > - error = dpif_flow_dump_destroy(flow_dump); > > > > - if (error) { > > - dpctl_error(dpctl_p, error, "Failed to dump flows from datapath"); > > +out_dump_destroy: > > + { > > + int ret = dpif_flow_dump_destroy(flow_dump); > > I'm not convinced this is a good idea or necessary to open an anonymous scope > just to avoid declaring ret at the function level. Please move ret > definition at the top instead. > > > + if (ret) { > > + dpctl_error(dpctl_p, ret, "Failed to dump flows from > > datapath"); > > + error = ret; > > + } > > } > > + > > ds_destroy(&ds); > > > > out_dpifclose: > > @@ -2503,8 +2521,8 @@ static const struct dpctl_command all_commands[] = { > > { "set-if", "dp iface...", 2, INT_MAX, dpctl_set_if, DP_RW }, > > { "dump-dps", "", 0, 0, dpctl_dump_dps, DP_RO }, > > { "show", "[dp...]", 0, INT_MAX, dpctl_show, DP_RO }, > > - { "dump-flows", "[dp] [filter=..] [type=..]", > > - 0, 3, dpctl_dump_flows, DP_RO }, > > + { "dump-flows", "[dp] [filter=..] [type=..] [pmd=..]", > > + 0, 4, dpctl_dump_flows, DP_RO }, > > { "add-flow", "[dp] flow actions", 2, 3, dpctl_add_flow, DP_RW }, > > { "mod-flow", "[dp] flow actions", 2, 3, dpctl_mod_flow, DP_RW }, > > { "get-flow", "[dp] ufid", 1, 2, dpctl_get_flow, DP_RO }, > > diff --git a/lib/dpctl.man b/lib/dpctl.man > > index 727d1f7be8d4..192bee489de7 100644 > > --- a/lib/dpctl.man > > +++ b/lib/dpctl.man > > @@ -104,7 +104,7 @@ default. When multiple datapaths exist, then a > > datapath name is > > required. > > . > > .TP > > -.DO "[\fB\-m \fR| \fB\-\-more\fR] [\fB\-\-names \fR| \fB\-\-no\-names\fR]" > > \*(DX\fBdump\-flows\fR "[\fIdp\fR] [\fBfilter=\fIfilter\fR] > > [\fBtype=\fItype\fR]" > > +.DO "[\fB\-m \fR| \fB\-\-more\fR] [\fB\-\-names \fR| \fB\-\-no\-names\fR]" > > \*(DX\fBdump\-flows\fR "[\fIdp\fR] [\fBfilter=\fIfilter\fR] > > [\fBtype=\fItype\fR] [\fBpmd=\fIpmd\fR]" > > I saw the zero-day bot warning. As long as the manpage output is > properly limited in width I think you can ignore it. > > > Prints to the console all flow entries in datapath \fIdp\fR's flow > > table. Without \fB\-m\fR or \fB\-\-more\fR, output omits match fields > > that a flow wildcards entirely; with \fB\-m\fR or \fB\-\-more\fR, > > @@ -118,6 +118,10 @@ The \fIfilter\fR is also useful to match wildcarded > > fields in the datapath > > flow. As an example, \fBfilter='tcp,tp_src=100'\fR will match the > > datapath flow containing '\fBtcp(src=80/0xff00,dst=8080/0xff)\fR'. > > .IP > > +If \fBpmd=\fIpmd\fR is specified, only displays flows of the specified pmd. > > +The \fBpmd=\fI-1\fR means that dump only the flows on the main pthread. > > +This option supported only for \fBuserspace datapath\fR. > > +.IP > > If \fBtype=\fItype\fR is specified, only displays flows of the specified > > types. > > This option supported only for \fBovs\-appctl dpctl/dump\-flows\fR. > > \fItype\fR is a comma separated list, which can contain any of the > > following: > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > > index 2aad24511c1f..5f144564d3d6 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -4007,6 +4007,21 @@ dpif_netdev_flow_dump_thread_create(struct > > dpif_flow_dump *dump_) > > struct dpif_netdev_flow_dump *dump = dpif_netdev_flow_dump_cast(dump_); > > struct dpif_netdev_flow_dump_thread *thread; > > > > + dump->cur_pmd = NULL; > > + if (dump->up.pmd_id != PMD_ID_NULL) { > > + struct dp_netdev *dp = get_dp_netdev(dump->up.dpif); > > + struct dp_netdev_pmd_thread *pmd; > > + > > + pmd = dp_netdev_get_pmd(dp, dump->up.pmd_id); > > + if (!pmd || !dp_netdev_pmd_try_ref(pmd)) { > > + VLOG_DBG("The PMD ID (%u) not found or ref.\n", > > + dump->up.pmd_id); > > + return NULL; > > + } > > + > > + dump->cur_pmd = pmd; > > + } > > + > > thread = xmalloc(sizeof *thread); > > dpif_flow_dump_thread_init(&thread->up, &dump->up); > > thread->dump = dump; > > @@ -4018,6 +4033,11 @@ dpif_netdev_flow_dump_thread_destroy(struct > > dpif_flow_dump_thread *thread_) > > { > > struct dpif_netdev_flow_dump_thread *thread > > = dpif_netdev_flow_dump_thread_cast(thread_); > > + struct dpif_netdev_flow_dump *dump = thread->dump; > > + > > + if (dump->up.pmd_id != PMD_ID_NULL && dump->cur_pmd) { > > + dp_netdev_pmd_unref(dump->cur_pmd); > > + } > > > > free(thread); > > } > > @@ -4063,6 +4083,11 @@ dpif_netdev_flow_dump_next(struct > > dpif_flow_dump_thread *thread_, > > struct dp_netdev_flow, > > node); > > } > > + > > You need to comment here why the loop must break. > > Actually, reading the code I think 'up.pmd_id' does not seem explicit > enough. Within the dpif_flow_dump you could call it 'pmd_id_filter' instead? > > But even with this change, a comment would be welcome here. > > > + if (dump->up.pmd_id != PMD_ID_NULL) { > > + break; > > + } > > + > > /* When finishing dumping the current pmd thread, moves to > > * the next. */ > > if (n_flows < flow_limit) { > > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h > > index 0e024c1c9851..aef96277d4d7 100644 > > --- a/lib/dpif-provider.h > > +++ b/lib/dpif-provider.h > > @@ -57,6 +57,7 @@ static inline void dpif_assert_class(const struct dpif > > *dpif, > > > > struct dpif_flow_dump { > > struct dpif *dpif; > > + unsigned pmd_id; /* As a filter for PMD flows. */ > > It seems logical to want an 'unsigned int' for the PMD id, but given all > future comparison and parsing is done on integers, instead it should be > an 'int' to stay consistent. > > Reading other definitions it seems that 'unsigned int' should be used > instead if it was to stay an unsigned integer. > > > bool terse; /* If true, key/mask/actions may be omitted. */ > > }; > > > > @@ -64,6 +65,7 @@ static inline void > > dpif_flow_dump_init(struct dpif_flow_dump *dump, const struct dpif *dpif) > > { > > dump->dpif = CONST_CAST(struct dpif *, dpif); > > + dump->pmd_id = PMD_ID_NULL; > > } > > > > struct dpif_flow_dump_thread { > > -- > > 2.26.1 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > -- > Gaëtan > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev -- Gaëtan _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
