Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 16 Nov 2022 12:13:51 +0100
From:   "Jason A. Donenfeld" <Jason@zx2c4.com>
To:     Robert Elliott <elliott@hpe.com>
Cc:     herbert@gondor.apana.org.au, davem@davemloft.net,
        tim.c.chen@linux.intel.com, ap420073@gmail.com, ardb@kernel.org,
        David.Laight@ACULAB.COM, ebiggers@kernel.org,
        linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 10/24] crypto: x86/poly - limit FPU preemption
Message-ID: <Y3TF7/+DejcnN0eV@zx2c4.com>
References: <20221103042740.6556-1-elliott@hpe.com>
 <20221116041342.3841-1-elliott@hpe.com>
 <20221116041342.3841-11-elliott@hpe.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20221116041342.3841-11-elliott@hpe.com>
Precedence: bulk

On Tue, Nov 15, 2022 at 10:13:28PM -0600, Robert Elliott wrote:
> +/* avoid kernel_fpu_begin/end scheduler/rcu stalls */
> +static const unsigned int bytes_per_fpu = 337 * 1024;
> +

Use an enum for constants like this:

    enum { BYTES_PER_FPU = ... };

You can even make it function-local, so it's near the code that uses it,
which will better justify its existence.

Also, where did you get this number? Seems kind of weird.

>  asmlinkage void nh_avx2(const u32 *key, const u8 *message, size_t message_len,
>  			u8 hash[NH_HASH_BYTES]);
>  
> @@ -26,18 +29,20 @@ static void _nh_avx2(const u32 *key, const u8 *message, size_t message_len,
>  static int nhpoly1305_avx2_update(struct shash_desc *desc,
>  				  const u8 *src, unsigned int srclen)
>  {
> +	BUILD_BUG_ON(bytes_per_fpu == 0);

Make the constant function local and remove this check.

> +7
>  	if (srclen < 64 || !crypto_simd_usable())
>  		return crypto_nhpoly1305_update(desc, src, srclen);
>  
> -	do {
> -		unsigned int n = min_t(unsigned int, srclen, SZ_4K);
> +	while (srclen) {

Does this add a needless additional check or does it generate better
code? Would be nice to have some explanation of the rationale.

Same comments as above apply for the rest of this patch ans series.