[Baremetal] Various minor changes to improve RAM measurements #2850

hanno-becker · 2019-09-20T12:25:23Z

This PR aims at making RAM measurements with scripts/baremetal.sh --ram more useful.

Specifically, this PR:

Updates ssl_client2 and ssl_server2 to allocate various larger structures on the heap instead of the stack, in order to ease memory usage tracking with massif.
Updates ssl_client2 and ssl_server2 to use a smaller request buffer in order to not inflate heap usage statistics with something not related to Mbed TLS' actual usage.
Updates ssl_client2 and ssl_server2 to make use of the existing copyless CRT parsing API mbedtls_x509_crt_parse_der_nocopy().
Modifes the baremetal.h config to allow only a single entropy source, which is likely enough for constrained environments, and significantly reduces the size of mbedtls_entropy_context.

Changes to RAM measurements

Before: Max heap usage: 30832 (heap 7283+165, stack 23384)
After: Max heap usage: 11944 (heap 9437+259, stack 2248)

This does not reflect changes in actual RAM usage of the library, only in RAM usage of the test client used for the measurements by the script. The following effects can be observed:

Total usage appears lower. That's because (1) we adjusted the size of the message buffer (previously hardcoded to 20k on the stack, now 4k on the heap) and (2) we're using the copyless CRT parsing API.
Heap usage appears a bit higher while stack usage appears much lower. Stack usage as measured is the sum of what's used by the library to perform a handshake, plus the stack frame of the test client's main() function, which was previously huge and has now been made much smaller, in part by moving the larger parts to the heap. Measured stack usage is now closer to that of the library.

mpg

Thanks for making those changes! I found an issue with the size of a buffer, and the CI found another with the type/allocation of a variable - actually I stopped at the first issue in the CI, so perhaps it found others as well.

mpg · 2019-09-23T07:34:00Z

programs/ssl/ssl_client2.c

@@ -75,8 +75,8 @@ int main( void )
 #include <stdlib.h>
 #include <string.h>

-#define MAX_REQUEST_SIZE      20000
-#define MAX_REQUEST_SIZE_STR "20000"
+#define MAX_REQUEST_SIZE      100


I don't think this is going to work well with ssl-opt.sh. We have multiple tests with request_size=123 (arguably for no good reason, so this could be changed to a lower value), and some with request_size=$(( MAX_CONTENT_LEN + 1 )). However those test are skipped when MAX_CONTENT_LEN < 4096, so perhaps here we could define MAX_REQUEST_SIZE as either MAX_CONTENT_LEN + 1 or a lower value depending on the value of MAX_REQUEST_SIZE (and leave appropriate comments here and in ssl-opt.sh.

Alternatively, we could allow MAX_REQUEST_SIZE to be defined on on the compiler's command line, providing only a default here, and defined a low enough value in baremetal.sh. Perhaps that would be a bit cleaner than having non-trivial logic tying this file to ssl-opt.sh.

Oh yes, of course... thank you for spotting. I have reverted this change for now.

programs/ssl/ssl_server2.c

hanno-becker · 2019-09-25T13:08:32Z

Thank you @mpg for your review - I have reverted the change of MAX_REQUEST_SIZE, and added the missing & operator. Could you please re-review?

jarvte · 2019-10-10T07:31:21Z

/test

This commit modifies the example programs ssl_client2 and ssl_server2 to allocate various structures on the heap instead of the stack. This allows more fine-grained memory usage tracking via valgrind massif.

…t way when giving to functions.

Allocations are now done after command line parsing. Added more checks if allocations are needed and fixed baremetal tests with these defines.

jarvte · 2019-10-16T04:49:13Z

/test

jarvte · 2019-10-17T10:38:27Z

/test

jarlamsa

One requested change, otherwise LGTM.

configs/baremetal.h

Override define MBEDTLS_ENTROPY_MAX_SOURCES from 1 to 3 in baremetal_test.h mbedtls_entropy_init adds 2 sources already so max must be 3 so that one source can be added with mbedtls_entropy_add_source.

jarlamsa

Thank you for making the change, LGTM

mpg

This is generally looking good! However, I found a possible memory error, and left a suggestion for an improvement.

programs/ssl/ssl_server2.c

programs/ssl/ssl_client2.c

mpg · 2019-10-25T07:46:12Z

Note: CI only failed in API-ABI check in pr-head job, while the same test passed in the pr-merge job. This is an artifact of the checking script, and not an indication that the PR breaks anything (actually the test passing in the pr-merge job confirms that the PR did not break the API or ABI), so can be ignored.

…sible

mpg

Thanks for fixing the issue and implementing the suggested improvement. Looks good to me!

jarlamsa

LGTM

mpg · 2019-10-28T10:19:57Z

CI passed, except for the API/ABI check which failed in pr-head and succeeded in pr-merge. As with other PRs, this only reflect a design issue versions picked by the API/ABI checker in the CI, not a problem in the PR: the fact the the check passed in pr-merge means the PR did not break the API or ABI. Therefore removing label "needs: CI".

mpg · 2019-10-28T10:21:05Z

This has two approval, a passing CI (except for the known-questionable API/ABI checker in pr-head only, see above), and no conflict or prerequisite, so it's ready for merge.

simonbutcher

LGTM

simonbutcher · 2019-10-29T14:05:41Z

programs/ssl/ssl_client2.c

 #endif

 #if defined(MBEDTLS_TIMING_C)
 #if !defined(MBEDTLS_SSL_CONF_SET_TIMER) && \
    !defined(MBEDTLS_SSL_CONF_GET_TIMER)
-    mbedtls_ssl_set_timer_cb( &ssl, &timer, mbedtls_timing_set_delay,
+    mbedtls_ssl_set_timer_cb( ssl, &timer, mbedtls_timing_set_delay,


Trivial - Alignment is out on the next line, but it's not important.

hanno-becker added enhancement mbed TLS team needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests labels Sep 20, 2019

hanno-becker requested review from mpg and artokin September 20, 2019 12:25

mpg requested changes Sep 23, 2019

View reviewed changes

mpg added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Sep 23, 2019

hanno-becker force-pushed the baremetal_minor_changes branch from 808f9f0 to 03e3751 Compare September 25, 2019 13:07

hanno-becker requested a review from mpg September 25, 2019 13:07

jarvte requested review from jarvte and removed request for artokin October 10, 2019 07:11

Hanno Becker added 3 commits October 10, 2019 11:47

ssl_client2/ssl_server2: Use heap for various structures

7d864c4

This commit modifies the example programs ssl_client2 and ssl_server2 to allocate various structures on the heap instead of the stack. This allows more fine-grained memory usage tracking via valgrind massif.

Make use of copy-less CRT parsing API in ssl_client2/ssl_server2

c828432

Reduce number of entropy sources to 1 in baremetal configuration

ead3aae

jarvte force-pushed the baremetal_minor_changes branch from 03e3751 to ead3aae Compare October 10, 2019 09:06

Fixed to compile: ssl_async_keys is not a pointer but was treated tha…

ff1ce26

…t way when giving to functions.

jarvte force-pushed the baremetal_minor_changes branch 2 times, most recently from 0d0cfe9 to 9697401 Compare October 15, 2019 11:16

Fixed memory leak in ssl_server2.c main

52c9ecb

Allocations are now done after command line parsing. Added more checks if allocations are needed and fixed baremetal tests with these defines.

jarvte force-pushed the baremetal_minor_changes branch from 9697401 to 52c9ecb Compare October 15, 2019 11:21

jarvte force-pushed the baremetal_minor_changes branch 2 times, most recently from 287f493 to d2a63ed Compare October 17, 2019 10:05

jarvte force-pushed the baremetal_minor_changes branch from d2a63ed to efba673 Compare October 17, 2019 10:45

jarvte requested review from jarlamsa and removed request for jarvte October 24, 2019 07:30

jarlamsa suggested changes Oct 24, 2019

View reviewed changes

configs/baremetal.h Outdated Show resolved Hide resolved

Fixed CI tests by overriding value MBEDTLS_ENTROPY_MAX_SOURCES

99082ae

Override define MBEDTLS_ENTROPY_MAX_SOURCES from 1 to 3 in baremetal_test.h mbedtls_entropy_init adds 2 sources already so max must be 3 so that one source can be added with mbedtls_entropy_add_source.

jarvte force-pushed the baremetal_minor_changes branch from 66b39c9 to 99082ae Compare October 24, 2019 12:06

jarlamsa previously approved these changes Oct 24, 2019

View reviewed changes

mpg requested changes Oct 25, 2019

View reviewed changes

programs/ssl/ssl_server2.c Outdated Show resolved Hide resolved

programs/ssl/ssl_client2.c Outdated Show resolved Hide resolved

mpg added needs-work and removed needs-ci Needs to pass CI tests needs-review Every commit must be reviewed by at least two team members, labels Oct 25, 2019

Moved allocation checks to correct place in ssl_server2.c

614efc2

jarvte dismissed jarlamsa’s stale review via 614efc2 October 25, 2019 08:03

Adjusted buf allocated from heap in ssl_client2 to be as small as pos…

7800719

…sible

mpg approved these changes Oct 28, 2019

View reviewed changes

jarvte requested a review from jarlamsa October 28, 2019 08:21

mpg added needs-ci Needs to pass CI tests needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Oct 28, 2019

jarlamsa approved these changes Oct 28, 2019

View reviewed changes

jarlamsa removed the needs-review Every commit must be reviewed by at least two team members, label Oct 28, 2019

mpg removed the needs-ci Needs to pass CI tests label Oct 28, 2019

mpg changed the title ~~[Baremetal] Various minor changes~~ [Baremetal] Various minor changes to ssl_client2/server2 Oct 28, 2019

mpg changed the title ~~[Baremetal] Various minor changes to ssl_client2/server2~~ [Baremetal] Various minor changes to improve RAM measurements Oct 28, 2019

mpg added the approved Design and code approved - may be waiting for CI or backports label Oct 28, 2019

simonbutcher approved these changes Oct 29, 2019

View reviewed changes

simonbutcher merged commit 7800719 into Mbed-TLS:baremetal Oct 29, 2019

mpg mentioned this pull request Aug 27, 2020

Identify improvements in baremetal branch that should be upstreamed #3535

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Baremetal] Various minor changes to improve RAM measurements #2850

[Baremetal] Various minor changes to improve RAM measurements #2850

hanno-becker commented Sep 20, 2019 •

edited by mpg

Loading

mpg left a comment

mpg Sep 23, 2019

hanno-becker Sep 25, 2019

hanno-becker commented Sep 25, 2019

jarvte commented Oct 10, 2019

jarvte commented Oct 16, 2019

jarvte commented Oct 17, 2019

jarlamsa left a comment

jarlamsa left a comment

mpg left a comment

mpg commented Oct 25, 2019

mpg left a comment

jarlamsa left a comment

mpg commented Oct 28, 2019

mpg commented Oct 28, 2019

simonbutcher left a comment

simonbutcher Oct 29, 2019

[Baremetal] Various minor changes to improve RAM measurements #2850

[Baremetal] Various minor changes to improve RAM measurements #2850

Conversation

hanno-becker commented Sep 20, 2019 • edited by mpg Loading

Changes to RAM measurements

mpg left a comment

Choose a reason for hiding this comment

mpg Sep 23, 2019

Choose a reason for hiding this comment

hanno-becker Sep 25, 2019

Choose a reason for hiding this comment

hanno-becker commented Sep 25, 2019

jarvte commented Oct 10, 2019

jarvte commented Oct 16, 2019

jarvte commented Oct 17, 2019

jarlamsa left a comment

Choose a reason for hiding this comment

jarlamsa left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

mpg commented Oct 25, 2019

mpg left a comment

Choose a reason for hiding this comment

jarlamsa left a comment

Choose a reason for hiding this comment

mpg commented Oct 28, 2019

mpg commented Oct 28, 2019

simonbutcher left a comment

Choose a reason for hiding this comment

simonbutcher Oct 29, 2019

Choose a reason for hiding this comment

hanno-becker commented Sep 20, 2019 •

edited by mpg

Loading