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

["Kusztal, ArkadiuszX" <arkadiuszx.kusztal@intel.com>]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, October 2, 2024 9:42 AM
> To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>
> Cc: dev@dpdk.org; ferruh.yigit@amd.com; Ji, Kai <kai.ji@intel.com>; Dooley,
> Brian <brian.dooley@intel.com>
> Subject: Re: [PATCH v2 1/3] net: add thread-safe crc api
> 
> 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?

In this case both.

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

In the current one, there exists a race condition that causes multi-threaded applications to crash; currently only QAT PMD is using it.
But I know it is late for such a change, so I probably should defer it. We can manage to fix it in a different way.

> 
> 
> > ---
> >  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.
> 
So there must have been a misunderstanding on my part, the initial idea was to prevent the user from calling these functions.

> 
> --
> David Marchand