[ovs-dev] [v1] dpif-netdev: Add ovs-appctl dpif-netdev/subtable-show.

Finn, Emma emma.finn at intel.com
Wed Jan 8 11:38:25 UTC 2020



> -----Original Message-----
> From: Stokes, Ian <ian.stokes at intel.com>
> Sent: Tuesday 7 January 2020 10:21
> To: Finn, Emma <emma.finn at intel.com>; u9012063 at gmail.com;
> i.maximets at ovn.org; ovs-dev at openvswitch.org
> Subject: Re: [ovs-dev] [v1] dpif-netdev: Add ovs-appctl dpif-
> netdev/subtable-show.
> 
> 
> 
> On 12/20/2019 3:28 PM, Emma Finn wrote:
> > Add an ovs-appctl command to iterate through the dpcls and for each
> > subtable output the miniflow bits for any existing table.
> >
> > $ ovs-appctl dpif-netdev/subtable-show pmd thread numa_id 0
> >    dpcls port 2:
> >      subtable:
> >        unit_0: 2 (0x5)
> >        unit_1: 1 (0x1)
> > pmd thread numa_id 1
> >    dpcls port 3:
> >      subtable:
> >        unit_0: 2 (0x5)
> >        unit_1: 1 (0x1)
> >
> > Signed-off-by: Emma Finn <emma.finn at intel.com>
> >
> 
> Hi Emma,
> 
> thanks for the patch, comments below.
> 
> 
> > ---
> >
> > RFC -> v1
> >
> > * Changed revision from RFC to v1
> > * Reformatted based on comments
> > * Fixed same classifier being dumped multiple times
> >    flagged by Ilya
> > * Fixed print of subtables flagged by William
> > * Updated print count of bits as well as bits themselves
> > ---
> >   NEWS                        |  2 ++
> >   lib/dpif-netdev-unixctl.man |  4 ++++
> >   lib/dpif-netdev.c           | 53
> ++++++++++++++++++++++++++++++++++++++++++++-
> >   3 files changed, 58 insertions(+), 1 deletion(-)
> >
> > diff --git a/NEWS b/NEWS
> > index 2acde3f..2b7b0cc 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -26,6 +26,8 @@ Post-v2.12.0
> >        * DPDK ring ports (dpdkr) are deprecated and will be removed in next
> >          releases.
> >        * Add support for DPDK 19.11.
> > +     * New "ovs-appctl dpif-netdev/subtable-show" command for
> userspace
> > +       datapath to show subtable miniflow bits.
> 
> Will need to rebase to master as new items have been added in NEWS. Also I
> don't think this belongs under the DPDK section, rather the userspace
> header should be added and used i.e.
> 
> +   - Userspace datapath:
> +     * New "ovs-appctl dpif-netdev/subtable-show" command for userspace
> +       datapath to show subtable miniflow bits.
> 
> >
> >   v2.12.0 - 03 Sep 2019
> >   ---------------------
> > diff --git a/lib/dpif-netdev-unixctl.man b/lib/dpif-netdev-unixctl.man
> > index 6c54f6f..c443465 100644
> > --- a/lib/dpif-netdev-unixctl.man
> > +++ b/lib/dpif-netdev-unixctl.man
> > @@ -217,3 +217,7 @@ with port names, which this thread polls.
> >   .
> >   .IP "\fBdpif-netdev/pmd-rxq-rebalance\fR [\fIdp\fR]"
> >   Reassigns rxqs to pmds in the datapath \fIdp\fR based on their current
> usage.
> > +.
> > +.IP "\fBdpif-netdev/subtable-show\fR [\fB-pmd\fR \fIcore\fR] [\fIdp\fR]"
> > +For one or all pmd threads of the datapath \fIdp\fR show the list of
> > +miniflow bits for each subtable in the datapath classifier.
> > \ No newline at end of file
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
> > 8485b54..a166b9e 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -857,6 +857,8 @@ static inline bool
> >   pmd_perf_metrics_enabled(const struct dp_netdev_pmd_thread *pmd);
> >   static void queue_netdev_flow_del(struct dp_netdev_pmd_thread
> *pmd,
> >                                     struct dp_netdev_flow *flow);
> > +static void pmd_info_show_subtable(struct ds *reply,
> > +                                   struct dp_netdev_pmd_thread *pmd);
> >
> >   static void
> >   emc_cache_init(struct emc_cache *flow_cache) @@ -979,6 +981,7 @@
> > enum pmd_info_type {
> >       PMD_INFO_CLEAR_STATS, /* Set the cycles count to 0. */
> >       PMD_INFO_SHOW_RXQ,    /* Show poll lists of pmd threads. */
> >       PMD_INFO_PERF_SHOW,   /* Show pmd performance details. */
> > +    PMD_INFO_SHOW_SUBTABLE, /* Show subtable miniflow bits. */
> >   };
> >
> >   static void
> > @@ -1334,6 +1337,8 @@ dpif_netdev_pmd_info(struct unixctl_conn
> *conn, int argc, const char *argv[],
> >               pmd_info_show_stats(&reply, pmd);
> >           } else if (type == PMD_INFO_PERF_SHOW) {
> >               pmd_info_show_perf(&reply, pmd, (struct pmd_perf_params
> > *)aux);
> > +        } else if (type == PMD_INFO_SHOW_SUBTABLE) {
> > +            pmd_info_show_subtable(&reply, pmd);
> >           }
> >       }
> >       free(pmd_list);
> > @@ -1391,7 +1396,8 @@ dpif_netdev_init(void)
> >   {
> >       static enum pmd_info_type show_aux = PMD_INFO_SHOW_STATS,
> >                                 clear_aux = PMD_INFO_CLEAR_STATS,
> > -                              poll_aux = PMD_INFO_SHOW_RXQ;
> > +                              poll_aux = PMD_INFO_SHOW_RXQ,
> > +                              subtable_aux = PMD_INFO_SHOW_SUBTABLE;
> >
> >       unixctl_command_register("dpif-netdev/pmd-stats-show", "[-pmd
> core] [dp]",
> >                                0, 3, dpif_netdev_pmd_info, @@ -1416,6
> > +1422,9 @@ dpif_netdev_init(void)
> >                                "[-us usec] [-q qlen]",
> >                                0, 10, pmd_perf_log_set_cmd,
> >                                NULL);
> > +    unixctl_command_register("dpif-netdev/subtable-show", "[-pmd core]
> [dp]",
> > +                             0, 3, dpif_netdev_pmd_info,
> > +                             (void *)&subtable_aux);
> >       return 0;
> >   }
> >
> > @@ -8139,3 +8148,45 @@ dpcls_lookup(struct dpcls *cls, const struct
> netdev_flow_key *keys[],
> >       }
> >       return false;
> >   }
> > +
> > +/* Iterate through all dpcls instances and dump out all subtable
> > + * miniflow bits. */
> > +static void
> > +pmd_info_show_subtable(struct ds *reply, struct
> dp_netdev_pmd_thread *pmd)
> > +    OVS_REQUIRES(dp_netdev_mutex)
> > +{
> > +     if (pmd->core_id != NON_PMD_CORE_ID) {
> > +        struct dp_netdev_port *port = NULL;
> > +        struct dp_netdev *dp = pmd->dp;
> Alignment for the structs above, should be 4 spaces indented from where
> the if statement is declared, currently it's only 3.
> > +
> > +         ovs_mutex_lock(&dp->port_mutex);
> > +         HMAP_FOR_EACH (port, node, &dp->ports) {
> > +             odp_port_t in_port = port->port_no;
> > +
>  From here onwards the alignment seems to be out by 1 space, should be 4
> spaces in from HMAP_FOR_EACH i.e. inline with where odp_port_t is
> declared. It's worth re-checking that all indention alignments are 4 spaces for
> the rest of the function.
> 
> > +            struct dpcls *cls = dp_netdev_pmd_lookup_dpcls(pmd, in_port);
> > +            if (!cls) {
> > +                 continue;
> > +            } else {
> > +                struct pvector *pvec = &cls->subtables;
> > +                ds_put_format(reply, "pmd thread numa_id %d "
> > +                              "core_id %u: \n",
> > +                              pmd->numa_id, pmd->core_id);
> > +                 ds_put_format(reply, "  dpcls port %d:
> > + \n",cls->in_port);
> 
> 
> Not sure if it was discussed previously, I'm wondering for verbosity should
> the name of the port be included here also, there could be many ports and at
> first glimpse it may not be obvious which port number corresponds to which
> port in a deployment.
> 
> This info is available already via port->netdev->name so should be easy to
> add as part of the else block (it wont be needed if theres no cls available.
> 
> +            } else {
> +                struct pvector *pvec = &cls->subtables;
> +                const char *name = netdev_get_name(port->netdev);
> +
> +                ds_put_format(reply, "pmd thread numa_id %d "
> +                              "core_id %u: \n",
> +                              pmd->numa_id, pmd->core_id);
> +                ds_put_format(reply, "  port: %s \n", name);
> +                ds_put_format(reply, "  dpcls port %d:
> + \n",cls->in_port);
> 
> 
> What do you think?
> 
> Thanks
> Ian

Thanks for the feedback.
I agree for clarity the port name should probably be included.
I will make the above changes and submit a v2.

Thanks, 
Emma
> > +
> > +                struct dpcls_subtable *subtable;
> > +                PVECTOR_FOR_EACH (subtable, pvec) {
> > +                     ds_put_format(reply, "    subtable: \n");
> > +                     ds_put_format(reply,
> > +                                   "     unit_0: %d (0x%x)\n"
> > +                                   "     unit_1: %d (0x%x)\n",
> > +                                   count_1bits(subtable->mf_bits_set_unit0),
> > +                                   subtable->mf_bits_set_unit0,
> > +                                   count_1bits(subtable->mf_bits_set_unit1),
> > +                                   subtable->mf_bits_set_unit1);
> > +                }
> > +            }
> > +        }
> > +        ovs_mutex_unlock(&dp->port_mutex);
> > +    }
> > +}
> > +
> >


More information about the dev mailing list