New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lwIP: Add memory configs to JSON #5250

Merged
merged 1 commit into from Oct 13, 2017

Conversation

Projects
None yet
@kjbracey-arm
Contributor

kjbracey-arm commented Oct 4, 2017

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-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 4, 2017

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.

@ChiefBureaucraticOfficer

This comment has been minimized.

ChiefBureaucraticOfficer commented Oct 4, 2017

@theotherjimmy - tag please

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Oct 4, 2017

Looks like you got it yourself!

@geky

geky approved these changes Oct 4, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Oct 4, 2017

/morph test-nightly

@sg-

This comment has been minimized.

Member

sg- commented Oct 4, 2017

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?

@sg-

This comment has been minimized.

Member

sg- commented Oct 4, 2017

And how about a test case to show a transaction overflowing the buffer to make sure this is handled properly in the future?

@geky

This comment has been minimized.

Member

geky commented Oct 5, 2017

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:
greentea test - https://github.com/ARMmbed/mbed-os/blob/master/TESTS/netsocket/udp_dtls_handshake/main.cpp
host test - https://github.com/ARMmbed/mbed-os/blob/master/TESTS/netsocket/host_tests/udp_shotgun.py

@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 mbed test -m ODIN -t GCC_ARM -n tests-netsocket-udp_dtls_handshake.

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.

features/FEATURE_LWIP/lwip-interface/mbed_lib.json Outdated
@@ -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

This comment has been minimized.

@geky

geky Oct 5, 2017

Member

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.

@geky
  • needs test update
  • needs odin specific

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Oct 5, 2017

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 5, 2017

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.

@mbed-bot

This comment has been minimized.

mbed-bot commented Oct 5, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1524

All builds and test passed!

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 5, 2017

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.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 5, 2017

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?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 5, 2017

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

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:pbuf16 branch Oct 5, 2017

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 5, 2017

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-arm kjbracey-arm changed the title from Increase default lwIP pbuf pool size from 5 to 16 to Give STM more pbufs; add memory configs to JSON Oct 5, 2017

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 5, 2017

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.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 5, 2017

@sg- Are you happy with this now. Tuomo is saying this is critical for client that this makes 5.6.2.

@geky

This comment has been minimized.

Member

geky commented Oct 5, 2017

The bug this is fixing is still missing a test, but I don't know if that should block this pr.

Nonblocking question:
So this fixes STM, does that mean Odin still has a problem?

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 5, 2017

@geky Have your comments been addressed also ?

@geky

This comment has been minimized.

Member

geky commented Oct 5, 2017

@adbridge see above, I beat you by 32 seconds 😆

The bug this is fixing is still missing a test, but I don't know if that should block this pr.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 6, 2017

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.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 6, 2017

retest uvisor

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 6, 2017

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 --profile=debug is selected. It could maybe just go in a release note.

@mbed-bot

This comment has been minimized.

mbed-bot commented Oct 6, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1538

All builds and test passed!

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 6, 2017

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%.

@geky

This comment has been minimized.

Member

geky commented Oct 6, 2017

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?

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.

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?

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 9, 2017

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.

lwIP: Add memory configs to JSON
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-arm kjbracey-arm force-pushed the kjbracey-arm:pbuf16 branch to cd06ebb Oct 10, 2017

@kjbracey-arm kjbracey-arm changed the title from Give STM more pbufs; add memory configs to JSON to lwIP: Add memory configs to JSON Oct 10, 2017

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 10, 2017

PR reduced to just adding the JSON memory config options, so it's suitable for a patch release.

@0xc0170 0xc0170 added needs: review and removed needs: work labels Oct 10, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 10, 2017

PR reduced to just adding the JSON memory config options, so it's suitable for a patch release.

Open for review , @geky @SeppoTakalo

@geky

geky approved these changes Oct 10, 2017

Looks good to me

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Oct 11, 2017

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.

@40Grit

This comment has been minimized.

40Grit commented Oct 11, 2017

@SeppoTakalo yes please.

The Configuration section is sorely lacking.
Also related #5225

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 11, 2017

Build : SUCCESS

Build number : 107
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5250/

Triggering tests

/test mbed-os

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 11, 2017

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.

👍 @AnotherButler for visibility

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Oct 11, 2017

@AnotherButler

This comment has been minimized.

Contributor

AnotherButler commented Oct 11, 2017

@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.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Oct 13, 2017

@theotherjimmy theotherjimmy merged commit 926ed13 into ARMmbed:master Oct 13, 2017

5 checks passed

Cam-CI uvisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment