Re: [Qemu-devel] Question about add AF_ALG backend for virtio-crypto

["Daniel P. Berrange" <berrange@redhat.com>]

On Thu, Feb 09, 2017 at 10:58:55AM +0800, Longpeng (Mike) wrote:
> Hi Daniel,
> 
> On 2017/2/8 18:53, Daniel P. Berrange wrote:
> 
> > On Wed, Feb 08, 2017 at 06:46:04PM +0800, Longpeng (Mike) wrote:
> >> Hi Daniel,
> >>
> >> I was writing AF_ALG-backed for QEMU crypto these days, I think there're more
> >> than two ways to implements it.
> >>
> >> The first one look likes below:
> >> [ cipher.c ]
> >> qcrypto_cipher_new(...)
> >> {
> >> 	if (...) { /* use AF_ALG */
> >> 		cipher = afalg_cipher_new(...)
> >> 		if (cipher) {
> >> 			return cipher;
> >> 		}
> >> 	}
> >> 	
> >> 	/* disabled AF_ALG or AF_ALG failed, then back to
> >> 	 * using 'builtin'(gcrypt/nettle/...)
> >> 	 */
> >> 	cipher = __qcrypto_cipher_new(...)
> >> }
> >>
> >> [ cipher-afalg.c ]
> >> afalg_cipher_new(...) {....}
> >> afalg_cipher_encrypt(...) {...}
> >> ......
> >>
> >> [ cipher-gcrypt.c ]
> >> __qcrypto_cipher_new(...) {...}
> >> __qcrypto_cipher_encrypt(...) {...}
> >> ......
> >>
> >> [ cipher-nettle.c ]
> >> __qcrypto_cipher_new(...) {...}
> >> __qcrypto_cipher_encrypt(...) {...}
> >> ......
> >>
> >> In this way, I think I need to rename most functions in
> >> cipher-gcrypt.c/cipher-nettle.c with a prefixion(such as '__')
> >>
> 
> 
> >> I'm confusing about which way you'd prefer, or do you have any better
> >> suggestion?
> > 
> > Yeah, both approaches have some reasonably significant downsides. Approach
> > 1 is sort of like providing a virtual driver table, except it is hardcoded
> > to switch between 2 impls only.
> > 
> > A variant on approach 1 is to actually setup a proper driver-table dispatch
> > layer. eg define a struct that contains callbacks for each public api
> > operation. The qcrypto_cipher_new() method will then either setup callbacks
> > for AF_ALG, or for the library impl.
> > 
> > This is the design we took in crypto/{ivgen.c,ivgenpriv.h}
> > 
> 
> 
> So...you prefer approach 1 with a driver-table dispatch layer, right?
> And this implies that we must either rename some public methods in
> cipher-gcrypt.c/cipher-nettle.c, or change them to 'static'.

I'd suggest both - renaming them to have 'gcrypt' or 'nettle' in their
name, and also make them static.

> I also have some other ideas:
> 
> 1) *using bitmap to improve performance*
> 
> As you suggested before:
> "if we had AF_ALG in QEMU, we would have to have a stacked impl, where
> we try AF_ALG and then fallback to the current code when QEMU runs on a
> kernel lacking the feature needed."
> 
> I think it would impact the performance if we "try AF_ALG and then fallback to
> library" each time, so we can use a bitmap to indicate whether the @alg is
> supported by AF_ALG.

Yep, remembering the decision makes total sense if that is a high overhead
decision.

> 2) *maybe we need a heuristic policy*
> 
> I added some speed test in test-crypto-cipher/hash and found that for big
> packets AF_ALG is much faster than library-impl while library-impl is better
> when the packets is small:
>
> packet(bytes)	AF_ALG(MB/sec, intel QAT)	Library-impl(MB/sec)
> 512		53.68				127.82
> 1024		98.39				133.21
> 2048		167.56				134.62
> 4096		276.21				135.10
> 8192		410.80				135.82
> 16384		545.08				136.01
> 32768		654.49				136.30
> 65536		723.00				136.29
> 
> If a @alg is both supported by AF_ALG and library-impl, I think we should decide
> to use which one dynamically.

What exactly are you measuring here?

Is this comparing encryption of a fixed total size of data, and
varying the packet size. ie sending 1024 * 512 byte packets against
256  * 2048 byte packages.

Or is it sending a constant number of packets eg 1024 * 512 byte
packets against 1024 * 2048 byte packets ?

The problem is that when constructing the cipher initially, we have no
about the intended usage pattern, so can't decide which impl to use as
is


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|