| Message ID | 20200928091722.2830-1-xiangxia.m.yue@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v3] dpctl-netdev: Add the option "pmd" for dump-flows | expand |
Bleep bloop. Greetings Tonghao Zhang, 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 175 characters long (recommended limit is 79) #96 FILE: lib/dpctl.man:107: .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]" Lines checked: 113, 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
Hi again, on the proper version this time :) . On 28/09/20 17:17 +0800, xiangxia.m.yue@gmail.com wrote: > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > "ovs-appctl dpctl/dump-flows" added the option > "pmd" which allow user to dump pmd specified. > > That option is useful to dump rules from pmd > when we have a large number of rules in dp. > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > --- > v3: > * remove unnecessary check > * as filter to reduce the size of a printed data. > --- > NEWS | 2 ++ > lib/dpctl.c | 16 ++++++++++++---- > lib/dpctl.man | 6 +++++- > 3 files changed, 19 insertions(+), 5 deletions(-) > > diff --git a/NEWS b/NEWS > index a9c50add2da0..ae68f5fca4e3 100644 > --- a/NEWS > +++ b/NEWS > @@ -3,6 +3,8 @@ Post-v2.14.0 > - OVSDB: > * New unixctl command 'ovsdb-server/get-db-storage-status' to show the > status of the storage that's backing a database. > + * 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 thread if set. > > > v2.14.0 - 17 Aug 2020 > diff --git a/lib/dpctl.c b/lib/dpctl.c > index 09ae97f25cf3..cbf989ce6650 100644 > --- a/lib/dpctl.c > +++ b/lib/dpctl.c > @@ -980,6 +980,7 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p) > struct dpif_flow_dump *flow_dump; > struct dpif_flow f; > int pmd_id = PMD_ID_NULL; > + bool pmd_id_filter = false; > 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 (!strncmp(argv[argc - 1], "pmd=", 4)) { > + if (!ovs_scan(argv[--argc], "pmd=%"SCNu32, &pmd_id)) { The format SCNu32 should be reserved for parsing uint32_t values. You are also expecting users to pass '-1' to specify the main thread. It will work but this is not a solid implementation. That NON_PMD_CORE_ID is 'UINT32_MAX' is an implementation detail, especially when it is inherited from DPDK's LCORE_ID_ANY. You could instead parse using '%d' formatter and if the given ID is '-1', then set pmd_id to NON_PMD_CORE_ID. > + error = EINVAL; > + goto out_free; > + } > + pmd_id_filter = true; > } > } > > @@ -1070,7 +1077,7 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p) > /* If 'pmd_id' is specified, overlapping flows could be dumped from > * different pmd threads. So, separates dumps from different pmds > * by printing a title line. */ > - if (pmd_id != f.pmd_id) { > + if (!pmd_id_filter && pmd_id != f.pmd_id) { > if (f.pmd_id == NON_PMD_CORE_ID) { > ds_put_format(&ds, "flow-dump from the main thread:\n"); > } else { > @@ -1079,7 +1086,8 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p) > } > pmd_id = f.pmd_id; > } > - if (flow_passes_type_filter(&f, &dump_types)) { > + if (pmd_id == f.pmd_id && > + flow_passes_type_filter(&f, &dump_types)) { > format_dpif_flow(&ds, &f, portno_names, dpctl_p); > dpctl_print(dpctl_p, "%s\n", ds_cstr(&ds)); > } > @@ -2503,8 +2511,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]" > 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 I would reword as Using \fBpmd=\fI-1\fR will restrict the dump to flows from the main thread. This option is only supported by the \fBuserspace datapath\fR. > 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: > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Regards,
On Wed, Oct 14, 2020 at 11:06 PM Gaëtan Rivet <grive@u256.net> wrote: > > Hi again, > > on the proper version this time :) . > > On 28/09/20 17:17 +0800, xiangxia.m.yue@gmail.com wrote: > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > "ovs-appctl dpctl/dump-flows" added the option > > "pmd" which allow user to dump pmd specified. > > > > That option is useful to dump rules from pmd > > when we have a large number of rules in dp. > > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > --- > > v3: > > * remove unnecessary check > > * as filter to reduce the size of a printed data. > > --- > > NEWS | 2 ++ > > lib/dpctl.c | 16 ++++++++++++---- > > lib/dpctl.man | 6 +++++- > > 3 files changed, 19 insertions(+), 5 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index a9c50add2da0..ae68f5fca4e3 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -3,6 +3,8 @@ Post-v2.14.0 > > - OVSDB: > > * New unixctl command 'ovsdb-server/get-db-storage-status' to show the > > status of the storage that's backing a database. > > + * 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 thread if set. Ok, thanks. > > > > > > v2.14.0 - 17 Aug 2020 > > diff --git a/lib/dpctl.c b/lib/dpctl.c > > index 09ae97f25cf3..cbf989ce6650 100644 > > --- a/lib/dpctl.c > > +++ b/lib/dpctl.c > > @@ -980,6 +980,7 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p) > > struct dpif_flow_dump *flow_dump; > > struct dpif_flow f; > > int pmd_id = PMD_ID_NULL; > > + bool pmd_id_filter = false; > > 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 (!strncmp(argv[argc - 1], "pmd=", 4)) { > > + if (!ovs_scan(argv[--argc], "pmd=%"SCNu32, &pmd_id)) { > > The format SCNu32 should be reserved for parsing uint32_t values. > You are also expecting users to pass '-1' to specify the main thread. > > It will work but this is not a solid implementation. That NON_PMD_CORE_ID is 'UINT32_MAX' > is an implementation detail, especially when it is inherited from DPDK's LCORE_ID_ANY. Great, thanks. > You could instead parse using '%d' formatter and if the given ID is '-1', then set pmd_id to > NON_PMD_CORE_ID. > > + error = EINVAL; > > + goto out_free; > > + } > > + pmd_id_filter = true; > > } > > } > > > > @@ -1070,7 +1077,7 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p) > > /* If 'pmd_id' is specified, overlapping flows could be dumped from > > * different pmd threads. So, separates dumps from different pmds > > * by printing a title line. */ > > - if (pmd_id != f.pmd_id) { > > + if (!pmd_id_filter && pmd_id != f.pmd_id) { > > if (f.pmd_id == NON_PMD_CORE_ID) { > > ds_put_format(&ds, "flow-dump from the main thread:\n"); > > } else { > > @@ -1079,7 +1086,8 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p) > > } > > pmd_id = f.pmd_id; > > } > > - if (flow_passes_type_filter(&f, &dump_types)) { > > + if (pmd_id == f.pmd_id && > > + flow_passes_type_filter(&f, &dump_types)) { > > format_dpif_flow(&ds, &f, portno_names, dpctl_p); > > dpctl_print(dpctl_p, "%s\n", ds_cstr(&ds)); > > } > > @@ -2503,8 +2511,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]" > > 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 > > I would reword as > > Using \fBpmd=\fI-1\fR will restrict the dump to flows from the main thread. > This option is only supported by the \fBuserspace datapath\fR. Ok > > 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: > > -- > > 1.8.3.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Regards, > -- > Gaëtan
diff --git a/NEWS b/NEWS index a9c50add2da0..ae68f5fca4e3 100644 --- a/NEWS +++ b/NEWS @@ -3,6 +3,8 @@ Post-v2.14.0 - OVSDB: * New unixctl command 'ovsdb-server/get-db-storage-status' to show the status of the storage that's backing a database. + * Command "ovs-appctl dpctl/dump-flows" added option "pmd" which allow + user to dump pmd specified. v2.14.0 - 17 Aug 2020 diff --git a/lib/dpctl.c b/lib/dpctl.c index 09ae97f25cf3..cbf989ce6650 100644 --- a/lib/dpctl.c +++ b/lib/dpctl.c @@ -980,6 +980,7 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p) struct dpif_flow_dump *flow_dump; struct dpif_flow f; int pmd_id = PMD_ID_NULL; + bool pmd_id_filter = false; 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 (!strncmp(argv[argc - 1], "pmd=", 4)) { + if (!ovs_scan(argv[--argc], "pmd=%"SCNu32, &pmd_id)) { + error = EINVAL; + goto out_free; + } + pmd_id_filter = true; } } @@ -1070,7 +1077,7 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p) /* If 'pmd_id' is specified, overlapping flows could be dumped from * different pmd threads. So, separates dumps from different pmds * by printing a title line. */ - if (pmd_id != f.pmd_id) { + if (!pmd_id_filter && pmd_id != f.pmd_id) { if (f.pmd_id == NON_PMD_CORE_ID) { ds_put_format(&ds, "flow-dump from the main thread:\n"); } else { @@ -1079,7 +1086,8 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p) } pmd_id = f.pmd_id; } - if (flow_passes_type_filter(&f, &dump_types)) { + if (pmd_id == f.pmd_id && + flow_passes_type_filter(&f, &dump_types)) { format_dpif_flow(&ds, &f, portno_names, dpctl_p); dpctl_print(dpctl_p, "%s\n", ds_cstr(&ds)); } @@ -2503,8 +2511,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]" 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: