[PATCH 1/2] net: add thread-safe crc api

[PATCH 1/2] net: add thread-safe crc api

[Arkadiusz Kusztal]

The current net CRC API is not thread-safe, this patch
solves this by adding another, thread-safe API functions.
These functions are not safe when using between different
processes, though.

Signed-off-by: Arkadiusz Kusztal <arkadiuszx.kusztal@intel.com>
---
 lib/net/rte_net_crc.c | 40 +++++++++++++++++++++++++++++++++++++---
 lib/net/rte_net_crc.h | 14 ++++++++++++++
 lib/net/version.map   |  2 ++
 3 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/lib/net/rte_net_crc.c b/lib/net/rte_net_crc.c
index 346c285c15..87808a31dc 100644
--- a/lib/net/rte_net_crc.c
+++ b/lib/net/rte_net_crc.c
@@ -35,9 +35,6 @@ rte_crc16_ccitt_handler(const uint8_t *data, uint32_t data_len);
 static uint32_t
 rte_crc32_eth_handler(const uint8_t *data, uint32_t data_len);
 
-typedef uint32_t
-(*rte_net_crc_handler)(const uint8_t *data, uint32_t data_len);
-
 static rte_net_crc_handler handlers_default[] = {
 	[RTE_NET_CRC16_CCITT] = rte_crc16_ccitt_default_handler,
 	[RTE_NET_CRC32_ETH] = rte_crc32_eth_default_handler,
@@ -331,6 +328,43 @@ rte_net_crc_calc(const void *data,
 	return ret;
 }
 
+struct rte_net_crc rte_net_crc_set(enum rte_net_crc_type type,
+	enum rte_net_crc_alg alg)
+{
+	const rte_net_crc_handler *handlers = NULL;
+
+	if (max_simd_bitwidth == 0)
+		max_simd_bitwidth = rte_vect_get_max_simd_bitwidth();
+
+	switch (alg) {
+	case RTE_NET_CRC_AVX512:
+		handlers = avx512_vpclmulqdq_get_handlers();
+		if (handlers != NULL)
+			break;
+		/* fall-through */
+	case RTE_NET_CRC_SSE42:
+		handlers = sse42_pclmulqdq_get_handlers();
+		break;
+	case RTE_NET_CRC_NEON:
+		handlers = neon_pmull_get_handlers();
+		/* fall-through */
+	case RTE_NET_CRC_SCALAR:
+		/* fall-through */
+	default:
+		break;
+	}
+	if (handlers == NULL)
+		handlers = handlers_scalar;
+
+	return (struct rte_net_crc){ type, handlers[type] };
+}
+
+uint32_t rte_net_crc(const struct rte_net_crc *ctx,
+	const void *data, const uint32_t data_len)
+{
+	return ctx->crc(data, data_len);
+}
+
 /* Call initialisation helpers for all crc algorithm handlers */
 RTE_INIT(rte_net_crc_init)
 {
diff --git a/lib/net/rte_net_crc.h b/lib/net/rte_net_crc.h
index 72d3e10ff6..f5c8f7173f 100644
--- a/lib/net/rte_net_crc.h
+++ b/lib/net/rte_net_crc.h
@@ -11,6 +11,9 @@
 extern "C" {
 #endif
 
+typedef uint32_t
+(*rte_net_crc_handler)(const uint8_t *data, uint32_t data_len);
+
 /** CRC types */
 enum rte_net_crc_type {
 	RTE_NET_CRC16_CCITT = 0,
@@ -26,6 +29,11 @@ enum rte_net_crc_alg {
 	RTE_NET_CRC_AVX512,
 };
 
+struct rte_net_crc {
+	enum rte_net_crc_type type;
+	rte_net_crc_handler crc;
+};
+
 /**
  * This API set the CRC computation algorithm (i.e. scalar version,
  * x86 64-bit sse4.2 intrinsic version, etc.) and internal data
@@ -59,6 +67,12 @@ rte_net_crc_calc(const void *data,
 	uint32_t data_len,
 	enum rte_net_crc_type type);
 
+struct rte_net_crc rte_net_crc_set(enum rte_net_crc_type type,
+	enum rte_net_crc_alg alg);
+
+uint32_t rte_net_crc(const struct rte_net_crc *ctx,
+	const void *data, const uint32_t data_len);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/net/version.map b/lib/net/version.map
index bec4ce23ea..5c3dbffba7 100644
--- a/lib/net/version.map
+++ b/lib/net/version.map
@@ -4,6 +4,8 @@ DPDK_25 {
 	rte_eth_random_addr;
 	rte_ether_format_addr;
 	rte_ether_unformat_addr;
+	rte_net_crc;
+	rte_net_crc_set;
 	rte_net_crc_calc;
 	rte_net_crc_set_alg;
 	rte_net_get_ptype;
-- 
2.13.6

[PATCH 2/2] crypto/qat: fix thread-safety issue in the crc

[Arkadiusz Kusztal]

This patch fixes CRC thread-safety issue in the QAT PMD.

Signed-off-by: Arkadiusz Kusztal <arkadiuszx.kusztal@intel.com>
---
 drivers/crypto/qat/qat_sym.h         | 3 +--
 drivers/crypto/qat/qat_sym_session.c | 3 +++
 drivers/crypto/qat/qat_sym_session.h | 2 ++
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/qat/qat_sym.h b/drivers/crypto/qat/qat_sym.h
index eedf5de755..8db6044390 100644
--- a/drivers/crypto/qat/qat_sym.h
+++ b/drivers/crypto/qat/qat_sym.h
@@ -291,8 +291,7 @@ qat_crc_generate(struct qat_sym_session *ctx,
 		crc_data = rte_pktmbuf_mtod_offset(sym_op->m_src, uint8_t *,
 				sym_op->auth.data.offset);
 		crc = (uint32_t *)(crc_data + crc_data_len);
-		*crc = rte_net_crc_calc(crc_data, crc_data_len,
-				RTE_NET_CRC32_ETH);
+		*crc = rte_net_crc(&ctx->crc, crc_data, crc_data_len);
 	}
 }
 
diff --git a/drivers/crypto/qat/qat_sym_session.c b/drivers/crypto/qat/qat_sym_session.c
index eb267db424..828466edea 100644
--- a/drivers/crypto/qat/qat_sym_session.c
+++ b/drivers/crypto/qat/qat_sym_session.c
@@ -3176,6 +3176,9 @@ qat_sec_session_set_docsis_parameters(struct rte_cryptodev *dev,
 		ret = qat_sym_session_configure_crc(dev, xform, session);
 		if (ret < 0)
 			return ret;
+	} else {
+		/* Initialize crc algorithm */
+		session->crc = rte_net_crc_set(RTE_NET_CRC32_ETH, RTE_NET_CRC_AVX512);
 	}
 	qat_sym_session_finalize(session);
 
diff --git a/drivers/crypto/qat/qat_sym_session.h b/drivers/crypto/qat/qat_sym_session.h
index f2634774ec..680ac9126c 100644
--- a/drivers/crypto/qat/qat_sym_session.h
+++ b/drivers/crypto/qat/qat_sym_session.h
@@ -7,6 +7,7 @@
 #include <rte_crypto.h>
 #include <cryptodev_pmd.h>
 #include <rte_security.h>
+#include <rte_net_crc.h>
 
 #include "qat_common.h"
 #include "icp_qat_hw.h"
@@ -150,6 +151,7 @@ struct qat_sym_session {
 	uint8_t is_wireless;
 	uint32_t slice_types;
 	enum qat_sym_proto_flag qat_proto_flag;
+	struct rte_net_crc crc;
 	qat_sym_build_request_t build_request[2];
 #ifndef RTE_QAT_OPENSSL
 	IMB_MGR *mb_mgr;
-- 
2.13.6

RE: [EXTERNAL] [PATCH 1/2] net: add thread-safe crc api

[Akhil Goyal]

> The current net CRC API is not thread-safe, this patch
> solves this by adding another, thread-safe API functions.
> These functions are not safe when using between different
> processes, though.
> 
> Signed-off-by: Arkadiusz Kusztal <arkadiuszx.kusztal@intel.com>

Added Jasvinder for review.

This patch is mainly related to net library. Delegated this patchset to Ferruh.

> ---
>  lib/net/rte_net_crc.c | 40 +++++++++++++++++++++++++++++++++++++---
>  lib/net/rte_net_crc.h | 14 ++++++++++++++
>  lib/net/version.map   |  2 ++
>  3 files changed, 53 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/net/rte_net_crc.c b/lib/net/rte_net_crc.c
> index 346c285c15..87808a31dc 100644
> --- a/lib/net/rte_net_crc.c
> +++ b/lib/net/rte_net_crc.c
> @@ -35,9 +35,6 @@ rte_crc16_ccitt_handler(const uint8_t *data, uint32_t
> data_len);
>  static uint32_t
>  rte_crc32_eth_handler(const uint8_t *data, uint32_t data_len);
> 
> -typedef uint32_t
> -(*rte_net_crc_handler)(const uint8_t *data, uint32_t data_len);
> -
>  static rte_net_crc_handler handlers_default[] = {
>  	[RTE_NET_CRC16_CCITT] = rte_crc16_ccitt_default_handler,
>  	[RTE_NET_CRC32_ETH] = rte_crc32_eth_default_handler,
> @@ -331,6 +328,43 @@ rte_net_crc_calc(const void *data,
>  	return ret;
>  }
> 
> +struct rte_net_crc rte_net_crc_set(enum rte_net_crc_type type,
> +	enum rte_net_crc_alg alg)
> +{
> +	const rte_net_crc_handler *handlers = NULL;
> +
> +	if (max_simd_bitwidth == 0)
> +		max_simd_bitwidth = rte_vect_get_max_simd_bitwidth();
> +
> +	switch (alg) {
> +	case RTE_NET_CRC_AVX512:
> +		handlers = avx512_vpclmulqdq_get_handlers();
> +		if (handlers != NULL)
> +			break;
> +		/* fall-through */
> +	case RTE_NET_CRC_SSE42:
> +		handlers = sse42_pclmulqdq_get_handlers();
> +		break;
> +	case RTE_NET_CRC_NEON:
> +		handlers = neon_pmull_get_handlers();
> +		/* fall-through */
> +	case RTE_NET_CRC_SCALAR:
> +		/* fall-through */
> +	default:
> +		break;
> +	}
> +	if (handlers == NULL)
> +		handlers = handlers_scalar;
> +
> +	return (struct rte_net_crc){ type, handlers[type] };
> +}
> +
> +uint32_t rte_net_crc(const struct rte_net_crc *ctx,
> +	const void *data, const uint32_t data_len)
> +{
> +	return ctx->crc(data, data_len);
> +}
> +
>  /* Call initialisation helpers for all crc algorithm handlers */
>  RTE_INIT(rte_net_crc_init)
>  {
> diff --git a/lib/net/rte_net_crc.h b/lib/net/rte_net_crc.h
> index 72d3e10ff6..f5c8f7173f 100644
> --- a/lib/net/rte_net_crc.h
> +++ b/lib/net/rte_net_crc.h
> @@ -11,6 +11,9 @@
>  extern "C" {
>  #endif
> 
> +typedef uint32_t
> +(*rte_net_crc_handler)(const uint8_t *data, uint32_t data_len);
> +
>  /** CRC types */
>  enum rte_net_crc_type {
>  	RTE_NET_CRC16_CCITT = 0,
> @@ -26,6 +29,11 @@ enum rte_net_crc_alg {
>  	RTE_NET_CRC_AVX512,
>  };
> 
> +struct rte_net_crc {
> +	enum rte_net_crc_type type;
> +	rte_net_crc_handler crc;
> +};
> +
>  /**
>   * This API set the CRC computation algorithm (i.e. scalar version,
>   * x86 64-bit sse4.2 intrinsic version, etc.) and internal data
> @@ -59,6 +67,12 @@ rte_net_crc_calc(const void *data,
>  	uint32_t data_len,
>  	enum rte_net_crc_type type);
> 
> +struct rte_net_crc rte_net_crc_set(enum rte_net_crc_type type,
> +	enum rte_net_crc_alg alg);
> +
> +uint32_t rte_net_crc(const struct rte_net_crc *ctx,
> +	const void *data, const uint32_t data_len);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/net/version.map b/lib/net/version.map
> index bec4ce23ea..5c3dbffba7 100644
> --- a/lib/net/version.map
> +++ b/lib/net/version.map
> @@ -4,6 +4,8 @@ DPDK_25 {
>  	rte_eth_random_addr;
>  	rte_ether_format_addr;
>  	rte_ether_unformat_addr;
> +	rte_net_crc;
> +	rte_net_crc_set;
>  	rte_net_crc_calc;
>  	rte_net_crc_set_alg;
>  	rte_net_get_ptype;
> --
> 2.13.6

RE: [EXTERNAL] [PATCH 1/2] net: add thread-safe crc api

[Singh, Jasvinder]

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Wednesday, September 18, 2024 6:58 AM
> To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; dev@dpdk.org;
> Singh, Jasvinder <jasvinder.singh@intel.com>
> Cc: Ji, Kai <kai.ji@intel.com>; Dooley, Brian <brian.dooley@intel.com>; Ferruh
> Yigit <ferruh.yigit@amd.com>
> Subject: RE: [EXTERNAL] [PATCH 1/2] net: add thread-safe crc api
> 
> > The current net CRC API is not thread-safe, this patch solves this by
> > adding another, thread-safe API functions.
> > These functions are not safe when using between different processes,
> > though.
> >
> > Signed-off-by: Arkadiusz Kusztal <arkadiuszx.kusztal@intel.com>
> 
> Added Jasvinder for review.
> 
> This patch is mainly related to net library. Delegated this patchset to Ferruh.
> 
> > ---
> >  lib/net/rte_net_crc.c | 40 +++++++++++++++++++++++++++++++++++++-
> --
> >  lib/net/rte_net_crc.h | 14 ++++++++++++++
> >  lib/net/version.map   |  2 ++
> >  3 files changed, 53 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/net/rte_net_crc.c b/lib/net/rte_net_crc.c index
> > 346c285c15..87808a31dc 100644
> > --- a/lib/net/rte_net_crc.c
> > +++ b/lib/net/rte_net_crc.c
> > @@ -35,9 +35,6 @@ rte_crc16_ccitt_handler(const uint8_t *data,
> > uint32_t data_len);  static uint32_t  rte_crc32_eth_handler(const
> > uint8_t *data, uint32_t data_len);
> >
> > -typedef uint32_t
> > -(*rte_net_crc_handler)(const uint8_t *data, uint32_t data_len);
> > -
> >  static rte_net_crc_handler handlers_default[] = {
> >  	[RTE_NET_CRC16_CCITT] = rte_crc16_ccitt_default_handler,
> >  	[RTE_NET_CRC32_ETH] = rte_crc32_eth_default_handler, @@ -331,6
> > +328,43 @@ rte_net_crc_calc(const void *data,
> >  	return ret;
> >  }
> >
> > +struct rte_net_crc rte_net_crc_set(enum rte_net_crc_type type,
> > +	enum rte_net_crc_alg alg)
> > +{
> > +	const rte_net_crc_handler *handlers = NULL;
> > +
> > +	if (max_simd_bitwidth == 0)
> > +		max_simd_bitwidth = rte_vect_get_max_simd_bitwidth();
> > +
> > +	switch (alg) {
> > +	case RTE_NET_CRC_AVX512:
> > +		handlers = avx512_vpclmulqdq_get_handlers();
> > +		if (handlers != NULL)
> > +			break;
> > +		/* fall-through */
> > +	case RTE_NET_CRC_SSE42:
> > +		handlers = sse42_pclmulqdq_get_handlers();
> > +		break;
> > +	case RTE_NET_CRC_NEON:
> > +		handlers = neon_pmull_get_handlers();
> > +		/* fall-through */
> > +	case RTE_NET_CRC_SCALAR:
> > +		/* fall-through */
> > +	default:
> > +		break;
> > +	}
> > +	if (handlers == NULL)
> > +		handlers = handlers_scalar;
> > +
> > +	return (struct rte_net_crc){ type, handlers[type] }; }
> > +
> > +uint32_t rte_net_crc(const struct rte_net_crc *ctx,
> > +	const void *data, const uint32_t data_len) {
> > +	return ctx->crc(data, data_len);
> > +}
> > +
> >  /* Call initialisation helpers for all crc algorithm handlers */
> >  RTE_INIT(rte_net_crc_init)
> >  {
> > diff --git a/lib/net/rte_net_crc.h b/lib/net/rte_net_crc.h index
> > 72d3e10ff6..f5c8f7173f 100644
> > --- a/lib/net/rte_net_crc.h
> > +++ b/lib/net/rte_net_crc.h
> > @@ -11,6 +11,9 @@
> >  extern "C" {
> >  #endif
> >
> > +typedef uint32_t
> > +(*rte_net_crc_handler)(const uint8_t *data, uint32_t data_len);
> > +
> >  /** CRC types */
> >  enum rte_net_crc_type {
> >  	RTE_NET_CRC16_CCITT = 0,
> > @@ -26,6 +29,11 @@ enum rte_net_crc_alg {
> >  	RTE_NET_CRC_AVX512,
> >  };
> >
> > +struct rte_net_crc {
> > +	enum rte_net_crc_type type;
> > +	rte_net_crc_handler crc;
> > +};
> > +
> >  /**
> >   * This API set the CRC computation algorithm (i.e. scalar version,
> >   * x86 64-bit sse4.2 intrinsic version, etc.) and internal data @@
> > -59,6 +67,12 @@ rte_net_crc_calc(const void *data,
> >  	uint32_t data_len,
> >  	enum rte_net_crc_type type);
> >
> > +struct rte_net_crc rte_net_crc_set(enum rte_net_crc_type type,
> > +	enum rte_net_crc_alg alg);
> > +
> > +uint32_t rte_net_crc(const struct rte_net_crc *ctx,
> > +	const void *data, const uint32_t data_len);
> > +
> >  #ifdef __cplusplus
> >  }
> >  #endif
> > diff --git a/lib/net/version.map b/lib/net/version.map index
> > bec4ce23ea..5c3dbffba7 100644
> > --- a/lib/net/version.map
> > +++ b/lib/net/version.map
> > @@ -4,6 +4,8 @@ DPDK_25 {
> >  	rte_eth_random_addr;
> >  	rte_ether_format_addr;
> >  	rte_ether_unformat_addr;
> > +	rte_net_crc;
> > +	rte_net_crc_set;
> >  	rte_net_crc_calc;
> >  	rte_net_crc_set_alg;
> >  	rte_net_get_ptype;
> > --
> > 2.13.6

Hi Arkadiusz, 

Thanks for the patches. 

New api will make the existing ones obsolete, therefore would suggest removing them to avoid confusion as they implement similar functionality. Also, update the documentation accordingly.