Hi Flavio, Thanks for the review. My responses are inline.
Cian > -----Original Message----- > From: Flavio Leitner <[email protected]> > Sent: Wednesday 23 June 2021 20:21 > To: Ferriter, Cian <[email protected]> > Cc: [email protected]; [email protected] > 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 <[email protected]> > > > > 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 <[email protected]> > > > > --- > > > > 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 > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > -- > fbl _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
