[dpdk-dev] [PATCH v3 17/18] net: add checks for max SIMD bitwidth

[Coyle, David]

Hi Ciara,

> From: dev <dev-bounces at dpdk.org> On Behalf Of Ciara Power
> When choosing a vector path to take, an extra condition must be satisfied to
> ensure the max SIMD bitwidth allows for the CPU enabled path.
> 
> The vector path was initially chosen in RTE_INIT, however this is no longer
> suitable as we cannot check the max SIMD bitwidth at that time.
> The default chosen in RTE_INIT is now scalar. For best performance and to
> use vector paths, apps must explicitly call the set algorithm function before
> using other functions from this library, as this is where vector handlers are
> now chosen.

[DC] Has it been decided that it is ok to now require applications to pick the
CRC algorithm they want to use?

An application which previously automatically got SSE4.2 CRC, for example, will
now automatically only get scalar.

If this is ok, this should probably be called out explicitly in release notes as it may
not be Immediately noticeable to users that they now need to select the CRC algo.

Actually, in general, the release notes need to be updated for this patchset.

> 
> Suggested-by: Jasvinder Singh <jasvinder.singh at intel.com>
> 
> Signed-off-by: Ciara Power <ciara.power at intel.com>
> 
> ---
> v3:
>   - Moved choosing vector paths out of RTE_INIT.
>   - Moved checking max_simd_bitwidth into the set_alg function.
> ---
>  lib/librte_net/rte_net_crc.c | 26 +++++++++++++++++---------
> lib/librte_net/rte_net_crc.h |  3 ++-
>  2 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/librte_net/rte_net_crc.c b/lib/librte_net/rte_net_crc.c index
> 9fd4794a9d..241eb16399 100644
> --- a/lib/librte_net/rte_net_crc.c
> +++ b/lib/librte_net/rte_net_crc.c

<snip>

> @@ -145,18 +149,26 @@ rte_crc32_eth_handler(const uint8_t *data,
> uint32_t data_len)  void  rte_net_crc_set_alg(enum rte_net_crc_alg alg)  {
> +	if (max_simd_bitwidth == 0)
> +		max_simd_bitwidth = rte_get_max_simd_bitwidth();
> +
>  	switch (alg) {
>  #ifdef X86_64_SSE42_PCLMULQDQ
>  	case RTE_NET_CRC_SSE42:
> -		handlers = handlers_sse42;
> -		break;
> +		if (max_simd_bitwidth >= RTE_MAX_128_SIMD) {
> +			handlers = handlers_sse42;
> +			return;
> +		}
> +		RTE_LOG(INFO, NET, "Max SIMD Bitwidth too low, using
> scalar\n");

[DC] Not sure if you're aware but there is another patchset which adds an AVX512 CRC
implementation and run-time checking of cpuflags to select the CRC path to use:
https://patchwork.dpdk.org/project/dpdk/list/?series=12596

There will be a task to merge these 2 patchsets if both are merged. It looks fairly
straightforward to me to merge these, but it would be good if you take a look too