diff mbox series

[ovs-dev,v3] dpctl-netdev: Add the option "pmd" for dump-flows

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

Commit Message

Tonghao Zhang Sept. 28, 2020, 9:17 a.m. UTC
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(-)

Comments

0-day Robot Sept. 28, 2020, 9:59 a.m. UTC | #1
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
Gaëtan Rivet Oct. 14, 2020, 3:06 p.m. UTC | #2
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,
Tonghao Zhang Oct. 15, 2020, 1:47 a.m. UTC | #3
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 mbox series

Patch

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: