From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id AD5F7A0093; Tue, 3 May 2022 16:33:46 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5E90840C35; Tue, 3 May 2022 16:33:46 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id B7BFD40691 for ; Tue, 3 May 2022 16:33:44 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1651588401; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=sXwKAzFbZVggeONiA0MbGorszhAdgiqpjnz+1Xh7xb0=; b=FQIRQ59j4N411B6sDLoxqP4BZt1sEo74/XFKT88sLFOOwu84/EEe7CzpGpJhCAUMAe9YLG iagNyKBhsooES56fYikuxIzzEjA4HiZL1zRF6kLblddZ6iGZ3Q0LiYZNJLWnD85qB2Usb7 2qp00MMOA9JlRKApMNsy7t4izd72Hec= Received: from mail-lj1-f200.google.com (mail-lj1-f200.google.com [209.85.208.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-587-xqhyRyqgNlex-2cGpRxw1w-1; Tue, 03 May 2022 10:33:17 -0400 X-MC-Unique: xqhyRyqgNlex-2cGpRxw1w-1 Received: by mail-lj1-f200.google.com with SMTP id l16-20020a2e5710000000b0024f0c34eff1so4298476ljb.10 for ; Tue, 03 May 2022 07:33:17 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=sXwKAzFbZVggeONiA0MbGorszhAdgiqpjnz+1Xh7xb0=; b=zeP6RwptTrh62xtukFKbwoZGHf0Mj/ZjoXhL/5v3IF/BcD/eBclatD/AeyXfQMpQE+ xel7ml2iAFys6DNyS/ZeSLw/MB3z08dGSM9aJcaEYE2bevWnMtjDprp4KxdiaImZ4/QY PyWa+IpG2uoz3q6L2ur9IRTPW7JINVs1lnXvSH/2lvaElLqWXR1RazksyjxnExhKyFTH M2srrKivKLtJCaovfGq1nHyigUD916AwLI1bJM+Oev//0AgF8n9aJtbzZ1dLFnUbJ5U+ jcsIKILBtKFQ2D0fAwa6P3i/x45SBXRn2z7jz49WdVJtMnk1fVVJqX+vtWFhE0MWiEna 0lsA== X-Gm-Message-State: AOAM5332HIbrHaD27belj+EzBdr8UQOEh2XKst1p0mhfTiqxUpwy1WWX 40aJVwQUrbRgxnveI9Gwr3eWldL65Efy2sy7lcUsu3oDaRTIeg+vsYM1/i868wPcswzc1n9hCBq nTimrarB1VPTEmnSRvEY= X-Received: by 2002:a05:6512:3185:b0:472:10b4:34e3 with SMTP id i5-20020a056512318500b0047210b434e3mr11470932lfe.217.1651588394631; Tue, 03 May 2022 07:33:14 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxArjSYmy+azouLUqbHPGeHCZuHXwZ8Arm8EV4RP9bT8+itw6u3BmCph29NJlgob1gsQxCEEqjZBOexsy+dVQY= X-Received: by 2002:a05:6512:3185:b0:472:10b4:34e3 with SMTP id i5-20020a056512318500b0047210b434e3mr11470914lfe.217.1651588394300; Tue, 03 May 2022 07:33:14 -0700 (PDT) MIME-Version: 1.0 References: <20220427152232.19223-1-pbhagavatula@marvell.com> <20220429161700.2145-1-pbhagavatula@marvell.com> <20220429161700.2145-2-pbhagavatula@marvell.com> In-Reply-To: <20220429161700.2145-2-pbhagavatula@marvell.com> From: David Marchand Date: Tue, 3 May 2022 16:33:03 +0200 Message-ID: Subject: Re: [PATCH v8 2/2] hash: unify crc32 selection for x86 and Arm To: Pavan Nikhilesh Cc: "Ruifeng Wang (Arm Technology China)" , Yipeng Wang , Sameh Gobriel , Bruce Richardson , Vladimir Medvedkin , Jerin Jacob Kollanukkaran , dev Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dmarchan@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Fri, Apr 29, 2022 at 6:17 PM Pavan Nikhilesh wrote: > > Merge crc32 hash calculation public API implementation for x86 and Arm. > Select the best available CRC32 algorithm when unsupported algorithm > on a given CPU architecture is requested by an application. > > Previously, if an application directly includes `rte_crc_arm64.h` > without including `rte_hash_crc.h` it will fail to compile. On this topic, a followup patch could add a check to those internal headers, to ensure _RTE_HASH_CRC_H_ is defined and #error otherwise. Like what is done in lib/eal/x86/include/rte_atomic_64.h, for example. > > Signed-off-by: Pavan Nikhilesh > Reviewed-by: Ruifeng Wang > --- > lib/hash/meson.build | 1 + > lib/hash/rte_crc_arm64.h | 69 +++--------------- > lib/hash/rte_crc_generic.h | 72 ++++++++++++++++++ > lib/hash/rte_crc_x86.h | 89 +++++++++++++++++++++++ > lib/hash/rte_hash_crc.h | 145 ++++++++++--------------------------- > 5 files changed, 210 insertions(+), 166 deletions(-) > create mode 100644 lib/hash/rte_crc_generic.h > > diff --git a/lib/hash/meson.build b/lib/hash/meson.build > index ff13e2c7f9..b36c8b0c01 100644 > --- a/lib/hash/meson.build > +++ b/lib/hash/meson.build > @@ -13,6 +13,7 @@ indirect_headers += files( > 'rte_crc_sw.h', > 'rte_crc_x86.h', > 'rte_crc_arm64.h', > + 'rte_crc_generic.h', > 'rte_thash_x86_gfni.h', > ) > > diff --git a/lib/hash/rte_crc_arm64.h b/lib/hash/rte_crc_arm64.h > index b4628cfc09..172894335f 100644 > --- a/lib/hash/rte_crc_arm64.h > +++ b/lib/hash/rte_crc_arm64.h > @@ -2,23 +2,8 @@ > * Copyright(c) 2015 Cavium, Inc > */ > > -#ifndef _RTE_CRC_ARM64_H_ > -#define _RTE_CRC_ARM64_H_ > - > -/** > - * @file > - * > - * RTE CRC arm64 Hash > - */ > - > -#ifdef __cplusplus > -extern "C" { > -#endif > - > -#include > -#include > -#include > -#include > +#ifndef _HASH_CRC_ARM64_H_ > +#define _HASH_CRC_ARM64_H_ > > static inline uint32_t > crc32c_arm64_u8(uint8_t data, uint32_t init_val) > @@ -61,40 +46,8 @@ crc32c_arm64_u64(uint64_t data, uint32_t init_val) > } > > /** > - * Allow or disallow use of arm64 SIMD instrinsics for CRC32 hash > - * calculation. > - * > - * @param alg > - * An OR of following flags: > - * - (CRC32_SW) Don't use arm64 crc intrinsics > - * - (CRC32_ARM64) Use ARMv8 CRC intrinsic if available > - * > - */ > -static inline void > -rte_hash_crc_set_alg(uint8_t alg) > -{ > - switch (alg) { > - case CRC32_ARM64: > - if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_CRC32)) > - alg = CRC32_SW; > - /* fall-through */ > - case CRC32_SW: > - crc32_alg = alg; > - /* fall-through */ > - default: > - break; > - } > -} > - > -/* Setting the best available algorithm */ > -RTE_INIT(rte_hash_crc_init_alg) > -{ > - rte_hash_crc_set_alg(CRC32_ARM64); > -} > - > -/** > - * Use single crc32 instruction to perform a hash on a 1 byte value. > - * Fall back to software crc32 implementation in case arm64 crc intrinsics is > + * Use single crc32 instruction to perform a hash on a byte value. > + * Fall back to software crc32 implementation in case ARM CRC is > * not supported > * > * @param data > @@ -115,7 +68,7 @@ rte_hash_crc_1byte(uint8_t data, uint32_t init_val) > > /** > * Use single crc32 instruction to perform a hash on a 2 bytes value. > - * Fall back to software crc32 implementation in case arm64 crc intrinsics is > + * Fall back to software crc32 implementation in case ARM CRC is > * not supported > * > * @param data > @@ -136,7 +89,7 @@ rte_hash_crc_2byte(uint16_t data, uint32_t init_val) > > /** > * Use single crc32 instruction to perform a hash on a 4 byte value. > - * Fall back to software crc32 implementation in case arm64 crc intrinsics is > + * Fall back to software crc32 implementation in case ARM CRC is > * not supported > * > * @param data > @@ -157,7 +110,7 @@ rte_hash_crc_4byte(uint32_t data, uint32_t init_val) > > /** > * Use single crc32 instruction to perform a hash on a 8 byte value. > - * Fall back to software crc32 implementation in case arm64 crc intrinsics is > + * Fall back to software crc32 implementation in case ARM CRC is > * not supported > * > * @param data > @@ -170,14 +123,10 @@ rte_hash_crc_4byte(uint32_t data, uint32_t init_val) > static inline uint32_t > rte_hash_crc_8byte(uint64_t data, uint32_t init_val) > { > - if (likely(crc32_alg == CRC32_ARM64)) > + if (likely(crc32_alg & CRC32_ARM64)) > return crc32c_arm64_u64(data, init_val); > > return crc32c_2words(data, init_val); > } > > -#ifdef __cplusplus > -} > -#endif > - > -#endif /* _RTE_CRC_ARM64_H_ */ > +#endif /* _HASH_CRC_ARM64_H_ */ > diff --git a/lib/hash/rte_crc_generic.h b/lib/hash/rte_crc_generic.h > new file mode 100644 > index 0000000000..0c55947896 > --- /dev/null > +++ b/lib/hash/rte_crc_generic.h > @@ -0,0 +1,72 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2022 Marvell. > + */ > + > +#ifndef _HASH_CRC_GENERIC_H_ > +#define _HASH_CRC_GENERIC_H_ > + > +/** > + * Software crc32 implementation for 1 byte value. > + * > + * @param data > + * Data to perform hash on. > + * @param init_val > + * Value to initialise hash generator. > + * @return > + * 32bit calculated hash value. > + */ > +static inline uint32_t > +rte_hash_crc_1byte(uint8_t data, uint32_t init_val) > +{ > + return crc32c_1byte(data, init_val); > +} > + > +/** > + * Software crc32 implementation for 2 byte value. > + * > + * @param data > + * Data to perform hash on. > + * @param init_val > + * Value to initialise hash generator. > + * @return > + * 32bit calculated hash value. > + */ > +static inline uint32_t > +rte_hash_crc_2byte(uint16_t data, uint32_t init_val) > +{ > + return crc32c_2bytes(data, init_val); > +} > + > +/** > + * Software crc32 implementation for 4 byte value. > + * > + * @param data > + * Data to perform hash on. > + * @param init_val > + * Value to initialise hash generator. > + * @return > + * 32bit calculated hash value. > + */ > +static inline uint32_t > +rte_hash_crc_4byte(uint32_t data, uint32_t init_val) > +{ > + return crc32c_1word(data, init_val); > +} > + > +/** > + * Software crc32 implementation for 8 byte value. > + * > + * @param data > + * Data to perform hash on. > + * @param init_val > + * Value to initialise hash generator. > + * @return > + * 32bit calculated hash value. > + */ > +static inline uint32_t > +rte_hash_crc_8byte(uint64_t data, uint32_t init_val) > +{ > + return crc32c_2words(data, init_val); > +} > + > +#endif > diff --git a/lib/hash/rte_crc_x86.h b/lib/hash/rte_crc_x86.h > index b80a742afa..19eb3584e7 100644 > --- a/lib/hash/rte_crc_x86.h > +++ b/lib/hash/rte_crc_x86.h > @@ -59,4 +59,93 @@ crc32c_sse42_u64(uint64_t data, uint64_t init_val) > return (uint32_t)init_val; > } > > +/** > + * Use single crc32 instruction to perform a hash on a byte value. > + * Fall back to software crc32 implementation in case SSE4.2 is > + * not supported > + * > + * @param data > + * Data to perform hash on. > + * @param init_val > + * Value to initialise hash generator. > + * @return > + * 32bit calculated hash value. > + */ > +static inline uint32_t > +rte_hash_crc_1byte(uint8_t data, uint32_t init_val) > +{ > + if (likely(crc32_alg & CRC32_SSE42)) > + return crc32c_sse42_u8(data, init_val); > + > + return crc32c_1byte(data, init_val); > +} > + > +/** > + * Use single crc32 instruction to perform a hash on a 2 bytes value. > + * Fall back to software crc32 implementation in case SSE4.2 is > + * not supported > + * > + * @param data > + * Data to perform hash on. > + * @param init_val > + * Value to initialise hash generator. > + * @return > + * 32bit calculated hash value. > + */ > +static inline uint32_t > +rte_hash_crc_2byte(uint16_t data, uint32_t init_val) > +{ > + if (likely(crc32_alg & CRC32_SSE42)) > + return crc32c_sse42_u16(data, init_val); > + > + return crc32c_2bytes(data, init_val); > +} > + > +/** > + * Use single crc32 instruction to perform a hash on a 4 byte value. > + * Fall back to software crc32 implementation in case SSE4.2 is > + * not supported > + * > + * @param data > + * Data to perform hash on. > + * @param init_val > + * Value to initialise hash generator. > + * @return > + * 32bit calculated hash value. > + */ > +static inline uint32_t > +rte_hash_crc_4byte(uint32_t data, uint32_t init_val) > +{ > + if (likely(crc32_alg & CRC32_SSE42)) > + return crc32c_sse42_u32(data, init_val); > + > + return crc32c_1word(data, init_val); > +} > + > +/** > + * Use single crc32 instruction to perform a hash on a 8 byte value. > + * Fall back to software crc32 implementation in case SSE4.2 is > + * not supported > + * > + * @param data > + * Data to perform hash on. > + * @param init_val > + * Value to initialise hash generator. > + * @return > + * 32bit calculated hash value. > + */ > +static inline uint32_t > +rte_hash_crc_8byte(uint64_t data, uint32_t init_val) > +{ > +#ifdef RTE_ARCH_X86_64 > + if (likely(crc32_alg == CRC32_SSE42_x64)) > + return crc32c_sse42_u64(data, init_val); > +#endif > + > + if (likely(crc32_alg & CRC32_SSE42)) > + return crc32c_sse42_u64_mimic(data, init_val); > + > + return crc32c_2words(data, init_val); > +} > + > #endif > diff --git a/lib/hash/rte_hash_crc.h b/lib/hash/rte_hash_crc.h > index 308fdde414..cd9ec17082 100644 > --- a/lib/hash/rte_hash_crc.h > +++ b/lib/hash/rte_hash_crc.h > @@ -16,10 +16,12 @@ extern "C" { > #endif > > #include > -#include > -#include > + > #include > #include > +#include > +#include > +#include > > #include "rte_crc_sw.h" > > @@ -33,136 +35,67 @@ static uint8_t crc32_alg = CRC32_SW; > > #if defined(RTE_ARCH_ARM64) && defined(__ARM_FEATURE_CRC32) > #include "rte_crc_arm64.h" > -#else > +#elif defined(RTE_ARCH_X86) > #include "rte_crc_x86.h" > +#else > +#include "rte_crc_generic.h" > +#endif > > /** > - * Allow or disallow use of SSE4.2 instrinsics for CRC32 hash > + * Allow or disallow use of SSE4.2/ARMv8 intrinsics for CRC32 hash > * calculation. > * > * @param alg > * An OR of following flags: > - * - (CRC32_SW) Don't use SSE4.2 intrinsics > + * - (CRC32_SW) Don't use SSE4.2/ARMv8 intrinsics (default non-[x86/ARMv8]) > * - (CRC32_SSE42) Use SSE4.2 intrinsics if available > - * - (CRC32_SSE42_x64) Use 64-bit SSE4.2 intrinsic if available (default) > + * - (CRC32_SSE42_x64) Use 64-bit SSE4.2 intrinsic if available (default x86) > + * - (CRC32_ARM64) Use ARMv8 CRC intrinsic if available (default ARMv8) > * > */ > static inline void > rte_hash_crc_set_alg(uint8_t alg) > { > -#if defined(RTE_ARCH_X86) > - if (alg == CRC32_SSE42_x64 && > - !rte_cpu_get_flag_enabled(RTE_CPUFLAG_EM64T)) > - alg = CRC32_SSE42; > -#endif > - crc32_alg = alg; > -} > + crc32_alg = CRC32_SW; > > -/* Setting the best available algorithm */ > -RTE_INIT(rte_hash_crc_init_alg) > -{ > - rte_hash_crc_set_alg(CRC32_SSE42_x64); > -} > + if (alg == CRC32_SW) > + return; > > -/** > - * Use single crc32 instruction to perform a hash on a byte value. > - * Fall back to software crc32 implementation in case SSE4.2 is > - * not supported > - * > - * @param data > - * Data to perform hash on. > - * @param init_val > - * Value to initialise hash generator. > - * @return > - * 32bit calculated hash value. > - */ > -static inline uint32_t > -rte_hash_crc_1byte(uint8_t data, uint32_t init_val) By moving all these sw helpers into a new file, the rendered documentation loses their descriptions. We should keep a declaration of each function of the API under a #ifdef __DOXYGEN__ in rte_hash_crc.h. If there is no objection, I'll fix by keeping only the generic description of each function, like shown below: diff --git a/lib/hash/rte_hash_crc.h b/lib/hash/rte_hash_crc.h index cd9ec17082..86c341a7a4 100644 --- a/lib/hash/rte_hash_crc.h +++ b/lib/hash/rte_hash_crc.h @@ -96,6 +96,62 @@ RTE_INIT(rte_hash_crc_init_alg) #endif } +#ifdef __DOXYGEN__ + +/** + * Use single crc32 instruction to perform a hash on a byte value. + * + * @param data + * Data to perform hash on. + * @param init_val + * Value to initialise hash generator. + * @return + * 32bit calculated hash value. + */ +static inline uint32_t +rte_hash_crc_1byte(uint8_t data, uint32_t init_val); + [snip] +static inline uint32_t +rte_hash_crc_8byte(uint64_t data, uint32_t init_val); + +#endif /* __DOXYGEN__ */ + /** * Calculate CRC32 hash on user-supplied byte array. * -- David Marchand