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/ :|
