Re: [ovs-dev] [PATCH v3 4/5] dpif-netdev: Add function pointer for dpif re-circulate.

2022-07-05 Thread Van Haaren, Harry
> -Original Message-
> From: Amber, Kumar 
> Sent: Tuesday, June 28, 2022 9:45 AM
> To: ovs-dev@openvswitch.org
> Cc: echau...@redhat.com; i.maxim...@ovn.org; Ferriter, Cian
> ; Stokes, Ian ; 
> f...@sysclose.org;
> Van Haaren, Harry ; Amber, Kumar
> 
> Subject: [PATCH v3 4/5] 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.
> 
> Signed-off-by: Kumar Amber 
> Signed-off-by: Cian Ferriter 
> Co-authored-by: Cian Ferriter 
> 
> ---
> 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   | 57 +++-
>  lib/dpif-netdev-private-dpif.h   | 18 ++
>  lib/dpif-netdev-private-thread.h |  3 ++
>  lib/dpif-netdev.c| 19 +--
>  4 files changed, 87 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/dpif-netdev-private-dpif.c b/lib/dpif-netdev-private-dpif.c
> index 2dc51270a..96bfd4824 100644
> --- a/lib/dpif-netdev-private-dpif.c
> +++ b/lib/dpif-netdev-private-dpif.c
> @@ -27,6 +27,8 @@
>  #include "util.h"
> 
>  VLOG_DEFINE_THIS_MODULE(dpif_netdev_impl);
> +#define DPIF_NETDEV_IMPL_AVX512_CHECK (__x86_64__ && HAVE_AVX512F \
> +&& HAVE_LD_AVX512_GOOD && __SSE4_2__)

This #define rework should be in a different patch, it is not related to "add 
func ptr for dpif recirc".




> +/* Returns the default recirculate DPIF which is first ./configure selected,
> + * but can be overridden at runtime. */
> +dp_netdev_recirc_func dp_netdev_recirc_impl_get_default(void);

Simplify comment?
/* Returns the default implementation to recirculate packets. */

The wording today suggests the compile time ./configure selected can be 
overridden...
which reads funny and isn't technically true. Simply state what the func does 
instead.



> @@ -8819,7 +8828,10 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
> *packets_,
>  }
> 
>  (*depth)++;
> -dp_netdev_recirculate(pmd, packets_);
> +int ret = pmd->netdev_input_recirc_func(pmd, packets_);
> +if (ret != 0) {
> +dp_netdev_recirculate(pmd, packets_);
> +}

Prefer the simpler if() format below?

if (ret) {

exists another time below.

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


[ovs-dev] [PATCH v3 4/5] dpif-netdev: Add function pointer for dpif re-circulate.

2022-06-28 Thread Kumar Amber
The patch adds and re-uses the dpif set command to set the
function pointers to be used to switch between different inner
dpifs.

Signed-off-by: Kumar Amber 
Signed-off-by: Cian Ferriter 
Co-authored-by: Cian Ferriter 

---
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   | 57 +++-
 lib/dpif-netdev-private-dpif.h   | 18 ++
 lib/dpif-netdev-private-thread.h |  3 ++
 lib/dpif-netdev.c| 19 +--
 4 files changed, 87 insertions(+), 10 deletions(-)

diff --git a/lib/dpif-netdev-private-dpif.c b/lib/dpif-netdev-private-dpif.c
index 2dc51270a..96bfd4824 100644
--- a/lib/dpif-netdev-private-dpif.c
+++ b/lib/dpif-netdev-private-dpif.c
@@ -27,6 +27,8 @@
 #include "util.h"
 
 VLOG_DEFINE_THIS_MODULE(dpif_netdev_impl);
+#define DPIF_NETDEV_IMPL_AVX512_CHECK (__x86_64__ && HAVE_AVX512F \
+&& HAVE_LD_AVX512_GOOD && __SSE4_2__)
 
 DEFINE_EXTERN_PER_THREAD_DATA(recirc_depth, 0);
 
@@ -39,18 +41,21 @@ enum dpif_netdev_impl_info_idx {
 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 (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__)
 /* 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)
@@ -61,7 +66,7 @@ dp_netdev_impl_get_default(void)
 int dpif_idx = DPIF_NETDEV_IMPL_SCALAR;
 
 /* Configure-time overriding to run test suite on all implementations. */
-#if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__)
+#if DPIF_NETDEV_IMPL_AVX512_CHECK
 #ifdef DPIF_AVX512_DEFAULT
 dp_netdev_input_func_probe probe;
 
@@ -81,6 +86,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;
+}
+
 void
 dp_netdev_impl_get(struct ds *reply, struct dp_netdev_pmd_thread **pmd_list,
size_t n)
@@ -116,10 +150,12 @@ dp_netdev_impl_get(struct ds *reply, struct 
dp_netdev_pmd_thread **pmd_list,
  * returns the function pointer to the one requested by "name".
  */
 static int32_t
-dp_netdev_impl_get_by_name(const char *name, dp_netdev_input_func *out_func)
+dp_netdev_impl_get_by_name(const char *name, dp_netdev_input_func *dpif_func,
+   dp_netdev_recirc_func *dpif_recirc_func)
 {
 ovs_assert(name);
-ovs_assert(out_func);
+ovs_assert(dpif_func);
+ovs_assert(dpif_recirc_func);
 
 uint32_t i;
 
@@ -129,11 +165,13 @@ dp_netdev_impl_get_by_name(const char *name, 
dp_netdev_input_func *out_func)
 if (dpif_impls[i].probe) {
 int probe_err = dpif_impls[i].probe();
 if (probe_err) {
-*out_func = NULL;
+*dpif_func = NULL;
+*dpif_recirc_func = NULL;
 return probe_err;
 }
 }
-*out_func = dpif_impls[i].input_func;
+*dpif_func = dpif_impls[i].input_func;
+*dpif_recirc_func = dpif_impls[i].recirc_func;
 return 0;
 }
 }
@@ -144,12 +182,15 @@ dp_netdev_impl_get_by_name(const char *name, 
dp_netdev_input_func *out_func)
 int32_t
 dp_netdev_impl_set_default_by_name(const char *name)
 {
-dp_netdev_input_func new_default;
+dp_netdev_input_func new_dpif_default;
+dp_netdev_recirc_func new_dpif_recirc_default;
 
-int32_t err = dp_netdev_impl_get_by_name(name, _default);
+int32_t err = dp_netdev_impl_get_by_name(name, _dpif_default,
+