Re: [PATCH] cryptoloop

[Christoph Hellwig]

On Sat, Jul 05, 2003 at 11:41:45AM +0300, Jari Ruusu wrote:
> I haven't seen the modified transfer function yet, so no test data on speed
> difference yet. Notice that I said "tiny speed degration". Tiny in this
> context may well mean unmeasurable small. However, it will add mode code to
> optimized implementation. More code == tiny bit slower.

Then I'd suggest reading the suggestion carefully :)  It suggest to:

a) not perform that kmap/kunmap in loop.c before calling the transfer
   function and
b) pass the page frame + offset to the transfer function instead of the
   kernel virtual address

That means for the module implementing the transfer function either

1) a superflous kmap that it doesn't need is gone (cryptoapi case)
2) it needs to perform the kmap previously done by itself. (= no change)
3) it needs to perform the kmap itself but can use kmap_atomic now

> Loop code in loop-AES does not have any significant speed advantage over
> mainline loop. Most significant advantage that loop-AES' loop code has over
> mainline, is that it includes a ton of bug fixes still missing from mainline
> loop.

Well, we already had a discussion about that, and you haven't tried to
split your patch up so it's unacceptable for mainline.  I'm in fact pretty
sure some parts like the bio reservations are buggy but it's hard to tell
excatly with a that big patch.

> Loop-AES' speed advantage comes from optimized AES-only transfer
> function that does the CBC stuff and directly calls highly optimized AES
> implementation without any CryptoAPI overhead.

That's the good old VFS argument.  Of course we could rip out the
VFS and go directly to ext2 but in many cases abstraction have more
advantage than the tiny speed loss they impare.

> This tests only low level cipher functions aes_encrypt() and aes_decrypt()
> from linux-2.5.74/crypto/aes.c with all CryptoAPI overhead removed. In real
> use, including CryptoAPI overhead, these numbers should be a little bit
> smaller.

<snip>

The number is interesting.  It might be worthwile to try embedding your
aes implementations into the CryptoAPI.  Given your negative comments
I suppose you are not interested in something like that?

> I have posted patches to be included in mainline. Fixes are available, and
> if they are not merged, then so be it. If fixes are not merged to mainline,
> they will be maintained outside of mainline so people who need them can
> actually use them. It is not really my failt that mainline people seem to
> prefer buggy loop and slow loop crypto.

So far all loop bugs that have been reported during 2.5 have been fixed.
As for slow crypto:  yes, we prefer the cryptoapi abstraction over
directly going to a single algorithm.  And no one here said he diskliked
your AES implementation, they just haven't been offered yet.  I'm sure
your assembly implementation will be merged once the framework for
optimized crypto algorithms is in place and I'm also pretty sure
we'll find out why your C implementation is faster and either switch the
current implementation for yours or improve it.  It's just that all this
would be a lot easier if you just offered them in a form of a nicely
described do one thing patch instead of moaning from the backside..