Hi Sunil,
I will take all the comments in V6
Regards
Amber
> -----Original Message-----
> From: Pai G, Sunil <sunil.pa...@intel.com>
> Sent: Tuesday, October 4, 2022 7:50 PM
> To: Amber, Kumar <kumar.am...@intel.com>; ovs-dev@openvswitch.org
> Cc: i.maxim...@ovn.org; f...@sysclose.org; Amber, Kumar
> <kumar.am...@intel.com>
> Subject: RE: [ovs-dev] [PATCH v5 5/9] dpif-netdev: Add function pointer for
> dpif re-circulate.
>
> 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
