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

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 of pmd
when we have a large number of rules in dp.

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
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(-)

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)
#113 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: 200, 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 Fri, Jul 24, 2020 at 4:01 PM 0-day Robot <robot@bytheb.org> wrote:
>
> 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)
> #113 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: 200, Warnings: 1, Errors: 0
I guess we should ignore this warning.
>
> Please check this out.  If you feel there has been an error, please email aconole@redhat.com
>
> Thanks,
> 0-day Robot

On Fri, Jul 24, 2020 at 4:26 PM Tonghao Zhang <xiangxia.m.yue@gmail.com>
wrote:

> On Fri, Jul 24, 2020 at 4:01 PM 0-day Robot <robot@bytheb.org> wrote:
> >
> > 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)
> > #113 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: 200, Warnings: 1, Errors: 0
> I guess we should ignore this warning.
> >


Any comment about this?

>
> > Please check this out.  If you feel there has been an error, please
> email aconole@redhat.com
> >
> > Thanks,
> > 0-day Robot
>
>
>
> --
> Best regards, Tonghao
>

On Mon, Aug 10, 2020 at 11:31 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
>
>
>
> On Fri, Jul 24, 2020 at 4:26 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
>>
>> On Fri, Jul 24, 2020 at 4:01 PM 0-day Robot <robot@bytheb.org> wrote:
>> >
>> > 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)
>> > #113 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: 200, Warnings: 1, Errors: 0
>> I guess we should ignore this warning.
>> >
>
>
> Any comment about this?
Hi maintainers, please review.
That patch was sent for a long time. Any maintainer has plan to review ?
>>
>> > Please check this out.  If you feel there has been an error, please email aconole@redhat.com
>> >
>> > Thanks,
>> > 0-day Robot
>>
>>
>>
>> --
>> Best regards, Tonghao
>
> --
> Best regards, Tonghao

Hello,

On 24/07/20 15:33 +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 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 <xiangxia.m.yue@gmail.com>
> ---
> 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
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On 10/14/20 4:27 PM, Gaëtan Rivet wrote:
> Hello,
> 
> On 24/07/20 15:33 +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 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.

Hi Gaëtan,
Thanks for review.  However, since v2 there was 'v2 resend' and v3 of
this patch.  Some comments here are still relevant, but it's better to
review v3 now since it changed significantly.

v2 resend with review comments:
https://patchwork.ozlabs.org/project/openvswitch/patch/20200824024408.56559-1-xiangxia.m.yue@gmail.com/

v3:
https://patchwork.ozlabs.org/project/openvswitch/patch/20200928091722.2830-1-xiangxia.m.yue@gmail.com/

Best regards, Ilya Maximets.

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, 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 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 <xiangxia.m.yue@gmail.com>
> > ---
> > 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
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 
> -- 
> Gaëtan
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev