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.haa...@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.haa...@intel.com> > Co-authored-by: Cian Ferriter <cian.ferri...@intel.com> > Signed-off-by: Cian Ferriter <cian.ferri...@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 > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev -- fbl _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev