lwIP: Add memory configs to JSON #5250
Conversation
|
This will increase default RAM footprint for most devices using Ethernet, but I think we may be able to claw that back. Most Ethernet targets seem to be setting MEM_SIZE unduly high. Not going to try reducing that here though. Also, introduction of EMAC work will need more attention paid later about how to tune the networking memory - gets a bit trickier with an abstraction layer in. |
|
@theotherjimmy - tag please |
ghost
added
the
|
Looks like you got it yourself! |
|
/morph test-nightly |
|
This increase is on the order of 7k RAM. How do we unify this for PPP, eth and wifi drivers? Does this only affect odin Wi-Fi or ethernet as well? Is this a point solution for the driver not having proper buffering? |
|
And how about a test case to show a transaction overflowing the buffer to make sure this is handled properly in the future? |
|
We do have a DTLS handshake test, although the timeout values must be wrong for this board. @kjbracey-arm, are you able to update the timeouts to a value that shows the problem? IMO it's fine to have target specific values in this case. I agree with @sg- this shouldn't come in until there's a test verifying the problem. Here's the DTLS handshake test: @sarahmarshy, sorry to pull you in, but with the new configurable tests do you know how we specify U-Blox Odin Ethernet? It'd be something like Also the host pc and board will need to be able to send messages to each other. Also, @studavekar or myself, we need to double check that the netsocket tests are running in CI since the configuration update. The DTLS handshake has given us a massive amount of problems before, and unfortunately, increasing buffers is the only fix we have available on the device side. See #1596 from a little under 4000 issues ago. Also note the DTLS handshake is still preventing cellular boards like the C027 from supported DTLS. I do think this increase might be a bit overkill, but it only effects the Odin board, which has 256KB of RAM anyways. A more permanent fix would involve tinkering with the DTLS spec itself. If we could increase the time between packets in the handshake (exponential backoff?) it should be possible for a lot more of the mbed enabled devices to support DTLS. This really needs to happen. |
| @@ -72,6 +72,14 @@ | |||
| "help": "Maximum number of open UDPSocket instances allowed, including one used internally for DNS. Each requires 84 bytes of pre-allocated RAM", | |||
| "value": 4 | |||
| }, | |||
| "pbuf-pool-size": { | |||
| "help": "Number of pbufs in pool - usually used for received packets, so this determines how much data can be buffered between reception and the application reading. If a driver uses PBUF_RAM for reception, less pool may be needed", | |||
| "value": 16 | |||
There was a problem hiding this comment.
Oh. Actually this isn't an Odin specific change. Can we restrict this change to just the Odin board for now? That would help minimize the impact to applications above us.
We probably shouldn't increase this value globally unless it's a minor release. That's just asking for trouble.
|
The DTLS handshake is a recurring problem in many many environments - partly due to the spec, and partly due to mbed TLS's network-unfriendly implementation (always transmits 1 TLS record per datagram, no packing, no fragmentation, and no out-of-order reception handling). Given all those, increasing the backoff doesn't usually help much, and didn't help in this case - it would repeatedly receive only the first 4 datagrams of each handshake flight attempt, never the fifth. Having the pbuf pool default to 5 in mbed OS, on a platform that is trying to do DTLS, when lwIP defaults to 16, feels like premature optimisation. It does work with drivers that happen to not use PBUF_POOL for reception, like the K64F, but then they end up configuring the "mem" heap size huge for the PBUF_RAM buffers they allocate. (Conversely, many drivers seem to be setting MEM_SIZE larger than should be necessary, given that they're actually using PBUF_POOL. I think there's scope for a counterbalancing drop of 8K on most platforms there). I think 8 pbufs would be enough with the current handshake I'm doing, but I believe they're plotting bigger certificates... These tuning parameters do need to be in the JSON, so the applications can mess with them - what you need for tuning ultimately depends on both the application and the target. But at the minute that setting is per-target only, with a default dangerously low for typical uses unless you have an Ethernet driver that doesn't use PBUF_POOL. For a patch release, it would make sense to not change the default, and only increase it now for the Odin W2 board, as that's the one failing. Apparently the Nucleo F429ZI isn't. I currently can't explain that, given that it's the same driver and configuration shared for all STM32F4 boards. Having increased this setting, the Odin W2 does still fail, but for reasons unknown - everything is received and all output now seems correct, but the server doesn't replay. It is possible that the packet loss due to lack of pbufs only happened with extra mbed-client tracing, and lack of pbufs was never part of the original problem. I'll go back to original tracing levels and double check that - if so, any change can wait until minor release or EMAC update. There may be some scope for Ethernet driver improvement - the STM driver has static RX buffers, and on an "RX complete" attempts to copy from them into a pbuf. If no pbufs available, the packet is dropped. This means the maximum burst it can accept is something like min(PBUF_POOL_SIZE, RX_BUF_SIZE). If on pbuf exhaustion it could hold stuff in the RX buffer and attempt a copy again later, it could accept a PBUF_POOL + RX_BUF_SIZE surge. |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
OutputAll builds and test passed! |
|
I've confirmed that PBUF_POOL_SIZE=5 does receive the DTLS handshake flight OK without any tracing added added to mbed client or mbed TLS. mbed TLS tracing immediately kills it. Which explains how the Nucleo board can work. So I think we don't need this for a patch, we can consider what to do for the next minor release. |
|
Update - in 5.6.1, it does not receive the flight successfully, even though 5.6.0 seemed to. Possibly the difference is the HW crypto - 5.6.0 had hardware crypto enabled, so was fast enough to receive the packets, but it did the crypto wrong. 5.6.1 has software crypto, so isn't fast enough to receive the packets, but if it did it would do the crypto right. Looks like we're going to need to increase the pbufs for at least the Odin. But how does the Nucleo F429ZI work? Is it clocked faster than the Odin W2? |
cc @adustm |
|
Okay, reworked to be something that should be viable as a patch - increases PBUFs from 5 to 16 for STM Ethernet, and reduces MEM_SIZE by a corresponding amount (from 25K to 16K), which should be fine given that it's not using PBUF_RAM for reception. No changes for other platforms. Description updated with new commit message. |
kjbracey
changed the title
|
Had a look at the DTLS handshake test - don't think it's quite realistic enough because you're not doing enough work on the packets. Adding a delay after each recvfrom would probably reveal the issue. Also, the "certificate" there is quite a small 384 - in our test it was 699, making it an extra pbuf. That 1 extra could have made all the difference. Haven't tried it yet, but those 2 changes combined should reproduce it. |
|
@sg- Are you happy with this now. Tuomo is saying this is critical for client that this makes 5.6.2. |
|
The bug this is fixing is still missing a test, but I don't know if that should block this pr. Nonblocking question: |
|
@geky Have your comments been addressed also ? |
|
@adbridge see above, I beat you by 32 seconds 😆
|
It does, but they arrive in a rapid burst. And 5 pbufs is not enough to hold the burst - they're acting as the socket buffer between the driver and the application. What appears to be happening is that when mbed TLS spends too long processing the 1st packet before doing the recvfrom to get the 2nd packet, the 5th packet overflows from the system. mbed TLS doesn't have any extra holding buffers of it own - it only holds one datagram and does another recvfrom when it's consumed and processed all of the previous one. |
|
We're now having conflicting information though - some new tests suggest increasing the main stack size makes 5.6.1 work. This was done to avoid an unrelated corruption assert on ARMCC, but there's no actual evidence of a stack overflow in GCC. Which means maybe the "release" 5.6.1 Ublox UDP failure is some sort of other memory alignment/layout issue, and the buffer size increase is only affecting the thing indirectly - the problem goes away if either the stack size increase or the pbuf pool size increase shifts something. The pbuf overflow issue was clearly seen with tracing enabled and slowing mbed TLS down. But maybe it's not what's happening with trace at normal levels. Symptoms on Wireshark look the same though. |
|
retest uvisor |
|
Latest update - the timing really is marginal. In a "develop" build with 5.6.1, the DTLS handshakes complete 90%+ of the time. In a "debug" build, they work about 20-30% of the time. The randomness appears to have added to the confusion in testing. I was using debug profile to allow me to breakpoint after the trace was removed, and that meant it was still too slow. I believe the stack increase is a red herring - the automated test changed to success after simultaneously updating to 5.6.1 and increasing the main stack - it was actually 5.6.1 turning off the hardware crypto that made the difference. So, this is only needed as a 5.6.y patch if we are concerned about the system breaking when |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
OutputAll builds and test passed! |
|
I think we also now have an explanation for the Nucleo "difference" - it never had the acceleration bug in 5.6.0, and I never investigated it that hard, so never produced a debug build for 5.6.0 or 5.6.1. Testing now with a debug build of 5.6.1, Nucleo seems to exhibit the same 70% failure rate as the U-blox. I have now seen a few failures in a "develop" build where my "whoops no pbufs" error went off in the DTLS handshake, on both Ublox and Nucleo. My error was inadvertantly killing the system though - I believe it would normally lead to a retransmit that works on the second attempt, meaning that the overall success rate on develop including retries is over 90%. |
|
That's an unfortunate side effect of only testing the develop profile. 90% success isn't great, are you still thinking we should get this in on a patch release?
If you increased the time between the packets in the burst, you could give mbed TLS enough time to process a datagram and get to the next recvfrom, could you not? |
|
Not sure if the remaining issue is severe enough for a patch. Maybe would could at least add the JSON for tuning for a patch release, without changing the default? Then people using DTLS could easily tweak the settings up. Modifying the cloud server to pace its transmissions would certainly help. At the minute it sends as fast as it can. The handshake burst of 5 packets is faster than a conventional starting TCP connection would do (2-4 packets), and hence more prone to failure. And it doesn't reduce the burst size on a retransmit like TCP would (to 1). Due to congestion effects, it's vaguely possible that a paced burst could still arrive in 1 fairly sudden spike even if paced at the server - they could have got held up in a queue somewhere, eg due to address resolution, then suddenly arrive at 100Mbit/s Ethernet line rate. Unlikely to happen twice in a row though, I guess. TCP doesn't slow itself down by timed pacing - it slows itself down by reducing unacked-packets-in-transit. And its minimum for that is 1, unlike DTLS's handshake which can't get an ack until 5 records are received. DTLS could also be made more friendly by putting those 5 TLS records in fewer IP packets - they would fit in 1 or 2 here if packed. |
We currently set the lwIP pbuf pool size small - to 5 x 576-byte buffers. This is insufficient to hold a single DTLS handshake flight, so can cause cloud client connections to fail. STM-based platforms are failing handshake because of this. (K64F works because it doesn't use the pbuf pool for reception, but lwIP does recommend drivers use the pbuf pool). Not changing the default memory sizes here, as intended for a patch release, but adding mbed configuration options to allow the numbers to be adjusted for memory tuning in an application. In a future minor revision, I would recommend increasing the default PBUF_POOL_SIZE - we are well below lwIP's out-of-the-box default - and offsetting by a reduction in MEM_SIZE for the drivers that don't use PBUF_RAM.
kjbracey
changed the title
|
PR reduced to just adding the JSON memory config options, so it's suitable for a patch release. |
Open for review , @geky @SeppoTakalo |
|
Looks good to me. This sort of thing makes me feel that we need a new chapter to our Handbook describing all the tuning parameters of Mbed OS. There are now pieces here and there that some need to know to get most of it. |
|
@SeppoTakalo yes please. The Configuration section is sorely lacking. |
Build : SUCCESSBuild number : 107 Triggering tests/test mbed-os |
👍 @AnotherButler for visibility |
|
@SeppoTakalo There are plans for added documentation to the configuring section. We have JIRA tickets for the addition of configuration options from generated json. If this something you can work on, I'd be happy to tag you in those tickets. |
Test : SUCCESSBuild number : 65 |
We currently set the lwIP pbuf pool size small - to 5 x 576-byte
buffers.
This is insufficient to hold a single DTLS handshake flight, so can
cause cloud client connections to fail. STM-based platforms are failing
handshake because of this. (K64F works because it doesn't use the pbuf
pool for reception, but lwIP does recommend drivers use the pbuf pool).
Not changing the default memory sizes here, as intended for a patch
release, but adding mbed configuration options to allow the numbers to
be adjusted for memory tuning in an application.
In a future minor revision, I would recommend increasing the default
PBUF_POOL_SIZE - we are well below lwIP's out-of-the-box default - and
offsetting by a reduction in MEM_SIZE for the drivers that don't use
PBUF_RAM.