[dpdk-dev] [PATCH v3 01/18] eal: add max SIMD bitwidth

[Coyle, David]

Hi Ciara

> -----Original Message-----
> From: dev <dev-bounces at dpdk.org> On Behalf Of Ciara Power

<snip>

> diff --git a/lib/librte_eal/common/eal_internal_cfg.h
> b/lib/librte_eal/common/eal_internal_cfg.h
> index 13f93388a7..367e0cc19e 100644
> --- a/lib/librte_eal/common/eal_internal_cfg.h
> +++ b/lib/librte_eal/common/eal_internal_cfg.h
> @@ -33,6 +33,12 @@ struct hugepage_info {
>  	int lock_descriptor;    /**< file descriptor for hugepage dir */
>  };
> 
> +struct simd_bitwidth {
> +	/**< flag indicating if bitwidth is locked from further modification */
> +	bool locked;
> +	uint16_t bitwidth; /**< bitwidth value */ };

[DC] The doxygen comment on 'locked' flag uses '/**<' so should come after the field.
Having the comment after the field seems to be the way it's done in this file so I'd move
the comment as opposed to removing the '<'

> +
>  /**
>   * internal configuration
>   */
> @@ -85,6 +91,8 @@ struct internal_config {
>  	volatile unsigned int init_complete;
>  	/**< indicates whether EAL has completed initialization */
>  	unsigned int no_telemetry; /**< true to disable Telemetry */
> +	/** max simd bitwidth path to use */
> +	struct simd_bitwidth max_simd_bitwidth;

[DC] Again the doxygen comments seem to come after the struct fields in this file
so I'd move the comment for max_simd_bitwidth to after it and add the '<'

>  };
> 
>  void eal_reset_internal_config(struct internal_config *internal_cfg); diff --git

<snip>

> 
> diff --git a/lib/librte_eal/include/rte_eal.h b/lib/librte_eal/include/rte_eal.h
> index ddcf6a2e7a..fb739f3474 100644
> --- a/lib/librte_eal/include/rte_eal.h
> +++ b/lib/librte_eal/include/rte_eal.h
> @@ -43,6 +43,14 @@ enum rte_proc_type_t {
>  	RTE_PROC_INVALID
>  };
> 
> +enum rte_max_simd_t {
> +	RTE_NO_SIMD = 64,
> +	RTE_MAX_128_SIMD = 128,
> +	RTE_MAX_256_SIMD = 256,
> +	RTE_MAX_512_SIMD = 512,
> +	RTE_MAX_SIMD_DISABLE = UINT16_MAX,
> +};

[DC] Add doxygen comments on enum rte_max_simd_t and each of it's values

> +
>  /**
>   * Get the process type in a multi-process setup
>   *
> @@ -51,6 +59,31 @@ enum rte_proc_type_t {
>   */
>  enum rte_proc_type_t rte_eal_process_type(void);
> 
> +/**
> + * Get the supported SIMD bitwidth.
> + *
> + * @return
> + *   uint16_t bitwidth.
> + */
> +__rte_experimental
> +uint16_t rte_get_max_simd_bitwidth(void);
> +
> +/**
> + * Set the supported SIMD bitwidth.
> + * This API should only be called once at initialization, before EAL init.
> + *
> + * @param bitwidth
> + *   uint16_t bitwidth.
> + * @return
> + *   0 on success.
> + * @return
> + *   -EINVAL on invalid bitwidth parameter.
> + * @return
> + *   -EPERM if bitwidth is locked.

[DC] Minor thing.. normally there's just 1 @return tag with all of the return values under
it as a bullet list

> + */
> +__rte_experimental
> +int rte_set_max_simd_bitwidth(uint16_t bitwidth);
> +
>  /**
>   * Request iopl privilege for all RPL.
>   *
> diff --git a/lib/librte_eal/rte_eal_version.map
> b/lib/librte_eal/rte_eal_version.map
> index c32461c663..17a7195a3d 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -397,6 +397,10 @@ EXPERIMENTAL {
>  	rte_service_lcore_may_be_active;
>  	rte_thread_register;
>  	rte_thread_unregister;
> +
> +	# added in 20.11
> +	rte_get_max_simd_bitwidth;
> +	rte_set_max_simd_bitwidth;
>  };

[DC] rte_get_max_simd_bitwidth is called from rte_net_crc (and other libraries) so this
symbol possibly needs to be added to librte_eal/rte_eal_exports.def file too.

This is the windows symbol export file, used on windows build.

This has caught us out on the AVX512 CRC patchset https://patchwork.dpdk.org/project/dpdk/list/?series=12596
where a windows build failed in the 'ci/iol-testing' checks in patchwork because
rte_net_crc couldn't find the symbol rte_cpu_get_flag_enabled, which also comes
from rte_eal. We have to add this symbol to rte_eal_exports.def to fix this.

The 'ci/iol-testing' check has not run for your patchset so I can't say for certain if the
windows build would have failed for you, but I think it would

> 
>  INTERNAL {
> --
> 2.17.1