[PATCH] Use Intel AES instruction set if it exists - v3

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Use Intel AES instruction set if it exists - v3

Samba - samba-technical mailing list
Hi all,

Off-list Justin @ Netgear has been doing
some performance measurements between native
Samba AES, the libnettle crypto library and
Intel AES instructions.

Whilst doing that he discovered that on Debian 9,
and Ubuntu 17.04 and before, libnettle has been
built without AES instruction support and is thus
much *slower* than our native crypto. On Fedora
and SuSE it's correctly built and so provides better
performance, although the native Intel AES code is
still the fastest.

I don't have permission to publish his absolute numbers,
but have a work-around here of publishing comparative
results (hope that's OK Justin, but it's easier to
ask for forgiveness than wait for permission:-).
Consider native Samba as performance 1.000. We have:

Native Samba AES code: 1.000
Intel AES code: 2.386
libnettle --enable-fat (Fedora|SuSE): 1.704
libnettle (Debian|Ubuntu): 0.818

As you can see, Intel AES code gives a significant
advantage.

Given that, after discussions offline with Andreas
(who has to support FIPS certification for Fedora)
and Metze, here is a patchset that allows configure
time selection of AES crypto.

--accel-aes=none (default - use Samba native crypto)

--accel-aes=nettle|libnettle (Use libnettle)

--accel-aes=intelaesni (Use third_party code)

Part of this is a WHATSNEW that specifies that
the --accel-aes=intelaesni and supporting code
is a temporary fix and WILL be removed from Samba
once libnettle reaches performance parity.

Andreas, let me know if this meets your requirements.

Cheers,

Jeremy.

aes.patch (116K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists - v3

Samba - samba-technical mailing list
On Tue, 2017-09-05 at 14:08 -0700, Jeremy Allison via samba-technical
wrote:

> Native Samba AES code: 1.000
> Intel AES code: 2.386
> libnettle --enable-fat (Fedora|SuSE): 1.704
> libnettle (Debian|Ubuntu): 0.818
>
> As you can see, Intel AES code gives a significant
> advantage.
>
> Given that, after discussions offline with Andreas
> (who has to support FIPS certification for Fedora)
> and Metze, here is a patchset that allows configure
> time selection of AES crypto.

I'm satisfied that this addresses my objections, I'll leave the rest
between you and Andreas.

The next step would be to get more of our code using GnuTLS or
libnettle, just to get out of maintaining crypto.  

Andrew Bartlett
--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists - v3

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tuesday, 5 September 2017 23:08:45 CEST Jeremy Allison via samba-technical
wrote:

> Hi all,
>
> Off-list Justin @ Netgear has been doing
> some performance measurements between native
> Samba AES, the libnettle crypto library and
> Intel AES instructions.
>
> Whilst doing that he discovered that on Debian 9,
> and Ubuntu 17.04 and before, libnettle has been
> built without AES instruction support and is thus
> much *slower* than our native crypto. On Fedora
> and SuSE it's correctly built and so provides better
> performance, although the native Intel AES code is
> still the fastest.
>
> I don't have permission to publish his absolute numbers,
> but have a work-around here of publishing comparative
> results (hope that's OK Justin, but it's easier to
> ask for forgiveness than wait for permission:-).
> Consider native Samba as performance 1.000. We have:
>
> Native Samba AES code: 1.000
> Intel AES code: 2.386
> libnettle --enable-fat (Fedora|SuSE): 1.704
> libnettle (Debian|Ubuntu): 0.818
>
> As you can see, Intel AES code gives a significant
> advantage.
>
> Given that, after discussions offline with Andreas
> (who has to support FIPS certification for Fedora)
> and Metze, here is a patchset that allows configure
> time selection of AES crypto.
>
> --accel-aes=none (default - use Samba native crypto)
>
> --accel-aes=nettle|libnettle (Use libnettle)
>
> --accel-aes=intelaesni (Use third_party code)
>
> Part of this is a WHATSNEW that specifies that
> the --accel-aes=intelaesni and supporting code
> is a temporary fix and WILL be removed from Samba
> once libnettle reaches performance parity.
>
> Andreas, let me know if this meets your requirements.

I've talked to Nikos. GnuTLS uses the AES-NI assembler code from OpenSSL and
it is much much faster than what libnettle offers:

Benchmark with libnettle:
GNUTLS_CPUID_OVERRIDE=1 gnutls-cli --benchmark-ciphers

Benchmark with GnuTLS AES-NI:
gnutls-cli --benchmark-ciphers

Since GnuTLS 3.4 (we require 3.4.7 right now) there are new AEAD cipher
functions. Maybe this is going into the direction metze wants to have, see

https://www.gnutls.org/manual/html_node/Symmetric-algorithms.html


Jeremy, just push the Intel AES-NI. I think we should use the GnuTLS for this
which will be faster then what nettle offeres right now. Also distributions
have probably the GnuTLS version we require and with AES-NI support.



        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists - v3

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Hi Jeremy,

can you please check the indentation in various wscript files,
they're python scripts and should not use tabs, 4 spaces.

Also running "git show eb1f476657c" in a recent kernel checkout,
gives a failure.

Can you please make sure we important a well defined state
of arch/x86/crypto/aesni-intel_asm.S from the kernel,
including the exact full commit hash of the checkout
you used in our commit message.

git show $commit:arch/x86/crypto/aesni-intel_asm.S should
should exactly what we import.

This will make it much easier to track what state we have.

There have been some changes this year regarding AES-GCM
(which we don't use yet, but may want to experiment with).
So I think it would be good to import the recent version from the
kernel.

> Off-list Justin @ Netgear has been doing
> some performance measurements between native
> Samba AES, the libnettle crypto library and
> Intel AES instructions.
>
> Whilst doing that he discovered that on Debian 9,
> and Ubuntu 17.04 and before, libnettle has been
> built without AES instruction support and is thus
> much *slower* than our native crypto. On Fedora
> and SuSE it's correctly built and so provides better
> performance, although the native Intel AES code is
> still the fastest.
>
> I don't have permission to publish his absolute numbers,
> but have a work-around here of publishing comparative
> results (hope that's OK Justin, but it's easier to
> ask for forgiveness than wait for permission:-).
> Consider native Samba as performance 1.000. We have:
>
> Native Samba AES code: 1.000
> Intel AES code: 2.386
> libnettle --enable-fat (Fedora|SuSE): 1.704
> libnettle (Debian|Ubuntu): 0.818
>
> As you can see, Intel AES code gives a significant
> advantage.
>
> Given that, after discussions offline with Andreas
> (who has to support FIPS certification for Fedora)
> and Metze, here is a patchset that allows configure
> time selection of AES crypto.
>
> --accel-aes=none (default - use Samba native crypto)
>
> --accel-aes=nettle|libnettle (Use libnettle)
>
> --accel-aes=intelaesni (Use third_party code)
>
> Part of this is a WHATSNEW that specifies that
> the --accel-aes=intelaesni and supporting code
> is a temporary fix and WILL be removed from Samba
> once libnettle reaches performance parity.
>
> Andreas, let me know if this meets your requirements.
Otherwise it looks ok.

metze



signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists - v3

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Am 06.09.2017 um 13:35 schrieb Andreas Schneider:

> On Tuesday, 5 September 2017 23:08:45 CEST Jeremy Allison via samba-technical
> wrote:
>> Hi all,
>>
>> Off-list Justin @ Netgear has been doing
>> some performance measurements between native
>> Samba AES, the libnettle crypto library and
>> Intel AES instructions.
>>
>> Whilst doing that he discovered that on Debian 9,
>> and Ubuntu 17.04 and before, libnettle has been
>> built without AES instruction support and is thus
>> much *slower* than our native crypto. On Fedora
>> and SuSE it's correctly built and so provides better
>> performance, although the native Intel AES code is
>> still the fastest.
>>
>> I don't have permission to publish his absolute numbers,
>> but have a work-around here of publishing comparative
>> results (hope that's OK Justin, but it's easier to
>> ask for forgiveness than wait for permission:-).
>> Consider native Samba as performance 1.000. We have:
>>
>> Native Samba AES code: 1.000
>> Intel AES code: 2.386
>> libnettle --enable-fat (Fedora|SuSE): 1.704
>> libnettle (Debian|Ubuntu): 0.818
>>
>> As you can see, Intel AES code gives a significant
>> advantage.
>>
>> Given that, after discussions offline with Andreas
>> (who has to support FIPS certification for Fedora)
>> and Metze, here is a patchset that allows configure
>> time selection of AES crypto.
>>
>> --accel-aes=none (default - use Samba native crypto)
>>
>> --accel-aes=nettle|libnettle (Use libnettle)
>>
>> --accel-aes=intelaesni (Use third_party code)
>>
>> Part of this is a WHATSNEW that specifies that
>> the --accel-aes=intelaesni and supporting code
>> is a temporary fix and WILL be removed from Samba
>> once libnettle reaches performance parity.
>>
>> Andreas, let me know if this meets your requirements.
>
> I've talked to Nikos. GnuTLS uses the AES-NI assembler code from OpenSSL and
> it is much much faster than what libnettle offers:
>
> Benchmark with libnettle:
> GNUTLS_CPUID_OVERRIDE=1 gnutls-cli --benchmark-ciphers
>
> Benchmark with GnuTLS AES-NI:
> gnutls-cli --benchmark-ciphers
>
> Since GnuTLS 3.4 (we require 3.4.7 right now) there are new AEAD cipher
> functions. Maybe this is going into the direction metze wants to have, see
>
> https://www.gnutls.org/manual/html_node/Symmetric-algorithms.html
It's not what I'm looking for as it means we would have to use talloc
and memcpy in smb2_signing_[en|de]crypt_pdu(), but with a speed
of AES-128-GCM 1.40 GB/sec it might be a good start. So we better take
the cost of talloc and memcpy then the cost of aes-gcm, but we'll have
to test that first.

But first we should convert to an api like this:

void aes_gcm_128_init(struct aes_gcm_128_context *ctx,
                      const uint8_t K[AES_BLOCK_SIZE],
                      const uint8_t IV[AES_GCM_128_IV_SIZE]);
void aes_gcm_128_update(struct aes_gcm_128_context *ctx,
                        const struct iovec *a_iov, size_t a_iov_count,
                        struct iovec *m, size_t m_iov_count,
                        bool forward);
void aes_gcm_128_digest(struct aes_gcm_128_context *ctx,
                        uint8_t T[AES_BLOCK_SIZE]);

(and similar onces for aes_ccm).

So that the memcpy will be transparent to our SMB3 code.

metze


signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists - v3

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Wed, Sep 06, 2017 at 03:38:20PM +0200, Stefan Metzmacher wrote:
> Hi Jeremy,
>
> can you please check the indentation in various wscript files,
> they're python scripts and should not use tabs, 4 spaces.
>
> Also running "git show eb1f476657c" in a recent kernel checkout,
> gives a failure.

That's a Samba hash, not a kernel one.

> Can you please make sure we important a well defined state
> of arch/x86/crypto/aesni-intel_asm.S from the kernel,
> including the exact full commit hash of the checkout
> you used in our commit message.
>
> git show $commit:arch/x86/crypto/aesni-intel_asm.S should
> should exactly what we import.
>
> This will make it much easier to track what state we have.

That's a good idea and will help us track any changes
made for security.

> There have been some changes this year regarding AES-GCM
> (which we don't use yet, but may want to experiment with).
> So I think it would be good to import the recent version from the
> kernel.

I'll take a look.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists - v3

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Wed, Sep 06, 2017 at 01:35:54PM +0200, Andreas Schneider wrote:

>
> I've talked to Nikos. GnuTLS uses the AES-NI assembler code from OpenSSL and
> it is much much faster than what libnettle offers:
>
> Benchmark with libnettle:
> GNUTLS_CPUID_OVERRIDE=1 gnutls-cli --benchmark-ciphers
>
> Benchmark with GnuTLS AES-NI:
> gnutls-cli --benchmark-ciphers
>
> Since GnuTLS 3.4 (we require 3.4.7 right now) there are new AEAD cipher
> functions. Maybe this is going into the direction metze wants to have, see
>
> https://www.gnutls.org/manual/html_node/Symmetric-algorithms.html
>
>
> Jeremy, just push the Intel AES-NI. I think we should use the GnuTLS for this
> which will be faster then what nettle offeres right now. Also distributions
> have probably the GnuTLS version we require and with AES-NI support.

OK, I'll address Metze's comments and then post a version
that keeps the --accel-aes=[none|intelaesni] options only,
no nettle (set to "none" by default).

Cheers,

Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists - v3

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Am 06.09.2017 um 18:15 schrieb Jeremy Allison:

> On Wed, Sep 06, 2017 at 03:38:20PM +0200, Stefan Metzmacher wrote:
>> Hi Jeremy,
>>
>> can you please check the indentation in various wscript files,
>> they're python scripts and should not use tabs, 4 spaces.
>>
>> Also running "git show eb1f476657c" in a recent kernel checkout,
>> gives a failure.
>
> That's a Samba hash, not a kernel one.
No, that's the hash of the file content and I'd assume that doesn't
change if we just copy the file.

>> Can you please make sure we important a well defined state
>> of arch/x86/crypto/aesni-intel_asm.S from the kernel,
>> including the exact full commit hash of the checkout
>> you used in our commit message.
>>
>> git show $commit:arch/x86/crypto/aesni-intel_asm.S should
>> should exactly what we import.
>>
>> This will make it much easier to track what state we have.
>
> That's a good idea and will help us track any changes
> made for security.
>
>> There have been some changes this year regarding AES-GCM
>> (which we don't use yet, but may want to experiment with).
>> So I think it would be good to import the recent version from the
>> kernel.
>
> I'll take a look.
Thanks!

metze



signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists - v3

Samba - samba-technical mailing list
On Wed, Sep 06, 2017 at 08:52:58PM +0200, Stefan Metzmacher wrote:

> Am 06.09.2017 um 18:15 schrieb Jeremy Allison:
> > On Wed, Sep 06, 2017 at 03:38:20PM +0200, Stefan Metzmacher wrote:
> >> Hi Jeremy,
> >>
> >> can you please check the indentation in various wscript files,
> >> they're python scripts and should not use tabs, 4 spaces.
> >>
> >> Also running "git show eb1f476657c" in a recent kernel checkout,
> >> gives a failure.
> >
> > That's a Samba hash, not a kernel one.
>
> No, that's the hash of the file content and I'd assume that doesn't
> change if we just copy the file.

Ah, got it. We do need to change it to compile as a -fPIC library
for Samba.

> Thanks!

I've now got a patchset that starts from a pristine kernel
check-in with a known git refspec and shows what is modified
from there.

Testing now.

Reply | Threaded
Open this post in threaded view
|

[PATCH] Use Intel AES instruction set if it exists - v4

Samba - samba-technical mailing list
On Wed, Sep 06, 2017 at 12:10:17PM -0700, Jeremy Allison via samba-technical wrote:

> On Wed, Sep 06, 2017 at 08:52:58PM +0200, Stefan Metzmacher wrote:
> > Am 06.09.2017 um 18:15 schrieb Jeremy Allison:
> > > On Wed, Sep 06, 2017 at 03:38:20PM +0200, Stefan Metzmacher wrote:
> > >> Hi Jeremy,
> > >>
> > >> can you please check the indentation in various wscript files,
> > >> they're python scripts and should not use tabs, 4 spaces.
> > >>
> > >> Also running "git show eb1f476657c" in a recent kernel checkout,
> > >> gives a failure.
> > >
> > > That's a Samba hash, not a kernel one.
> >
> > No, that's the hash of the file content and I'd assume that doesn't
> > change if we just copy the file.
>
> Ah, got it. We do need to change it to compile as a -fPIC library
> for Samba.
>
> > Thanks!
>
> I've now got a patchset that starts from a pristine kernel
> check-in with a known git refspec and shows what is modified
> from there.
>
> Testing now.
Here it is. Please review. I'm hoping v4 will be the last one,
fingers crossed !

Jeremy.

aes-4.patch (113K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists - v4

Samba - samba-technical mailing list
Am 06.09.2017 um 22:07 schrieb Jeremy Allison:

> On Wed, Sep 06, 2017 at 12:10:17PM -0700, Jeremy Allison via samba-technical wrote:
>> On Wed, Sep 06, 2017 at 08:52:58PM +0200, Stefan Metzmacher wrote:
>>> Am 06.09.2017 um 18:15 schrieb Jeremy Allison:
>>>> On Wed, Sep 06, 2017 at 03:38:20PM +0200, Stefan Metzmacher wrote:
>>>>> Hi Jeremy,
>>>>>
>>>>> can you please check the indentation in various wscript files,
>>>>> they're python scripts and should not use tabs, 4 spaces.
>>>>>
>>>>> Also running "git show eb1f476657c" in a recent kernel checkout,
>>>>> gives a failure.
>>>>
>>>> That's a Samba hash, not a kernel one.
>>>
>>> No, that's the hash of the file content and I'd assume that doesn't
>>> change if we just copy the file.
>>
>> Ah, got it. We do need to change it to compile as a -fPIC library
>> for Samba.
>>
>>> Thanks!
>>
>> I've now got a patchset that starts from a pristine kernel
>> check-in with a known git refspec and shows what is modified
>> from there.
>>
>> Testing now.
>
> Here it is. Please review. I'm hoping v4 will be the last one,
> fingers crossed !
What's the reason to take such an old version?

There were some fixes in the patchset up to
cce2ea8d90fec72ed954830e73f5d4995cc79193 (crypto: aesni - add generic
gcm(aes))

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/arch/x86/crypto

See 38d9deecab77b9eb38cb8bb5dfa43237c6710188
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/x86/crypto?id=38d9deecab77b9eb38cb8bb5dfa43237c6710188

metze



signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists - v4

Samba - samba-technical mailing list
On Thu, Sep 07, 2017 at 01:49:22AM +0200, Stefan Metzmacher wrote:

> Am 06.09.2017 um 22:07 schrieb Jeremy Allison:
> > On Wed, Sep 06, 2017 at 12:10:17PM -0700, Jeremy Allison via samba-technical wrote:
> >> On Wed, Sep 06, 2017 at 08:52:58PM +0200, Stefan Metzmacher wrote:
> >>> Am 06.09.2017 um 18:15 schrieb Jeremy Allison:
> >>>> On Wed, Sep 06, 2017 at 03:38:20PM +0200, Stefan Metzmacher wrote:
> >>>>> Hi Jeremy,
> >>>>>
> >>>>> can you please check the indentation in various wscript files,
> >>>>> they're python scripts and should not use tabs, 4 spaces.
> >>>>>
> >>>>> Also running "git show eb1f476657c" in a recent kernel checkout,
> >>>>> gives a failure.
> >>>>
> >>>> That's a Samba hash, not a kernel one.
> >>>
> >>> No, that's the hash of the file content and I'd assume that doesn't
> >>> change if we just copy the file.
> >>
> >> Ah, got it. We do need to change it to compile as a -fPIC library
> >> for Samba.
> >>
> >>> Thanks!
> >>
> >> I've now got a patchset that starts from a pristine kernel
> >> check-in with a known git refspec and shows what is modified
> >> from there.
> >>
> >> Testing now.
> >
> > Here it is. Please review. I'm hoping v4 will be the last one,
> > fingers crossed !
>
> What's the reason to take such an old version?

The modifications to move data to read-only sections and
other changes made it impossible for me to make this into
a Samba shared library. The version I took was the latest
one that contained all the code we use that is easy to
make a shared library (check out the small patch to the
pristine source code that follows the initial check-in).

> There were some fixes in the patchset up to
> cce2ea8d90fec72ed954830e73f5d4995cc79193 (crypto: aesni - add generic
> gcm(aes))

We're not currently using those. I think we should get
that when we do the migration to GNUtls.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists - v4

Samba - samba-technical mailing list
Am 07.09.2017 um 01:55 schrieb Jeremy Allison:

> On Thu, Sep 07, 2017 at 01:49:22AM +0200, Stefan Metzmacher wrote:
>> Am 06.09.2017 um 22:07 schrieb Jeremy Allison:
>>> On Wed, Sep 06, 2017 at 12:10:17PM -0700, Jeremy Allison via samba-technical wrote:
>>>> On Wed, Sep 06, 2017 at 08:52:58PM +0200, Stefan Metzmacher wrote:
>>>>> Am 06.09.2017 um 18:15 schrieb Jeremy Allison:
>>>>>> On Wed, Sep 06, 2017 at 03:38:20PM +0200, Stefan Metzmacher wrote:
>>>>>>> Hi Jeremy,
>>>>>>>
>>>>>>> can you please check the indentation in various wscript files,
>>>>>>> they're python scripts and should not use tabs, 4 spaces.
>>>>>>>
>>>>>>> Also running "git show eb1f476657c" in a recent kernel checkout,
>>>>>>> gives a failure.
>>>>>>
>>>>>> That's a Samba hash, not a kernel one.
>>>>>
>>>>> No, that's the hash of the file content and I'd assume that doesn't
>>>>> change if we just copy the file.
>>>>
>>>> Ah, got it. We do need to change it to compile as a -fPIC library
>>>> for Samba.
>>>>
>>>>> Thanks!
>>>>
>>>> I've now got a patchset that starts from a pristine kernel
>>>> check-in with a known git refspec and shows what is modified
>>>> from there.
>>>>
>>>> Testing now.
>>>
>>> Here it is. Please review. I'm hoping v4 will be the last one,
>>> fingers crossed !
>>
>> What's the reason to take such an old version?
>
> The modifications to move data to read-only sections and
> other changes made it impossible for me to make this into
> a Samba shared library. The version I took was the latest
> one that contained all the code we use that is easy to
> make a shared library (check out the small patch to the
> pristine source code that follows the initial check-in).
>
>> There were some fixes in the patchset up to
>> cce2ea8d90fec72ed954830e73f5d4995cc79193 (crypto: aesni - add generic
>> gcm(aes))
>
> We're not currently using those. I think we should get
> that when we do the migration to GNUtls.
Ok, reviewed-by-me then.

Thanks!
metze



signature.asc (853 bytes) Download Attachment