[ovs-dev] [v14 06/11] dpif-netdev: Add command to get dpif implementations.
Flavio Leitner
fbl at sysclose.org
Wed Jul 7 17:28:36 UTC 2021
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?
> + 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.
> + 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
> +
> + 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