Re: [PATCH v2 1/3] net: add thread-safe crc api

[David Marchand <david.marchand@redhat.com>]

On Tue, Oct 1, 2024 at 9:27 PM Arkadiusz Kusztal
<arkadiuszx.kusztal@intel.com> wrote:
>
> The current net CRC API is not thread-safe, this patch
> solves this by adding another, thread-safe API functions.
> This API is also safe to use across multiple processes,
> yet with limitations on max-simd-bitwidth, which will be checked only by
> the process that created the CRC context; all other processes will use
> the same CRC function when used with the same CRC context.
> It is an undefined behavior when process binaries are compiled
> with different SIMD capabilities when the same CRC context is used.
>
> Signed-off-by: Arkadiusz Kusztal <arkadiuszx.kusztal@intel.com>

I am lost... do you mean thread-safe ? or DPDK multi process safe?

For now, I don't see why we need a new API (and especially why we
should break the existing one..).


> ---
>  lib/net/net_crc.h     |  19 ++--
>  lib/net/rte_net_crc.c | 309 +++++++++++++++-----------------------------------
>  lib/net/rte_net_crc.h |  40 ++-----
>  lib/net/version.map   |  18 ++-
>  4 files changed, 124 insertions(+), 262 deletions(-)
>
> diff --git a/lib/net/net_crc.h b/lib/net/net_crc.h
> index 7a74d5406c..d220200685 100644
> --- a/lib/net/net_crc.h
> +++ b/lib/net/net_crc.h
> @@ -5,40 +5,41 @@
>  #ifndef _NET_CRC_H_
>  #define _NET_CRC_H_
>
> -/*
> - * Different implementations of CRC
> - */
> -
> -/* SSE4.2 */
> +#include <rte_compat.h>
>
> +__rte_internal
>  void
>  rte_net_crc_sse42_init(void);
>
> +__rte_internal
>  uint32_t
>  rte_crc16_ccitt_sse42_handler(const uint8_t *data, uint32_t data_len);
>
> +__rte_internal
>  uint32_t
>  rte_crc32_eth_sse42_handler(const uint8_t *data, uint32_t data_len);
>
> -/* AVX512 */
> -
> +__rte_internal
>  void
>  rte_net_crc_avx512_init(void);
>
> +__rte_internal
>  uint32_t
>  rte_crc16_ccitt_avx512_handler(const uint8_t *data, uint32_t data_len);
>
> +__rte_internal
>  uint32_t
>  rte_crc32_eth_avx512_handler(const uint8_t *data, uint32_t data_len);
>
> -/* NEON */
> -
> +__rte_internal
>  void
>  rte_net_crc_neon_init(void);
>
> +__rte_internal
>  uint32_t
>  rte_crc16_ccitt_neon_handler(const uint8_t *data, uint32_t data_len);
>
> +__rte_internal
>  uint32_t
>  rte_crc32_eth_neon_handler(const uint8_t *data, uint32_t data_len);

Exporting internals but not using them out of the library makes no sense.


-- 
David Marchand