[ovs-dev] [v14 06/11] dpif-netdev: Add command to get dpif implementations.
Ferriter, Cian
cian.ferriter at intel.com
Thu Jul 8 08:41:46 UTC 2021
Hi Flavio,
Thanks for your comments. My responses are inline.
Cian
> -----Original Message-----
> From: Flavio Leitner <fbl at sysclose.org>
> Sent: Wednesday 7 July 2021 18:29
> To: Ferriter, Cian <cian.ferriter at intel.com>
> Cc: ovs-dev at openvswitch.org; i.maximets at ovn.org
> Subject: Re: [ovs-dev] [v14 06/11] dpif-netdev: Add command to get dpif implementations.
>
>
> Hello,
>
> Please find my comments below.
>
> On Thu, Jul 01, 2021 at 04:06:14PM +0100, Cian Ferriter wrote:
> > From: Harry van Haaren <harry.van.haaren at intel.com>
> >
> > This commit adds a new command to retrieve the list of available
> > DPIF implementations. This can be used by to check what implementations
> > of the DPIF are available in any given OVS binary. It also returns which
> > implementations are in use by the OVS PMD threads.
> >
> > Usage:
> > $ ovs-appctl dpif-netdev/dpif-impl-get
> >
> > Signed-off-by: Harry van Haaren <harry.van.haaren at intel.com>
> > Co-authored-by: Cian Ferriter <cian.ferriter at intel.com>
> > Signed-off-by: Cian Ferriter <cian.ferriter at intel.com>
> >
> > ---
> >
> > v14:
> > - Rename command to dpif-impl-get.
> > - Hide more of the dpif impl details from lib/dpif-netdev.c. Pass a
> > dynamic_string to return the dpif-impl-get CMD output.
> > - Add information about which DPIF impl is currently in use by each PMD
> > thread.
> >
> > v13:
> > - Add NEWS item about DPIF get and set commands here rather than in a
> > later commit.
> > - Add documentation items about DPIF set commands here rather than in a
> > later commit.
> > ---
> > Documentation/topics/dpdk/bridge.rst | 8 +++++++
> > NEWS | 1 +
> > lib/dpif-netdev-private-dpif.c | 33 ++++++++++++++++++++++++++++
> > lib/dpif-netdev-private-dpif.h | 8 +++++++
> > lib/dpif-netdev-unixctl.man | 3 +++
> > lib/dpif-netdev.c | 30 +++++++++++++++++++++++++
> > 6 files changed, 83 insertions(+)
> >
> > diff --git a/Documentation/topics/dpdk/bridge.rst b/Documentation/topics/dpdk/bridge.rst
> > index 06d1f943c..2d0850836 100644
> > --- a/Documentation/topics/dpdk/bridge.rst
> > +++ b/Documentation/topics/dpdk/bridge.rst
> > @@ -226,6 +226,14 @@ stats associated with the datapath.
> > Just like with the SIMD DPCLS feature above, SIMD can be applied to the DPIF to
> > improve performance.
> >
> > +OVS provides multiple implementations of the DPIF. The available
> > +implementations can be listed with the following command ::
> > +
> > + $ ovs-appctl dpif-netdev/dpif-impl-get
> > + Available DPIF implementations:
> > + dpif_scalar (pmds: none)
> > + dpif_avx512 (pmds: 1,2,6,7)
> > +
> > By default, dpif_scalar is used. The DPIF implementation can be selected by
> > name ::
> >
> > diff --git a/NEWS b/NEWS
> > index e23506225..cf0987a24 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -13,6 +13,7 @@ Post-v2.15.0
> > * Refactor lib/dpif-netdev.c to multiple header files.
> > * Add avx512 implementation of dpif which can process non recirculated
> > packets. It supports partial HWOL, EMC, SMC and DPCLS lookups.
> > + * Add commands to get and set the dpif implementations.
> > - ovs-ctl:
> > * New option '--no-record-hostname' to disable hostname configuration
> > in ovsdb on startup.
> > diff --git a/lib/dpif-netdev-private-dpif.c b/lib/dpif-netdev-private-dpif.c
> > index da3511f51..4eaefb291 100644
> > --- a/lib/dpif-netdev-private-dpif.c
> > +++ b/lib/dpif-netdev-private-dpif.c
> > @@ -92,6 +92,39 @@ dp_netdev_impl_set_default_by_name(const char *name)
> >
> > }
> >
> > +uint32_t
> > +dp_netdev_impl_get(struct ds *reply, struct dp_netdev_pmd_thread **pmd_list,
> > + size_t n)
> > +{
> > + /* Add all dpif functions to reply string. */
> > + ds_put_cstr(reply, "Available DPIF implementations:\n");
> > +
> > + for (uint32_t i = 0; i < ARRAY_SIZE(dpif_impls); i++) {
> > + ds_put_format(reply, " %s (pmds: ", dpif_impls[i].name);
> > +
> > + for (size_t j = 0; j < n; j++) {
> > + struct dp_netdev_pmd_thread *pmd = pmd_list[j];
> > + if (pmd->core_id == NON_PMD_CORE_ID) {
> > + continue;
> > + }
> > +
> > + if (pmd->netdev_input_func == dpif_impls[i].input_func) {
> > + ds_put_format(reply, "%u,", pmd->core_id);
> > + }
> > + }
> > +
> > + ds_chomp(reply, ',');
> > +
> > + if (ds_last(reply) == ' ') {
> > + ds_put_cstr(reply, "none");
> > + }
> > +
> > + ds_put_cstr(reply, ")\n");
> > + }
> > +
> > + return ARRAY_SIZE(dpif_impls);
> > +}
> > +
> > /* This function checks all available DPIF implementations, and selects the
> > * returns the function pointer to the one requested by "name".
> > */
> > diff --git a/lib/dpif-netdev-private-dpif.h b/lib/dpif-netdev-private-dpif.h
> > index 0e58153f4..d2c2cbaf4 100644
> > --- a/lib/dpif-netdev-private-dpif.h
> > +++ b/lib/dpif-netdev-private-dpif.h
> > @@ -22,6 +22,7 @@
> > /* Forward declarations to avoid including files. */
> > struct dp_netdev_pmd_thread;
> > struct dp_packet_batch;
> > +struct ds;
> >
> > /* Typedef for DPIF functions.
> > * Returns whether all packets were processed successfully.
> > @@ -48,6 +49,13 @@ struct dpif_netdev_impl_info_t {
> > const char *name;
> > };
> >
> > +/* This function returns all available implementations to the caller. The
> > + * quantity of implementations is returned by the int return value.
> > + */
> > +uint32_t
> > +dp_netdev_impl_get(struct ds *reply, struct dp_netdev_pmd_thread **pmd_list,
> > + size_t n);
> > +
> > /* This function checks all available DPIF implementations, and selects the
> > * returns the function pointer to the one requested by "name".
> > */
> > diff --git a/lib/dpif-netdev-unixctl.man b/lib/dpif-netdev-unixctl.man
> > index 76cc949f9..5f9256215 100644
> > --- a/lib/dpif-netdev-unixctl.man
> > +++ b/lib/dpif-netdev-unixctl.man
> > @@ -227,5 +227,8 @@ When this is the case, the above command prints the load-balancing information
> > of the bonds configured in datapath \fIdp\fR showing the interface associated
> > with each bucket (hash).
> > .
> > +.IP "\fBdpif-netdev/dpif-impl-get\fR
> > +Lists the DPIF implementations that are available.
> > +.
> > .IP "\fBdpif-netdev/dpif-impl-set\fR \fIdpif_impl\fR"
> > Sets the DPIF to be used to \fIdpif_impl\fR. By default "dpif_scalar" is used.
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 19917c7c5..55c05f02b 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -980,6 +980,33 @@ dpif_netdev_subtable_lookup_set(struct unixctl_conn *conn, int argc,
> > ds_destroy(&reply);
> > }
> >
> > +static void
> > +dpif_netdev_impl_get(struct unixctl_conn *conn, int argc OVS_UNUSED,
> > + const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> > +{
> > + struct ds reply = DS_EMPTY_INITIALIZER;
> > + struct shash_node *node;
> > + uint32_t count = 0;
> > +
> > + SHASH_FOR_EACH (node, &dp_netdevs) {
>
> This is missing ovs_mutex_lock(&dp_netdev_mutex), right?
>
>
Yes, good catch, you are right. I was thinking at first: "we aren't making any changes to dp_netdev, which is what the mutex protects against", but we don't want any changes made to dp_netdevs while we are 'getting' them.
I'll fix this.
> > + struct dp_netdev *dp = node->data;
> > +
> > + /* Get PMD threads list, required to get DPCLS instances. */
> > + size_t n;
> > + struct dp_netdev_pmd_thread **pmd_list;
>
> Reverse xmas tree order please.
>
I'll fix this order to what you have below in your code snippet.
> > + sorted_poll_thread_list(dp, &pmd_list, &n);
> > + count = dp_netdev_impl_get(&reply, pmd_list, n);
> > + }
> > +
> > + if (count == 0) {
> > + unixctl_command_reply_error(conn, "Error getting dpif names.");
> > + } else {
> > + unixctl_command_reply(conn, ds_cstr(&reply));
> > + }
>
> This error checking on count doesn't look right as it only applies
> to the last dp. Giving that dp_netdev_impl_get() returns a constant
> regardless of dp (can't really fail actually), I think it could be
> simplified to:
>
> 1. dp_netdev_impl_get() returns void.
> 2. Prints the list as built by dp_netdev_impl_get().
>
> ovs_mutex_lock(&dp_netdev_mutex);
> SHASH_FOR_EACH (node, &dp_netdevs) {
> struct dp_netdev_pmd_thread **pmd_list;
> struct dp_netdev *dp = node->data;
> size_t n;
>
> /* Get PMD threads list, required to get DPCLS instances. */
> sorted_poll_thread_list(dp, &pmd_list, &n);
> dp_netdev_impl_get(&reply, pmd_list, n);
> }
> ovs_mutex_unlock(&dp_netdev_mutex);
> unixctl_command_reply(conn, ds_cstr(&reply));
> ds_destroy(&reply);
>
> Does that make sense?
> Thanks
> fbl
>
That makes perfect sense, thanks for the improvement. I'll implement the above.
> > +
> > + ds_destroy(&reply);
> > +}
> > +
> > static void
> > dpif_netdev_impl_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
> > const char *argv[], void *aux OVS_UNUSED)
> > @@ -1266,6 +1293,9 @@ dpif_netdev_init(void)
> > "dpif_implementation_name",
> > 1, 1, dpif_netdev_impl_set,
> > NULL);
> > + unixctl_command_register("dpif-netdev/dpif-impl-get", "",
> > + 0, 0, dpif_netdev_impl_get,
> > + NULL);
> > return 0;
> > }
> >
> > --
> > 2.32.0
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> --
> fbl
More information about the dev
mailing list