On 13 Jul 2022, at 20:28, Harry van Haaren wrote:
> This commit adds the AVX512 implementation of the
> pop_vlan action.
>
> Signed-off-by: Emma Finn <[email protected]>
>
> ---
>
> v10:
> - Improved ISA checks to fix CI build
> ---
<SNIP>
> +#include <config.h>
> +#include "odp-execute-private.h"
> +/* Function itself is required to be called, even in e.g. 32-bit builds.
> + * This dummy init function ensures 32-bit builds succeed too.
> + */
> +
> +int
> +action_avx512_init(struct odp_execute_action_impl *self OVS_UNUSED)
> +{
> + return 0;
See notes below, on init, but in this case it should return -ENOTSUP;
> +}
> +
> +#endif
> diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
> index 3591da2e5..2fabf6c62 100644
> --- a/lib/odp-execute-private.c
> +++ b/lib/odp-execute-private.c
> @@ -19,6 +19,7 @@
> #include <stdio.h>
> #include <string.h>
>
> +#include "cpu.h"
> #include "dpdk.h"
> #include "dp-packet.h"
> #include "odp-execute-private.h"
> @@ -29,6 +30,36 @@
> VLOG_DEFINE_THIS_MODULE(odp_execute_impl);
> static int active_action_impl_index;
>
> +#if ACTION_IMPL_AVX512_CHECK
> +/* Probe functions to check ISA requirements. */
> +static bool
> +action_avx512_isa_probe(void)
> +{
> + static enum ovs_cpu_isa isa_required[] = {
> + OVS_CPU_ISA_X86_AVX512F,
> + OVS_CPU_ISA_X86_AVX512BW,
> + OVS_CPU_ISA_X86_BMI2,
> + OVS_CPU_ISA_X86_AVX512VL,
> + };
> + for (int i = 0; i < ARRAY_SIZE(isa_required); i++) {
> + if (!cpu_has_isa(isa_required[i])) {
> + return false;
> + }
> + }
> + return true;
> +}
> +static int
> +action_avx512_probe(struct odp_execute_action_impl *self)
> +{
> + if (!action_avx512_isa_probe()) {
> + return -ENOTSUP;
> + } else {
> + action_avx512_init(self);
I do not like this design, see below, but here it should be "return
action_avx512_init(self);".
> + }
> + return 0;
> +}
> +#endif
> +
> static struct odp_execute_action_impl action_impls[] = {
> [ACTION_IMPL_AUTOVALIDATOR] = {
> .available = false,
> @@ -46,7 +77,7 @@ static struct odp_execute_action_impl action_impls[] = {
> [ACTION_IMPL_AVX512] = {
> .available = false,
> .name = "avx512",
> - .init_func = NULL,
> + .init_func = action_avx512_probe,
Do not really understand why the action_avx512_probe() becomes the init
function? The previous design was way more clear, i.e. init_func was
action_avx512_init(), and that function would call action_avx512_isa_probe().
I guess it's because of the patch done by David, but maybe we can still do
something like this (not tested at all):
diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
index 2fabf6c62..373c90ea3 100644
--- a/lib/odp-execute-private.c
+++ b/lib/odp-execute-private.c
@@ -49,14 +49,11 @@ action_avx512_isa_probe(void)
return true;
}
static int
-action_avx512_probe(struct odp_execute_action_impl *self)
+#else
+static bool
+action_avx512_isa_probe(void)
{
- if (!action_avx512_isa_probe()) {
- return -ENOTSUP;
- } else {
- action_avx512_init(self);
- }
- return 0;
+ retrun false;
}
#endif
@@ -77,7 +74,7 @@ static struct odp_execute_action_impl action_impls[] = {
[ACTION_IMPL_AVX512] = {
.available = false,
.name = "avx512",
- .init_func = action_avx512_probe,
+ .init_func = action_avx512_init,
},
#endif
};
And just export the action_avx512_isa_probe() function.
> },
> #endif
> };
> diff --git a/lib/odp-execute-private.h b/lib/odp-execute-private.h
> index 1943eb600..f66e6e6d1 100644
> --- a/lib/odp-execute-private.h
> +++ b/lib/odp-execute-private.h
> @@ -96,6 +96,8 @@ struct odp_execute_action_impl *
> odp_execute_action_set(const char *name);
>
> int action_autoval_init(struct odp_execute_action_impl *self);
>
> +int action_avx512_init(struct odp_execute_action_impl *self);
> +
> void odp_execute_action_get_info(struct ds *name);
>
> #endif /* ODP_EXTRACT_PRIVATE */
> --
> 2.32.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
