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

Gaëtan Rivet grive at u256.net
Wed Oct 14 14:44:09 UTC 2020


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

-- 
Gaëtan


More information about the dev mailing list