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

Stokes, Ian ian.stokes at intel.com
Tue Jan 7 10:21:04 UTC 2020



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
> +
> +                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