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

Flavio Leitner fbl at sysclose.org
Thu Sep 3 12:31:55 UTC 2020


Hi,

Thanks for the patch. I just did a quick look and found couple
things to discuss.

On Mon, Aug 24, 2020 at 10:44:08AM +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.
> 
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue at gmail.com>
> ---
>  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 2f67d504790c..6a7308aaaafd 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -40,6 +40,8 @@ v2.14.0 - 17 Aug 2020
>       * 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.
>     - 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)) {

We can simplify that by not checking pmd_id_filter. In the worse
case scenario it will use the last pmd= parameter, right?


> +            if (!ovs_scan(argv[--argc], "pmd=%"SCNu32, &pmd_id_filter)) {
> +                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;
> +    }

This should not be possible, see below.


> +
>      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);
> +        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]"
>  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 02df8f11e9ac..678badf02532 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;

This is part of a documented API and returning NULL is not possible.
See struct dpif_class declaration in lib/dpif-provider.h, which I
copy&pasted below the relevant part below:

    /* Flow dumping interface.
     *
     * This is the back-end for the flow dumping interface described in
     * dpif.h.  Please read the comments there first, because this code
     * closely follows it.
     *
     * 'flow_dump_create' and 'flow_dump_thread_create' must always return an
     * initialized and usable data structure and defer error return until
     * flow_dump_destroy().  This hasn't been a problem for the dpifs that
     * exist so far.
     *
     * 'flow_dump_create' and 'flow_dump_thread_create' must initialize the
     * structures that they return with dpif_flow_dump_init() and
     * dpif_flow_dump_thread_init(), respectively.

--
fbl

> +        }
> +
> +        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);
>              }
> +
> +            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. */
>      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 {
> -- 
> 1.8.3.1
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev



More information about the dev mailing list