Re: [PATCH v2 05/11] lib/crc64: add support for arch-optimized implementations

[Ard Biesheuvel]

On Thu, 30 Jan 2025 at 04:54, Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
>
> From: Eric Biggers <ebiggers@xxxxxxxxxx>
>
> Add support for architecture-optimized implementations of the CRC64
> library functions, following the approach taken for the CRC32 and
> CRC-T10DIF library functions.
>
> Also take the opportunity to tweak the function prototypes:
> - Use 'const void *' for the lib entry points (since this is easier for
>   users) but 'const u8 *' for the underlying arch and generic functions
>   (since this is easier for the implementations of these functions).
> - Don't bother with __pure.  It's an unusual optimization that doesn't
>   help properly written code.  It's a weird quirk we can do without.
>
> Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx>
> ---
>  include/linux/crc64.h | 26 ++++++++++++++++++++++----
>  lib/Kconfig           |  7 +++++++
>  lib/crc64.c           | 36 ++++++++----------------------------
>  3 files changed, 37 insertions(+), 32 deletions(-)
>

Reviewed-by: Ard Biesheuvel <ardb@xxxxxxxxxx>

> diff --git a/include/linux/crc64.h b/include/linux/crc64.h
> index 17cf5af3e78e..41de30b907df 100644
> --- a/include/linux/crc64.h
> +++ b/include/linux/crc64.h
> @@ -5,12 +5,28 @@
>  #ifndef _LINUX_CRC64_H
>  #define _LINUX_CRC64_H
>
>  #include <linux/types.h>
>
> -u64 __pure crc64_be(u64 crc, const void *p, size_t len);
> -u64 __pure crc64_nvme_generic(u64 crc, const void *p, size_t len);
> +u64 crc64_be_arch(u64 crc, const u8 *p, size_t len);
> +u64 crc64_be_generic(u64 crc, const u8 *p, size_t len);
> +u64 crc64_nvme_arch(u64 crc, const u8 *p, size_t len);
> +u64 crc64_nvme_generic(u64 crc, const u8 *p, size_t len);
> +
> +/**
> + * crc64_be - Calculate bitwise big-endian ECMA-182 CRC64
> + * @crc: seed value for computation. 0 or (u64)~0 for a new CRC calculation,
> + *       or the previous crc64 value if computing incrementally.
> + * @p: pointer to buffer over which CRC64 is run
> + * @len: length of buffer @p
> + */
> +static inline u64 crc64_be(u64 crc, const void *p, size_t len)
> +{
> +       if (IS_ENABLED(CONFIG_CRC64_ARCH))
> +               return crc64_be_arch(crc, p, len);
> +       return crc64_be_generic(crc, p, len);
> +}
>
>  /**
>   * crc64_nvme - Calculate CRC64-NVME
>   * @crc: seed value for computation. 0 for a new CRC calculation, or the
>   *      previous crc64 value if computing incrementally.
> @@ -18,11 +34,13 @@ u64 __pure crc64_nvme_generic(u64 crc, const void *p, size_t len);
>   * @len: length of buffer @p
>   *
>   * This computes the CRC64 defined in the NVME NVM Command Set Specification,
>   * *including the bitwise inversion at the beginning and end*.
>   */
> -static inline u64 crc64_nvme(u64 crc, const u8 *p, size_t len)
> +static inline u64 crc64_nvme(u64 crc, const void *p, size_t len)
>  {
> -       return crc64_nvme_generic(crc, p, len);
> +       if (IS_ENABLED(CONFIG_CRC64_ARCH))
> +               return ~crc64_nvme_arch(~crc, p, len);
> +       return ~crc64_nvme_generic(~crc, p, len);
>  }
>
>  #endif /* _LINUX_CRC64_H */
> diff --git a/lib/Kconfig b/lib/Kconfig
> index da07fd39cf97..67bbf4f64dd9 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -199,10 +199,17 @@ config CRC64
>           This option is provided for the case where no in-kernel-tree
>           modules require CRC64 functions, but a module built outside
>           the kernel tree does. Such modules that use library CRC64
>           functions require M here.
>
> +config ARCH_HAS_CRC64
> +       bool
> +
> +config CRC64_ARCH
> +       tristate
> +       default CRC64 if ARCH_HAS_CRC64 && CRC_OPTIMIZATIONS
> +
>  config CRC4
>         tristate "CRC4 functions"
>         help
>           This option is provided for the case where no in-kernel-tree
>           modules require CRC4 functions, but a module built outside
> diff --git a/lib/crc64.c b/lib/crc64.c
> index d6f3f245eede..5b1b17057f0a 100644
> --- a/lib/crc64.c
> +++ b/lib/crc64.c
> @@ -39,40 +39,20 @@
>  #include "crc64table.h"
>
>  MODULE_DESCRIPTION("CRC64 calculations");
>  MODULE_LICENSE("GPL v2");
>
> -/**
> - * crc64_be - Calculate bitwise big-endian ECMA-182 CRC64
> - * @crc: seed value for computation. 0 or (u64)~0 for a new CRC calculation,
> - *       or the previous crc64 value if computing incrementally.
> - * @p: pointer to buffer over which CRC64 is run
> - * @len: length of buffer @p
> - */
> -u64 __pure crc64_be(u64 crc, const void *p, size_t len)
> +u64 crc64_be_generic(u64 crc, const u8 *p, size_t len)
>  {
> -       size_t i, t;
> -
> -       const unsigned char *_p = p;
> -
> -       for (i = 0; i < len; i++) {
> -               t = ((crc >> 56) ^ (*_p++)) & 0xFF;
> -               crc = crc64table[t] ^ (crc << 8);
> -       }
> -
> +       while (len--)
> +               crc = (crc << 8) ^ crc64table[(crc >> 56) ^ *p++];
>         return crc;
>  }
> -EXPORT_SYMBOL_GPL(crc64_be);
> +EXPORT_SYMBOL_GPL(crc64_be_generic);
>
> -u64 __pure crc64_nvme_generic(u64 crc, const void *p, size_t len)
> +u64 crc64_nvme_generic(u64 crc, const u8 *p, size_t len)
>  {
> -       const unsigned char *_p = p;
> -       size_t i;
> -
> -       crc = ~crc;
> -
> -       for (i = 0; i < len; i++)
> -               crc = (crc >> 8) ^ crc64nvmetable[(crc & 0xff) ^ *_p++];
> -
> -       return ~crc;
> +       while (len--)
> +               crc = (crc >> 8) ^ crc64nvmetable[(crc & 0xff) ^ *p++];
> +       return crc;
>  }
>  EXPORT_SYMBOL_GPL(crc64_nvme_generic);
> --
> 2.48.1
>