MBEDTLS_MEMORY_BUFFER_ALLOC_C feature in benchmark is problematic

[RonEld]

The MBEDTLS_MEMORY_BUFFER_ALLOC_C feature in the benchmark application is problematic, as it uses 64 KB of stack size. By default Mbed-OS uses 4 KB of stack size. This feature should either be removed, or length should be taken from the link script

[simonbutcher]

I don't think anyone using Mbed OS should need or want to use MBEDTLS_MEMORY_BUFFER_ALLOC_C. Why did this come up? My automatic answer would be don't do it. (And maybe we should document that).

[ciarmcom]

ARM Internal Ref: IOTSSL-2419

[RonEld]

This came up in this comment

I agree that anyone using Mbed OS should need to use MBEDTLS_MEMORY_BUFFER_ALLOC_C , this is why the descriptionsais:

This feature should either be removed, or length should be taken from the link script

[simonbutcher]

Ah! I was being dumb. I didn't realise you meant it was in benchmark.cpp. I thought you were talking about the library. That should indeed be removed.

[RonEld]

no problem, I can see how me description was confusing.

[RonEld]

After looking some more, I am not sure we want to remove the call for MBEDTLS_MEMORY_BUFFER_ALLOC_C in benchmark.c The feature is also used for memory statistics, measured in MEMORY_MEASURE_PRINT, which we may want to utilize for debugging

[andresag01]

@RonEld: Mbed OS has its own facilities for getting memory statistics for both stack and heap. IMO is better to use those than try to fix this feature ourselves in Mbed OS. As far as I remember, we have never used MBEDTLS_MEMORY_BUFFER_ALLOC_C in Mbed OS and in any case it will cause a stack overflow the way it is currently implemented in this example because its trying to allocate statically a huge object in the stack. I think this is probably a hang over from the original benchmark example in the library and this memory part was just meant for large desktop machines (just like the idea of having a command line interface...).

[RonEld]

@andresag01 Yes, Mbed-OS has its own facilities, but you can't differentiate between Mbed TLS memory usage and other features.
I am aware that MBEDTLS_MEMORY_BUFFER_ALLOC_C was probably not used in Mbed-OS, but I was raising a thought that perhaps we would like to keep it, for debugging purposes.
Obviously, the buffer size should change, perhaps according to scatter list

[andresag01]

@RonEld

Yes, Mbed-OS has its own facilities, but you can't differentiate between Mbed TLS memory usage and other features.

I would definitely consider this if we were talking about a relatively large application with multiple threads and other modules (other than Mbed TLS) being used. However, the benchmark example is just a simple application running Mbed TLS tasks sequentially (and nothing else) and there should be almost no interference wrt performance or memory from other parts of the system. I personally think that we should aim to simplify this examples and reduce the maintenance cost instead of adding to it.

I am aware that MBEDTLS_MEMORY_BUFFER_ALLOC_C was probably not used in Mbed-OS, but I was raising a thought that perhaps we would like to keep it, for debugging purposes.
Obviously, the buffer size should change, perhaps according to scatter list

If we really want to keep this and if you mean "scatter file" as in linker script, then I would prefer to not rely on this as I am not sure that different platforms export the same symbols. I think that we probably just need to use some Mbed OS facilities to check how much memory there is (if possible) and then dynamically allocate a huge buffer using regular malloc, then configure the allocator.

[andresag01]

@sbutcher-arm, @RonEld: I think this was fixed by #194. Should we close this issue