[openssl.org #1974] [PATCH 12/14] Config option for cryptodev on other OS's (+ hash)

[Stephen Henson via RT]

> [David_Mccullough@... - Tue Jun 30 13:55:14 2009]:
>
>
> Add --with-cryptodev option to config so that an OS like linux may enable
> cryptodev support if it has been ported (ie., ocf-linux).
>
> Add --with-cryptodev-digests to optionally include hash support via
> cryptodev (disabled by default as it is usually too slow to be useful).
>
> Add a working cryptodev hash implementation.
>
> Fix up RSA API compliance for rsa_nocrt_mod_exp method while here.
>
> Overview :

http://www.mail-archive.com/openssl-dev@.../msg26096.html
>
>
>

The equivalent to the two configure options can be done by including
-Dfoo to Configure, for example Configure [options] -DHAVE_CRYPTODEV .

This patch also does considerably more than the above description,
adding new symmetric algorithms, changing the behaviour of existing code
etc. This should preferably be split into smaller patches which can be
more easily analysed with full descriptions of the reasoning for changes.






______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       openssl-dev@...
Automated List Manager                           majordomo@...

[Stephen Henson via RT]

Jivin Stephen Henson via RT lays it down ...

> > [David_Mccullough@... - Tue Jun 30 13:55:14 2009]:
> >
> >
> > Add --with-cryptodev option to config so that an OS like linux may enable
> > cryptodev support if it has been ported (ie., ocf-linux).
> >
> > Add --with-cryptodev-digests to optionally include hash support via
> > cryptodev (disabled by default as it is usually too slow to be useful).
> >
> > Add a working cryptodev hash implementation.
> >
> > Fix up RSA API compliance for rsa_nocrt_mod_exp method while here.
> >
> > Overview :
> http://www.mail-archive.com/openssl-dev@.../msg26096.html
>
> The equivalent to the two configure options can be done by including
> -Dfoo to Configure, for example Configure [options] -DHAVE_CRYPTODEV .

Ok,  thats works for me if it's the preferred solution.

> This patch also does considerably more than the above description,
> adding new symmetric algorithms, changing the behaviour of existing code
> etc. This should preferably be split into smaller patches which can be
> more easily analysed with full descriptions of the reasoning for changes.

Sorry about that, I'll sync up with whatever changes have gone in and
generate new patches for the cryptodev engine related bits.

Should they appear in new "rt" tickets of their own or keep it within this
one ?

Thanks,
Davidm

--
David McCullough,  david_mccullough@...,  Ph:[phone]
McAfee - SnapGear  http://www.snapgear.com                http://www.uCdot.org


______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       openssl-dev@...
Automated List Manager                           majordomo@...

[David McCullough-3]

Jivin Stephen Henson via RT lays it down ...

> > [David_Mccullough@... - Tue Jun 30 13:55:14 2009]:
> >
> >
> > Add --with-cryptodev option to config so that an OS like linux may enable
> > cryptodev support if it has been ported (ie., ocf-linux).
> >
> > Add --with-cryptodev-digests to optionally include hash support via
> > cryptodev (disabled by default as it is usually too slow to be useful).
> >
> > Add a working cryptodev hash implementation.
> >
> > Fix up RSA API compliance for rsa_nocrt_mod_exp method while here.
> >
> > Overview :
> http://www.mail-archive.com/openssl-dev@.../msg26096.html
>
> The equivalent to the two configure options can be done by including
> -Dfoo to Configure, for example Configure [options] -DHAVE_CRYPTODEV .

Ok,  thats works for me if it's the preferred solution.

> This patch also does considerably more than the above description,
> adding new symmetric algorithms, changing the behaviour of existing code
> etc. This should preferably be split into smaller patches which can be
> more easily analysed with full descriptions of the reasoning for changes.

Sorry about that, I'll sync up with whatever changes have gone in and
generate new patches for the cryptodev engine related bits.

Should they appear in new "rt" tickets of their own or keep it within this
one ?

Thanks,
Davidm

--
David McCullough,  david_mccullough@...,  Ph:[phone]
McAfee - SnapGear  http://www.snapgear.com                http://www.uCdot.org
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       openssl-dev@...
Automated List Manager                           majordomo@...

[Stephen Henson via RT]

> [David_Mccullough@... - Wed Jul 01 05:41:07 2009]:
>
>
> Jivin Stephen Henson via RT lays it down ...
> >
> > The equivalent to the two configure options can be done by including
> > -Dfoo to Configure, for example Configure [options] -DHAVE_CRYPTODEV
> .
>
> Ok,  thats works for me if it's the preferred solution.
>

OK, that has been added.

It's unfortunate that there is a need to enable digests separately
because it is possible at the ENGINE level to selectively enable
algorithm implementations. Presumably this is to cover cases where
applications just blindly use all algorithms and have no option to avoid
using digests.

> > This patch also does considerably more than the above description,
> > adding new symmetric algorithms, changing the behaviour of existing
> code
> > etc. This should preferably be split into smaller patches which can
> be
> > more easily analysed with full descriptions of the reasoning for
> changes.
>
> Sorry about that, I'll sync up with whatever changes have gone in and
> generate new patches for the cryptodev engine related bits.
>
> Should they appear in new "rt" tickets of their own or keep it within
> this
> one ?
>

I'll apply some of the bits where I know what they do. The main bit I
wasn't sure about was the reason for a change to a static fd in
get_dev_crypto(): there was no comment in the ticket of the patch as to
why this was done.

Steve.
--
Dr Stephen N. Henson. OpenSSL project core developer.
Commercial tech support now available see: http://www.openssl.org

______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       openssl-dev@...
Automated List Manager                           majordomo@...

[Stephen Henson via RT]

Jivin Stephen Henson via RT lays it down ...

> > [David_Mccullough@... - Wed Jul 01 05:41:07 2009]:
> >
> >
> > Jivin Stephen Henson via RT lays it down ...
> > >
> > > The equivalent to the two configure options can be done by including
> > > -Dfoo to Configure, for example Configure [options] -DHAVE_CRYPTODEV
> > .
> >
> > Ok,  thats works for me if it's the preferred solution.
> >
>
> OK, that has been added.
>
> It's unfortunate that there is a need to enable digests separately
> because it is possible at the ENGINE level to selectively enable
> algorithm implementations. Presumably this is to cover cases where
> applications just blindly use all algorithms and have no option to avoid
> using digests.

Maybe there is a better way, I am not and openssl expert thats for sure :-)

The reason the option exists is that in all but the most unusual cases,
using cryptodev hashing via some HW device is not worth it by any measure.
There are some people using it,  but they have specific applications/needs.

I wasn't aware of a way to allow an engine to selectively support different
combinations of hashes and ciphers, but if there is by all means point me at
it and I will see what I can do to remove that ifdef.

> > > This patch also does considerably more than the above description,
> > > adding new symmetric algorithms, changing the behaviour of existing
> > code
> > > etc. This should preferably be split into smaller patches which can
> > be
> > > more easily analysed with full descriptions of the reasoning for
> > changes.
> >
> > Sorry about that, I'll sync up with whatever changes have gone in and
> > generate new patches for the cryptodev engine related bits.
> >
> > Should they appear in new "rt" tickets of their own or keep it within
> > this
> > one ?
>
> I'll apply some of the bits where I know what they do. The main bit I
> wasn't sure about was the reason for a change to a static fd in
> get_dev_crypto(): there was no comment in the ticket of the patch as to
> why this was done.

No problems,  I have the log here,  it's just with version changes over
the years the code/reasons get moved around.  The log for that change:

        Clean up the cryptodev handling to not burn file descriptors or open the
        main channel more than needed.

But looking at the code I can't see how it achieves that.  I know that
ocf-linux had a lot of kernel issues in this area in the early days and
I can only guess this is somehow a remanent of working around that,
sorry for the noise :-(

Short answer, drop the fd changes in the patch.

Thanks,
Davidm

--
David McCullough,  david_mccullough@...,  Ph:[phone]
McAfee - SnapGear  http://www.snapgear.com                http://www.uCdot.org


______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       openssl-dev@...
Automated List Manager                           majordomo@...

[David McCullough-3]

Jivin Stephen Henson via RT lays it down ...

> > [David_Mccullough@... - Wed Jul 01 05:41:07 2009]:
> >
> >
> > Jivin Stephen Henson via RT lays it down ...
> > >
> > > The equivalent to the two configure options can be done by including
> > > -Dfoo to Configure, for example Configure [options] -DHAVE_CRYPTODEV
> > .
> >
> > Ok,  thats works for me if it's the preferred solution.
> >
>
> OK, that has been added.
>
> It's unfortunate that there is a need to enable digests separately
> because it is possible at the ENGINE level to selectively enable
> algorithm implementations. Presumably this is to cover cases where
> applications just blindly use all algorithms and have no option to avoid
> using digests.

Maybe there is a better way, I am not and openssl expert thats for sure :-)

The reason the option exists is that in all but the most unusual cases,
using cryptodev hashing via some HW device is not worth it by any measure.
There are some people using it,  but they have specific applications/needs.

I wasn't aware of a way to allow an engine to selectively support different
combinations of hashes and ciphers, but if there is by all means point me at
it and I will see what I can do to remove that ifdef.

> > > This patch also does considerably more than the above description,
> > > adding new symmetric algorithms, changing the behaviour of existing
> > code
> > > etc. This should preferably be split into smaller patches which can
> > be
> > > more easily analysed with full descriptions of the reasoning for
> > changes.
> >
> > Sorry about that, I'll sync up with whatever changes have gone in and
> > generate new patches for the cryptodev engine related bits.
> >
> > Should they appear in new "rt" tickets of their own or keep it within
> > this
> > one ?
>
> I'll apply some of the bits where I know what they do. The main bit I
> wasn't sure about was the reason for a change to a static fd in
> get_dev_crypto(): there was no comment in the ticket of the patch as to
> why this was done.

No problems,  I have the log here,  it's just with version changes over
the years the code/reasons get moved around.  The log for that change:

        Clean up the cryptodev handling to not burn file descriptors or open the
        main channel more than needed.

But looking at the code I can't see how it achieves that.  I know that
ocf-linux had a lot of kernel issues in this area in the early days and
I can only guess this is somehow a remanent of working around that,
sorry for the noise :-(

Short answer, drop the fd changes in the patch.

Thanks,
Davidm

--
David McCullough,  david_mccullough@...,  Ph:[phone]
McAfee - SnapGear  http://www.snapgear.com                http://www.uCdot.org
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       openssl-dev@...
Automated List Manager                           majordomo@...

[Thor Lancelot Simon-2]

On Thu, Jul 02, 2009 at 12:28:36AM +0200, David McCullough via RT wrote:
>
> The reason the option exists is that in all but the most unusual cases,
> using cryptodev hashing via some HW device is not worth it by any measure.
> There are some people using it,  but they have specific applications/needs.

This is an API limitation -- or was -- within OpenSSL.  The way hashes are
fed to the ENGINE effectively cuts performance in half because almost all
accellerators can do HMAC in one pass for the hshes they support.  But
there were not, at least a year or so ago, separate EVPs for HMAC.

I have noticed that those EVPs exist now.  That would make hashing in the
ENGINE a lot more efficient.

More generally, the approach taken by the original authors of the
cryptodev engine to enabling/disabling functionality according to its
performance was the wrong one.  The engine *already* has a protracted
startup process in which it tries every algorithm to see which work;
it could very easily do a quick-and-dirty calibration of the crossover
point, in terms of request size, for where hardware vs. software
handling of a request will be quicker.

The other things to remember with regard to cryptodev, particularly on
embedded platforms, sare:

1) If you have a single, single-core CPU, it can do _one_ operation at
   a time at the speed you'll measure with openssl speed -elapsed.  But
   the bottleneck going out to an accellerator via /dev/crypto is really
   a *syscall latency* bottleneck, which can be overcome with parallelism;
   for hashes, and even for small encryption requests, a test with, say,
   -multi 64 (which is a lot more realistic for most applications), will
   reveal that doing most things in hardware is a win.

2) The original cryptodev engine suffered from a serious misunderstanding
   of the /dev/crypto API, probably because the documentation was unclear.
   It basically does twice as many ioctls per session as it has to.  We
   fixed this in NetBSD but I don't think the change made it back to upstream
   OpenSSL.

3) There are backwards-compatible changes to the /dev/crypto API itself
   available (checked into NetBSD quite some time ago, shipped in NetBSD 5)
   which were designed to make it much more efficient for realistic use
   cases including OpenSSL.  Specifically, they streamline the session
   creation process, allow asynchronous operation via select/poll/kqueue,
   and allow multiple results to be retrieved with a single operation.  We
   intended to write a new OpenSSL engine to service multiple threads
   within an application using a single engine worker thread but that part
   isn't done yet.  Anyway, the basic support for this is there for the
   taking.  Supposedly, FreeBSD will pick it up when someone gets the
   time.

Thor
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       openssl-dev@...
Automated List Manager                           majordomo@...

[David McCullough-3]

Jivin Thor Lancelot Simon lays it down ...

> On Thu, Jul 02, 2009 at 12:28:36AM +0200, David McCullough via RT wrote:
> >
> > The reason the option exists is that in all but the most unusual cases,
> > using cryptodev hashing via some HW device is not worth it by any measure.
> > There are some people using it,  but they have specific applications/needs.
>
> This is an API limitation -- or was -- within OpenSSL.  The way hashes are
> fed to the ENGINE effectively cuts performance in half because almost all
> accellerators can do HMAC in one pass for the hshes they support.  But
> there were not, at least a year or so ago, separate EVPs for HMAC.
>
> I have noticed that those EVPs exist now.  That would make hashing in the
> ENGINE a lot more efficient.
>
> More generally, the approach taken by the original authors of the
> cryptodev engine to enabling/disabling functionality according to its
> performance was the wrong one.  The engine *already* has a protracted
> startup process in which it tries every algorithm to see which work;
> it could very easily do a quick-and-dirty calibration of the crossover
> point, in terms of request size, for where hardware vs. software
> handling of a request will be quicker.

Yes,  but the cross over is not that easy really.  If your requirement is
straight speed alone,  yes, easy,  but if CPU utilisation and/or
parallelisation come into play, then it gets trickier.

Having some HW doing crypto, albiet slower than the CPU can still be
beneficial if there is significant "other" processing you can get done while
waiting for the crypto HW to return.

But yes,  for the simple cases it can be automatically determined.

Do you know if NetBSD has done anything to fix the 8K request failures I see
on some OCF HW drivers (from memory hifn and safenet,  but it's been a while
so I may be wrong on one of them).

Add to that cryptodevs 64K request limit which is also not handled by
openssl.

> The other things to remember with regard to cryptodev, particularly on
> embedded platforms, sare:
>
> 1) If you have a single, single-core CPU, it can do _one_ operation at
>    a time at the speed you'll measure with openssl speed -elapsed.  But
>    the bottleneck going out to an accellerator via /dev/crypto is really
>    a *syscall latency* bottleneck, which can be overcome with parallelism;
>    for hashes, and even for small encryption requests, a test with, say,
>    -multi 64 (which is a lot more realistic for most applications), will
>    reveal that doing most things in hardware is a win.

Agreed, but if the overheads of the copyin/copyout to kernel space exceed
the cost of just doing the hash,  you are better off avoiding /dev/crypto.

The usual trend we see is small packets don't cost in that well and larger
ones do.  Mostly due to the context switching/copying overheads becoming
insignificant as the size increases.

> 2) The original cryptodev engine suffered from a serious misunderstanding
>    of the /dev/crypto API, probably because the documentation was unclear.
>    It basically does twice as many ioctls per session as it has to.  We
>    fixed this in NetBSD but I don't think the change made it back to upstream
>    OpenSSL.

Anything that halves the ioctls is a good thing from my point of view :-)

> 3) There are backwards-compatible changes to the /dev/crypto API itself
>    available (checked into NetBSD quite some time ago, shipped in NetBSD 5)
>    which were designed to make it much more efficient for realistic use
>    cases including OpenSSL.  Specifically, they streamline the session
>    creation process, allow asynchronous operation via select/poll/kqueue,
>    and allow multiple results to be retrieved with a single operation.  We
>    intended to write a new OpenSSL engine to service multiple threads
>    within an application using a single engine worker thread but that part
>    isn't done yet.  Anyway, the basic support for this is there for the
>    taking.  Supposedly, FreeBSD will pick it up when someone gets the
>    time.

How is/was the helper thread being used ?  I sort of figured the
kernel/user seperations of /dev/crypto avoided and real need for threads to
help out in user space,  but that may be my wishfull thinking on my part.

Cheers,
Davidm

--
David McCullough,  david_mccullough@...,  Ph:[phone]
McAfee - SnapGear  http://www.snapgear.com                http://www.uCdot.org
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       openssl-dev@...
Automated List Manager                           majordomo@...