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

[Eric Biggers <ebiggers@kernel.org>]

On Thu, Feb 16, 2023 at 02:00:20PM +0800, Herbert Xu wrote:
> > > +	if (IS_ENABLED(CONFIG_CRYPTO_STATS))
> > > +		memset(istat, 0, sizeof(*istat));
> > > +
> > 
> > Above is another place that can just do 'if (istat)'.
> 
> As I mentioned before, this would create an unnecessary branch
> when STATS are enabled (which would presumably be the common case).
> 

I was hoping the compiler would know the pointer is non-NULL, since it's created
through an expression like &foo->bar where bar is at nonzero offset, and foo is
also dereferenced.  Unfortunately it does seem that's not the case, though,
probably because of some of the compiler flags the kernel is compiled with
(-fno-strict-aliasing and -fno-delete-null-pointer-checks, maybe?).

Anyway, if CONFIG_CRYPTO_STATS=y is the common case, that's unfortunate.  Surely
hardly anyone actually uses the feature, and all this stats collection for every
crypto operation is for nothing?

Here's a thread where someone claimed that disabling CONFIG_CRYPTO_STATS
significantly improves performance:
https://lists.ceph.io/hyperkitty/list/ceph-users@ceph.io/thread/44GMO5UGOXDZKFSOQMCPPHYTREUEA3ZI/

IMO this feature should never have been accepted.  But could we at least put the
stats collection behind a static branch that defaults to off?  If someone really
wants to collect stats, they can set a sysctl that turns on the static branch.

- Eric