Hi Amber,
Couple of comments/suggestions inline.
> -----Original Message-----
> From: dev <ovs-dev-boun...@openvswitch.org> On Behalf Of Kumar Amber
> Sent: Friday, August 26, 2022 5:01 AM
> To: ovs-dev@openvswitch.org
> Cc: i.maxim...@ovn.org; f...@sysclose.org; Amber, Kumar
> <kumar.am...@intel.com>
> Subject: [ovs-dev] [PATCH v5 5/9] dpif-netdev: Add function pointer for
> dpif re-circulate.
>
> The patch adds and re-uses the dpif set command to set the function
> pointers to be used to switch between different inner dpifs.
>
I think the commit message could be reworded to something like:
This patch adds support for selecting the recirculation implementation based on
the DPIF implementation.
WDYT ?
> Signed-off-by: Kumar Amber <kumar.am...@intel.com>
> Signed-off-by: Cian Ferriter <cian.ferri...@intel.com>
> Co-authored-by: Cian Ferriter <cian.ferri...@intel.com>
>
> ---
> v3:
> - Add description to the dpif recirc function.
> - Fix use of return value to fall back to scalar dpif.
> ---
> ---
> lib/dpif-netdev-private-dpif.c | 53 +++++++++++++++++++++++++++-----
> lib/dpif-netdev-private-dpif.h | 18 +++++++++++
> lib/dpif-netdev-private-thread.h | 3 ++
> lib/dpif-netdev.c | 14 ++++++++-
> 4 files changed, 80 insertions(+), 8 deletions(-)
>
> diff --git a/lib/dpif-netdev-private-dpif.c b/lib/dpif-netdev-private-
> dpif.c index 07039b1c2..bf1131c5e 100644
> --- a/lib/dpif-netdev-private-dpif.c
> +++ b/lib/dpif-netdev-private-dpif.c
> @@ -55,18 +55,21 @@ dp_netdev_input_avx512_probe(void)
> static struct dpif_netdev_impl_info_t dpif_impls[] = {
> /* The default scalar C code implementation. */
> [DPIF_NETDEV_IMPL_SCALAR] = { .input_func = dp_netdev_input,
> + .recirc_func = dp_netdev_recirculate,
> .probe = NULL,
> .name = "dpif_scalar", },
>
> #if DPIF_NETDEV_IMPL_AVX512_CHECK
> /* Only available on x86_64 bit builds with SSE 4.2 used for OVS
> core. */
> [DPIF_NETDEV_IMPL_AVX512] = { .input_func = dp_netdev_input_avx512,
> + .recirc_func = dp_netdev_input_avx512_recirc,
> .probe = dp_netdev_input_avx512_probe,
> .name = "dpif_avx512", },
> #endif
> };
>
> static dp_netdev_input_func default_dpif_func;
> +static dp_netdev_recirc_func default_dpif_recirc_func;
>
> dp_netdev_input_func
> dp_netdev_impl_get_default(void)
> @@ -97,6 +100,35 @@ dp_netdev_impl_get_default(void)
> return default_dpif_func;
> }
>
> +dp_netdev_recirc_func
> +dp_netdev_recirc_impl_get_default(void)
> +{
> + /* For the first call, this will be NULL. Compute the compile time
> default.
> + */
> + if (!default_dpif_recirc_func) {
> + int dpif_idx = DPIF_NETDEV_IMPL_SCALAR;
> +
> +/* Configure-time overriding to run test suite on all implementations.
> +*/ #if DPIF_NETDEV_IMPL_AVX512_CHECK #ifdef DPIF_AVX512_DEFAULT
> + dp_netdev_input_func_probe probe;
> +
> + /* Check if the compiled default is compatible. */
> + probe = dpif_impls[DPIF_NETDEV_IMPL_AVX512].probe;
> + if (!probe || !probe()) {
> + dpif_idx = DPIF_NETDEV_IMPL_AVX512;
> + }
> +#endif
> +#endif
> +
> + VLOG_INFO("Default re-circulate DPIF implementation is %s.\n",
> + dpif_impls[dpif_idx].name);
> + default_dpif_recirc_func = dpif_impls[dpif_idx].recirc_func;
> + }
> +
> + return default_dpif_recirc_func;
> +}
> +
This function seems very identical to dp_netdev_impl_get_default. I wonder if
we can reuse this function instead or have the common parts moved to a separate
func ?
<snipped>
Thanks and regards
Sunil
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
