gh-ARMmbed/mbed-crypto

Watch

Fork 0

Remove temporary allocation in mbedtls_mpi_sub_abs #366

Closed

marchmarch wants to merge 3 commits from development into development

marchmarch commented

2020-02-11 21:25:25 -06:00

(Migrated from github.com)

In cases where X == B the code now no longer requires an extra call to
mbedtls_calloc if X->n >= A->n. This reduces calls to mbedtls_calloc in
test_suite_ecdsa by 52% and in test_suite_ecdh by 36%.

This does not update mpi_sub_hlp - in mpi_montmul this translates to a
performance decrease for rsa and dhm since in all cases X == A.

In cases where X == B the code now no longer requires an extra call to mbedtls_calloc if X->n >= A->n. This reduces calls to mbedtls_calloc in test_suite_ecdsa by 52% and in test_suite_ecdh by 36%. This does not update mpi_sub_hlp - in mpi_montmul this translates to a performance decrease for rsa and dhm since in all cases X == A.

gilles-peskine-arm commented

2020-02-12 03:40:52 -06:00

(Migrated from github.com)

Thank you for your contribution!

Unfortunately there are build failures on some of the platforms we test, specifically with Visual Studio and IAR. Here are the logs:
iar8-mingw.txt
Windows-2013.txt

Thank you for your contribution! Unfortunately there are build failures on some of the platforms we test, specifically with Visual Studio and IAR. Here are the logs: [iar8-mingw.txt](https://github.com/ARMmbed/mbed-crypto/files/4190995/iar8-mingw.txt) [Windows-2013.txt](https://github.com/ARMmbed/mbed-crypto/files/4191038/Windows-2013.txt)

marchmarch commented

2020-02-16 19:23:49 -06:00

(Migrated from github.com)

Hi, did my last push should resolve the build failures? Either way, it looks like some of the automated tests are still failing. Is there some way for me to get access to the logs for the failed tests? All the tests in pre-push.sh, as well as run-test-suites.pl, pass on my machine so it would be helpful to see the logs.

gilles-peskine-arm commented

2020-02-17 04:57:25 -06:00

(Migrated from github.com)

@marchmarch Thanks for the update! The CI now passes apart from some Mbed OS jobs which fail for a pre-existing reason and which wouldn't block this PR.

gilles-peskine-arm (Migrated from github.com) reviewed

2020-02-18 07:57:40 -06:00

gilles-peskine-arm (Migrated from github.com) left a comment

I'm ok with the new subtraction code. The performance gain is substantial on my PC (+10-20% on P curves). However it does increase the code size a little.

Is there really a performance hit if we use the subtraction new code only? I just did a very quick test on my PC: replace the calls to mpi_sub_hlp in mpi_montmul with calls to mbedtls_mpi_sub_abs (with the redundant cmp call) and ran programs/test/benchmark rsa dhm. The change didn't make a perceptible difference.

I'm ok with the new subtraction code. The performance gain is substantial on my PC (+10-20% on P curves). However it does increase the code size a little. Is there really a performance hit if we use the subtraction new code only? I just did a very quick test on my PC: replace the calls to `mpi_sub_hlp` in `mpi_montmul` with calls to `mbedtls_mpi_sub_abs` (with the redundant `cmp` call) and ran `programs/test/benchmark rsa dhm`. The change didn't make a perceptible difference.

gilles-peskine-arm commented

2020-02-18 08:03:21 -06:00

(Migrated from github.com)

We don't directly test most bignum functions with aliased arguments (i.e. when a result is also an argument). We do usually test them indirectly through rsa/ecp/… but that's indirect. Since this PR modifies mbedtls_mpi_sub_abs, I'd like to set the ball rolling with this function. Can you please cherry-pick github.com/gilles-peskine-arm/mbed-crypto@4273d9de92 into this PR?

We don't directly test most bignum functions with aliased arguments (i.e. when a result is also an argument). We do usually test them indirectly through rsa/ecp/… but that's indirect. Since this PR modifies `mbedtls_mpi_sub_abs`, I'd like to set the ball rolling with this function. Can you please cherry-pick https://github.com/gilles-peskine-arm/mbed-crypto/commit/4273d9de9265c565a9b5cc71580c506a95c1bf4c into this PR?

gilles-peskine-arm commented

2020-02-18 08:04:31 -06:00

(Migrated from github.com)

We credit all contributions in the Mbed TLS changelog. Would you like to be credited as “Eric Adamson”, “marchmarch” or something else?

gilles-peskine-arm commented

2020-02-18 08:39:06 -06:00

(Migrated from github.com)

I am instructed not to accept contributions without a contributor license agreement until further notice. Have you signed one already? If you haven't, you'll need to sign one, or wait a little as we're working to get rid of this legal requirement.

I am instructed not to accept contributions without a [contributor license agreement](https://www.mbed.com/en/about-mbed/contributor-license-agreements/) until further notice. Have you signed one already? If you haven't, you'll need to sign one, or wait a little as we're working to get rid of this legal requirement.

marchmarch commented

2020-02-18 20:52:40 -06:00

(Migrated from github.com)

Is there really a performance hit if we use the subtraction new code only?

Yes, the performance hit was pretty minor. On my PC the rsa/dhm benchmarks are on the order of 1% worse if I pull the new subtraction code out into mpi_sub_hlp. When I compile the current code mpi_sub_hlp is 226 bytes, so maybe it's worth taking the performance hit. I'm happy to either move the code into mpi_sub_hlp or leave it as-is.

Can you please cherry-pick gilles-peskine-arm@4273d9d into this PR?

Done.

Would you like to be credited as “Eric Adamson”, “marchmarch” or something else?

Please credit me as "Eric Adamson".

I am instructed not to accept contributions without a contributor license agreement until further notice. Have you signed one already? If you haven't, you'll need to sign one, or wait a little as we're working to get rid of this legal requirement.

I don't mind signing it but I won't have access to a scanner until later in the week. I'll reply again once I've submitted the form.

> Is there really a performance hit if we use the subtraction new code only? Yes, the performance hit was pretty minor. On my PC the rsa/dhm benchmarks are on the order of 1% worse if I pull the new subtraction code out into mpi_sub_hlp. When I compile the current code mpi_sub_hlp is 226 bytes, so maybe it's worth taking the performance hit. I'm happy to either move the code into mpi_sub_hlp or leave it as-is. > Can you please cherry-pick [gilles-peskine-arm@4273d9d](https://github.com/gilles-peskine-arm/mbed-crypto/commit/4273d9de9265c565a9b5cc71580c506a95c1bf4c) into this PR? Done. > Would you like to be credited as “Eric Adamson”, “marchmarch” or something else? Please credit me as "Eric Adamson". > I am instructed not to accept contributions without a [contributor license agreement](https://www.mbed.com/en/about-mbed/contributor-license-agreements/) until further notice. Have you signed one already? If you haven't, you'll need to sign one, or wait a little as we're working to get rid of this legal requirement. I don't mind signing it but I won't have access to a scanner until later in the week. I'll reply again once I've submitted the form.

gilles-peskine-arm commented

2020-02-19 05:19:46 -06:00

(Migrated from github.com)

On my PC the rsa/dhm benchmarks are on the order of 1% worse

Personally I find a 1% hit on operations that are slow anyway acceptable. You can often save or lose that much by doing things like changing the alignment of the code with respect to cache lines anyway.

For the code size, with a particular set of build options, I get 15885B → 15965B for bignum.o on Cortex-M0. That's not a lot, but it's within the order of magnitude where we do care.

Can you please make a separate PR with a single implementation of subtraction for comparison?

On the license agreement issue, we should have a resolution that allows you to sign electronically very soon. Please stay tuned.

> On my PC the rsa/dhm benchmarks are on the order of 1% worse Personally I find a 1% hit on operations that are slow anyway acceptable. You can often save or lose that much by doing things like changing the alignment of the code with respect to cache lines anyway. For the code size, with a particular set of build options, I get 15885B → 15965B for `bignum.o` on Cortex-M0. That's not a lot, but it's within the order of magnitude where we do care. Can you please make a separate PR with a single implementation of subtraction for comparison? On the license agreement issue, we should have a resolution that allows you to sign electronically very soon. Please stay tuned.

gilles-peskine-arm commented

2020-02-19 08:41:46 -06:00

(Migrated from github.com)

On the legal side, it's now official: we don't need a CLA anymore. Instead, please indicate that you accept to contribute your code under a dual license Apache-2.0 + GPL-2.0-or-later by including a “Signed-off-by” line in each commit message.

On the legal side, [it's now official](https://github.com/ARMmbed/mbed-crypto/issues/371): we don't need a CLA anymore. Instead, please indicate that you accept to contribute your code under a dual license Apache-2.0 + GPL-2.0-or-later by including a “Signed-off-by” line in each commit message.

marchmarch commented

2020-02-19 20:42:18 -06:00

(Migrated from github.com)

Can you please make a separate PR with a single implementation of subtraction for comparison?

Ok, I created a second pull request here: https://github.com/ARMmbed/mbed-crypto/pull/373

On the legal side, it's now official: we don't need a CLA anymore.

That's great news!

>Can you please make a separate PR with a single implementation of subtraction for comparison? Ok, I created a second pull request here: https://github.com/ARMmbed/mbed-crypto/pull/373 >On the legal side, it's now official: we don't need a CLA anymore. That's great news!

marchmarch commented

2020-03-01 19:34:28 -06:00

(Migrated from github.com)