Small performance improvement of mbedtls_mpi_div_mpi() #308
Conversation
krizhanovsky
commented
Oct 31, 2019
krizhanovsky
commented
- don't use dynamic allocator for fixed size T2;
- move T2 initialization out of the inner loop.
1. don't use dynamic allocator for fixed size T2; 2. move T2 initialization out of the inner loop.
There was a problem hiding this comment.
Thank you for your contribution!
You've made two distinct changes in the same commit:
- Lift the code that fills
T2out of the loop. - Allocate
T2's content on the stack rather than on the heap.
The first change is uncontroversial. The second change may be a slight performance improvement but comes with the cost of making the code harder to read. Is it worth it? Do you have benchmarks that show that using the stack makes a visible difference?
library/bignum.c
Outdated
| @@ -1632,14 +1632,19 @@ int mbedtls_mpi_div_mpi( mbedtls_mpi *Q, mbedtls_mpi *R, const mbedtls_mpi *A, | |||
| int ret; | |||
| size_t i, n, t, k; | |||
| mbedtls_mpi X, Y, Z, T1, T2; | |||
| mbedtls_mpi_uint __tp2[3]; | |||
There was a problem hiding this comment.
Blocker: identifiers starting with __ are reserved for the C implementation. __tp2 needs to have a different name such as TP2. I kept __tp2 in my other comments for clarity.
library/bignum.c
Outdated
| mbedtls_mpi_init( &T1 ); | ||
| /* Avoid dynamic memory allocations for constant-size T2. */ | ||
| T2.s = 1; | ||
| T2.n = 3; |
There was a problem hiding this comment.
Please don't hard-code a value like this. It makes maintenance risky.
| T2.n = 3; | |
| T2.n = sizeof( __tp2 ) / sizeof( *__tp2 ); |
library/bignum.c
Outdated
| @@ -1632,14 +1632,19 @@ int mbedtls_mpi_div_mpi( mbedtls_mpi *Q, mbedtls_mpi *R, const mbedtls_mpi *A, | |||
| int ret; | |||
| size_t i, n, t, k; | |||
| mbedtls_mpi X, Y, Z, T1, T2; | |||
| mbedtls_mpi_uint __tp2[3]; | |||
There was a problem hiding this comment.
Please add a comment to explain why T2 always fits in 3 words (no matter what the word size is). Before, if setting 3 words was, the worst outcome was an incorrect calculation. Now if any operation causes T2 to grow the result will be heap corruption. So it needs to be very clear why this is correct.
| @@ -1736,7 +1739,7 @@ int mbedtls_mpi_div_mpi( mbedtls_mpi *Q, mbedtls_mpi *R, const mbedtls_mpi *A, | |||
| cleanup: | |||
|
|
|||
| mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); mbedtls_mpi_free( &Z ); | |||
| mbedtls_mpi_free( &T1 ); mbedtls_mpi_free( &T2 ); | |||
| mbedtls_mpi_free( &T1 ); | |||
There was a problem hiding this comment.
Blocker: __tp2 contains potentially sensitive data. It needs to be wiped with mbedtls_platform_zeroize.
gilles-peskine-arm
added
enhancement
|
CI only fails on one job ( |
1. variable name accoriding to the Mbed TLS coding style; 2. add a comment explaining safety of the optimization; 3. safer T2 initialization and memory zeroing on the function exit;
gilles-peskine-arm
added
needs: review
|
New full CI run (internal link) now that the CY8CKIT boards are working: https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-308-merge/6/ → PASS |
gilles-peskine-arm
added
needs: ci
gilles-peskine-arm
removed
the
needs: review
* ARMmbed#321: Replace config.pl by config.py * ARMmbed#322: Update Mbed Crypto with latest Mbed TLS changes as of 2019-11-15 * ARMmbed#308: Small performance improvement of mbedtls_mpi_div_mpi() * ARMmbed#324: test_psa_constant_names: support key agreement, better code structure * ARMmbed#320: Link to the PSA crypto portal page from README.md * ARMmbed#293: Always gather MBEDTLS_ENTROPY_BLOCK_SIZE bytes of entropy * ARMmbed#310: Clarify test descriptions in test_suite_memory_buffer_alloc * ARMmbed#307: Add ASN.1 ENUMERATED tag support * ARMmbed#328: Remove dependency of crypto_values.h on crypto_extra.h * ARMmbed#325: Rename psa_asymmetric_{sign_verify} to psa_{sign,verify}_hash Missed listing in the previous submodule update: * ARMmbed#304: Make sure Asan failures are detected in 'make test'
