Re: [PATCH 2/10] crypto: aead - Count error stats differently

[Eric Biggers <ebiggers@kernel.org>]

On Wed, Feb 15, 2023 at 05:25:09PM +0800, Herbert Xu wrote:
>  int crypto_aead_encrypt(struct aead_request *req)
>  {
>  	struct crypto_aead *aead = crypto_aead_reqtfm(req);
> -	struct crypto_alg *alg = aead->base.__crt_alg;
> +	struct aead_alg *alg = crypto_aead_alg(aead);
>  	unsigned int cryptlen = req->cryptlen;

The cryptlen local variable is no longer needed.  Just use req->cryptlen below.

> +	struct crypto_istat_aead *istat;
>  	int ret;
>  
> -	crypto_stats_get(alg);
> +	istat = aead_get_stat(alg);
> +
> +	if (IS_ENABLED(CONFIG_CRYPTO_STATS)) {
> +		atomic64_inc(&istat->encrypt_cnt);
> +		atomic64_add(cryptlen, &istat->encrypt_tlen);
> +	}
> +

This could just check whether istat is NULL:

	istat = aead_get_stat(alg);
	if (istat) {
		atomic64_inc(&istat->encrypt_cnt);
		atomic64_add(req->cryptlen, &istat->encrypt_tlen);
	}

That's simpler, and it makes it clearer that the pointer is not dereferenced
when it is NULL.

Note that aead_get_stat() is an inline function, so the stats code will still be
optimized out when !CONFIG_CRYPTO_STATS.

> +static inline int crypto_aead_errstat(struct crypto_istat_aead *istat, int err)
> +{
> +	if (!IS_ENABLED(CONFIG_CRYPTO_STATS))
> +		return err;
> +
> +	if (err && err != -EINPROGRESS && err != -EBUSY)
> +		atomic64_inc(&istat->err_cnt);
> +
> +	return err;
> +}
> +
[...]
>  	if (crypto_aead_get_flags(aead) & CRYPTO_TFM_NEED_KEY)
>  		ret = -ENOKEY;
>  	else
> -		ret = crypto_aead_alg(aead)->encrypt(req);
> -	crypto_stats_aead_encrypt(cryptlen, alg, ret);
> -	return ret;
> +		ret = alg->encrypt(req);
> +
> +	return crypto_aead_errstat(istat, ret);

Similarly, istat != NULL could be used instead of CONFIG_CRYPTO_STATS.

IMO, this would also be easier to read if the stats increment was just coded
directly, like it is above, without the crypto_aead_errstat() function:

	if (crypto_aead_get_flags(aead) & CRYPTO_TFM_NEED_KEY)
		ret = -ENOKEY;
	else
		ret = alg->encrypt(req);

	if (istat && ret && ret != -EINPROGRESS && ret != -EBUSY)
		atomic64_inc(&istat->err_cnt);

	return ret;

Similarly for all the other algorithm types.

- Eric