[ovs-dev,v13,06/12] dpif-netdev: Add command to get dpif implementations.

From: Harry van Haaren <harry.van.haaren@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.

Usage:
 $ ovs-appctl dpif-netdev/dpif-get

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>

---

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       |  8 ++++++++
 lib/dpif-netdev-private-dpif.h       |  6 ++++++
 lib/dpif-netdev-unixctl.man          |  3 +++
 lib/dpif-netdev.c                    | 24 ++++++++++++++++++++++++
 6 files changed, 50 insertions(+)

On Thu, Jun 17, 2021 at 05:18:19PM +0100, Cian Ferriter wrote:
> From: Harry van Haaren <harry.van.haaren@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.
> 
> Usage:
>  $ ovs-appctl dpif-netdev/dpif-get

I didn't mention this in the dpif-set but it would be great
to have a more targeted command name, like dpif-impl-{get,set}.

> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> 
> ---
> 
> 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       |  8 ++++++++
>  lib/dpif-netdev-private-dpif.h       |  6 ++++++
>  lib/dpif-netdev-unixctl.man          |  3 +++
>  lib/dpif-netdev.c                    | 24 ++++++++++++++++++++++++
>  6 files changed, 50 insertions(+)
> 
> diff --git a/Documentation/topics/dpdk/bridge.rst b/Documentation/topics/dpdk/bridge.rst
> index fafa8c821..f59e26cbe 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-get
> +    Available DPIF implementations:
> +      dpif_scalar
> +      dpif_avx512
> +
>  By default, dpif_scalar is used. The DPIF implementation can be selected by
>  name ::
>  
> diff --git a/NEWS b/NEWS
> index 6a4a7b76d..c47ab349e 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -12,6 +12,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 d829a7ee5..3649e775d 100644
> --- a/lib/dpif-netdev-private-dpif.c
> +++ b/lib/dpif-netdev-private-dpif.c
> @@ -73,6 +73,14 @@ dp_netdev_impl_set_default(dp_netdev_input_func func)
>      default_dpif_func = func;
>  }
>  
> +uint32_t
> +dp_netdev_impl_get(const struct dpif_netdev_impl_info_t **out_impls)
> +{
> +    ovs_assert(out_impls);
> +    *out_impls = dpif_impls;
> +    return ARRAY_SIZE(dpif_impls);
> +}
> +

This could receive struct ds and fill with the internal details
to keep internal details in private-dpif.c


>  /* 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 a6db3c7f2..717e9e2f9 100644
> --- a/lib/dpif-netdev-private-dpif.h
> +++ b/lib/dpif-netdev-private-dpif.h
> @@ -48,6 +48,12 @@ 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(const struct dpif_netdev_impl_info_t **out_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-unixctl.man b/lib/dpif-netdev-unixctl.man
> index b348940b0..534823879 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-get\fR
> +Lists the DPIF implementations that are available.
> +.
>  .IP "\fBdpif-netdev/dpif-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 9c234ef3d..59a44a848 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -991,6 +991,27 @@ 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)
> +{
> +    const struct dpif_netdev_impl_info_t *dpif_impls;

then here you initialize 'reply', call dp_netdev_impl_get() and
reply if it succeed or report an error.

Does that make sense?

Thanks,
fbl

> +    uint32_t count = dp_netdev_impl_get(&dpif_impls);
> +    if (count == 0) {
> +        unixctl_command_reply_error(conn, "error getting dpif names");
> +        return;
> +    }
> +
> +    /* Add all dpif functions to reply string. */
> +    struct ds reply = DS_EMPTY_INITIALIZER;
> +    ds_put_cstr(&reply, "Available DPIF implementations:\n");
> +    for (uint32_t i = 0; i < count; i++) {
> +        ds_put_format(&reply, "  %s\n", dpif_impls[i].name);
> +    }
> +    unixctl_command_reply(conn, ds_cstr(&reply));
> +    ds_destroy(&reply);
> +}
> +
>  static void
>  dpif_netdev_impl_set(struct unixctl_conn *conn, int argc,
>                       const char *argv[], void *aux OVS_UNUSED)
> @@ -1292,6 +1313,9 @@ dpif_netdev_init(void)
>                               "dpif_implementation_name [dp]",
>                               1, 2, dpif_netdev_impl_set,
>                               NULL);
> +    unixctl_command_register("dpif-netdev/dpif-get", "",
> +                             0, 0, dpif_netdev_impl_get,
> +                             NULL);
>      return 0;
>  }
>  
> -- 
> 2.32.0
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Hi Flavio,

Thanks for the review. My responses are inline.

Cian

> -----Original Message-----
> From: Flavio Leitner <fbl@sysclose.org>
> Sent: Wednesday 23 June 2021 20:21
> To: Ferriter, Cian <cian.ferriter@intel.com>
> Cc: ovs-dev@openvswitch.org; i.maximets@ovn.org
> Subject: Re: [ovs-dev] [v13 06/12] dpif-netdev: Add command to get dpif implementations.
> 
> On Thu, Jun 17, 2021 at 05:18:19PM +0100, Cian Ferriter wrote:
> > From: Harry van Haaren <harry.van.haaren@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.
> >
> > Usage:
> >  $ ovs-appctl dpif-netdev/dpif-get
> 
> I didn't mention this in the dpif-set but it would be great
> to have a more targeted command name, like dpif-impl-{get,set}.
> 

I'll rename the commands to the above format, thanks for the suggestion.

> >
> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> >
> > ---
> >
> > 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       |  8 ++++++++
> >  lib/dpif-netdev-private-dpif.h       |  6 ++++++
> >  lib/dpif-netdev-unixctl.man          |  3 +++
> >  lib/dpif-netdev.c                    | 24 ++++++++++++++++++++++++
> >  6 files changed, 50 insertions(+)
> >
> > diff --git a/Documentation/topics/dpdk/bridge.rst b/Documentation/topics/dpdk/bridge.rst
> > index fafa8c821..f59e26cbe 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-get
> > +    Available DPIF implementations:
> > +      dpif_scalar
> > +      dpif_avx512
> > +
> >  By default, dpif_scalar is used. The DPIF implementation can be selected by
> >  name ::
> >
> > diff --git a/NEWS b/NEWS
> > index 6a4a7b76d..c47ab349e 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -12,6 +12,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 d829a7ee5..3649e775d 100644
> > --- a/lib/dpif-netdev-private-dpif.c
> > +++ b/lib/dpif-netdev-private-dpif.c
> > @@ -73,6 +73,14 @@ dp_netdev_impl_set_default(dp_netdev_input_func func)
> >      default_dpif_func = func;
> >  }
> >
> > +uint32_t
> > +dp_netdev_impl_get(const struct dpif_netdev_impl_info_t **out_impls)
> > +{
> > +    ovs_assert(out_impls);
> > +    *out_impls = dpif_impls;
> > +    return ARRAY_SIZE(dpif_impls);
> > +}
> > +
> 
> This could receive struct ds and fill with the internal details
> to keep internal details in private-dpif.c
> 
> 

Yes, hiding the details is a good suggestion. I'll fix this.

> >  /* 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 a6db3c7f2..717e9e2f9 100644
> > --- a/lib/dpif-netdev-private-dpif.h
> > +++ b/lib/dpif-netdev-private-dpif.h
> > @@ -48,6 +48,12 @@ 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(const struct dpif_netdev_impl_info_t **out_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-unixctl.man b/lib/dpif-netdev-unixctl.man
> > index b348940b0..534823879 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-get\fR
> > +Lists the DPIF implementations that are available.
> > +.
> >  .IP "\fBdpif-netdev/dpif-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 9c234ef3d..59a44a848 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -991,6 +991,27 @@ 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)
> > +{
> > +    const struct dpif_netdev_impl_info_t *dpif_impls;
> 
> then here you initialize 'reply', call dp_netdev_impl_get() and
> reply if it succeed or report an error.
> 
> Does that make sense?
> 

It makes sense. I'll do that. Thanks for the suggestion!

> Thanks,
> fbl
> 
> > +    uint32_t count = dp_netdev_impl_get(&dpif_impls);
> > +    if (count == 0) {
> > +        unixctl_command_reply_error(conn, "error getting dpif names");
> > +        return;
> > +    }
> > +
> > +    /* Add all dpif functions to reply string. */
> > +    struct ds reply = DS_EMPTY_INITIALIZER;
> > +    ds_put_cstr(&reply, "Available DPIF implementations:\n");
> > +    for (uint32_t i = 0; i < count; i++) {
> > +        ds_put_format(&reply, "  %s\n", dpif_impls[i].name);
> > +    }
> > +    unixctl_command_reply(conn, ds_cstr(&reply));
> > +    ds_destroy(&reply);
> > +}
> > +
> >  static void
> >  dpif_netdev_impl_set(struct unixctl_conn *conn, int argc,
> >                       const char *argv[], void *aux OVS_UNUSED)
> > @@ -1292,6 +1313,9 @@ dpif_netdev_init(void)
> >                               "dpif_implementation_name [dp]",
> >                               1, 2, dpif_netdev_impl_set,
> >                               NULL);
> > +    unixctl_command_register("dpif-netdev/dpif-get", "",
> > +                             0, 0, dpif_netdev_impl_get,
> > +                             NULL);
> >      return 0;
> >  }
> >
> > --
> > 2.32.0
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 
> --
> fbl

On 17 Jun 2021, at 18:18, Cian Ferriter wrote:

> From: Harry van Haaren <harry.van.haaren@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.
>
> Usage:
>  $ ovs-appctl dpif-netdev/dpif-get
>

This command only shows the actual available options, not the currently selected ones.

I think from a support perspective, we need this command to also show the current setting for each datapath. Having this only in the log will not work in cases where the log is overwritten.

//Eelco

Guess this also is true for the MFEX patchset (will reply during my review) and the already included “ovs-appctl dpif-netdev/subtable-lookup-prio-get”

<SNIP>

Hi Eelco,

Thanks for you comment. My reply is inline.

Cian

> -----Original Message-----
> From: Eelco Chaudron <echaudro@redhat.com>
> Sent: Tuesday 29 June 2021 09:25
> To: Ferriter, Cian <cian.ferriter@intel.com>; Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: ovs-dev@openvswitch.org; i.maximets@ovn.org; Flavio Leitner <fbl@sysclose.org>
> Subject: Re: [ovs-dev] [v13 06/12] dpif-netdev: Add command to get dpif implementations.
> 
> 
> 
> On 17 Jun 2021, at 18:18, Cian Ferriter wrote:
> 
> > From: Harry van Haaren <harry.van.haaren@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.
> >
> > Usage:
> >  $ ovs-appctl dpif-netdev/dpif-get
> >
> 
> This command only shows the actual available options, not the currently selected ones.
> 
> I think from a support perspective, we need this command to also show the current setting for each
> datapath. Having this only in the log will not work in cases where the log is overwritten.

Yes, good idea. More visibility here is a good suggestion, rather than fishing through the logs to see what DPIF is set.

> 
> //Eelco
> 
> Guess this also is true for the MFEX patchset (will reply during my review) and the already included
> “ovs-appctl dpif-netdev/subtable-lookup-prio-get”
> 
> <SNIP>


I'll improve the DPIF get command as per your suggestion. I'm following the thread where the DPIF, MFEX and DPCLS get commands are being discussed. I'll take the output of this thread into account for the next revision of the dpif-impl-get command.

Thread:
https://mail.openvswitch.org/pipermail/ovs-dev/2021-June/384644.html