Re: [PATCH 07/29] crypto: skcipher - optimize initializing skcipher_walk fields

[Eric Biggers <ebiggers@kernel.org>]

On Sat, Dec 21, 2024 at 07:16:07PM +0800, Herbert Xu wrote:
> Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > @@ -326,13 +326,13 @@ int skcipher_walk_virt(struct skcipher_walk *walk,
> >                return 0;
> > 
> >        scatterwalk_start(&walk->in, req->src);
> >        scatterwalk_start(&walk->out, req->dst);
> > 
> > -       walk->blocksize = crypto_skcipher_blocksize(tfm);
> > -       walk->ivsize = crypto_skcipher_ivsize(tfm);
> > -       walk->alignmask = crypto_skcipher_alignmask(tfm);
> > +       walk->blocksize = alg->base.cra_blocksize;
> > +       walk->ivsize = alg->co.ivsize;
> > +       walk->alignmask = alg->base.cra_alignmask;
> 
> Please do this instead:
> 
> 	unsigned bs, ivs, am;
> 
> 	bs = crypto_skcipher_blocksize(tfm);
> 	ivs = crypto_skcipher_ivsize(tfm);
> 	am = crypto_skcipher_alignmask(tfm);
> 	walk->blocksize = bs;
> 	walk->ivsize = ivs;
> 	walk->alignmask = am;
> 
> This generates the right thing for me with gcc12.
> 

This seems strictly worse than my version, so I don't plan to do this.  It's
more lines of code, and it causes an extra push and pop to be needed in
skcipher_walk_virt() to free up enough registers to hold all values at once.  It
may be intended that API users are supposed to use the helper functions instead
of accessing the algorithm struct directly, but this code is not a user; it's
part of the API implementation in crypto/skcipher.c.  There are already lots of
other direct accesses to the algorithm struct in the same file, and even another
in skcipher_walk_virt() already.  The helper functions are pointless in this
context and just cause problems like the one this patch is fixing.

- Eric