eth_sam_gmac driver (and BOARD=sam_e70_xplained using it) sets unduly high memory requirements on the IP stack overall

[pfalcon]

eth_sam_gmac is quite an advanced Ethernet driver, the only one using DMA, etc.

But at the same time it does something which no other Ethernet (or non-Ethernet) driver does: it enforces at compile-time (using #error directives) high limits on the number of fragment buffers used by the IP stack (CONFIG_NET_BUF_RX_COUNT, CONFIG_NET_BUF_TX_COUNT): https://github.com/zephyrproject-rtos/zephyr/blob/master/drivers/ethernet/eth_sam_gmac.c#L43 . This has the following negative effect on the entire IP stack and Zephyr overall:

Tests/sample apps which perfectly build for any other board, break when building for boards using the eth_sam_gmac driver, e.g. BOARD=sam_e70_xplained. As part of Zephyr CI, we build tests/samples for as many boards as possible, so this negatively affects Zephyr CI overall.

As a response to it, there are attempts to work around eth_sam_gmac's behavior by bumping CONFIG_NET_BUF_RX_COUNT, CONFIG_NET_BUF_TX_COUNT higher, e.g. a0df4f6, #8966, etc. (there were definitely more sam_e70_xplained specific commits like that). There're however following issues with such workarounds:

1. It negatively affects available memory on smaller boards. We probably didn't face cases of running out of RAM just yet, but can very easy start to see with more complex examples integrating other memory-hungry code (e.g. TLS).
2. It adversely affects IP stack performance/stability (which is due to bugs in the IP stack, but nonetheless, eth_sam_gmac's behavior complicates behavior here and now for many other boards). An example is Revert "samples: net: Fix sanitycheck for sam_e70_xplained board" #7831 .

This ticket is created to capture the concern, and consider various ways out of the situation.

[pfalcon]

@nashif, @jukkar : Let's take the discussion from #8966 (comment) into this ticket.

[jukkar]

I think the issue is not really in sam-e70 ethernet driver. It is just trying to set a lower limit of how many network buffers are configured so that the driver can receive a full ethernet frame. To me this sounds like a sane thing to do.

What is more questionable, is that various unit tests in tests/net are compiled and run in atmel board which then fails as there is not enough buffers. IMHO running these tests in this hw (or any other real hw) is not really needed as we are running these tests in qemu and native_posix which is enough.

[mnkp]

Drivers like eth_dw or eth_mcux reserve a private RX buffer in RAM - one continuous chunk - large enough to hold at least one full Ethernet frame. Upon receiving a frame they copy it form this private RX buffer to the RX fragment buffers provided by the networking stack. E.g. eth_mcux.c reserves the private RX buffer on line 135

static u8_t __aligned(ENET_BUFF_ALIGNMENT)
rx_buffer[CONFIG_ETH_MCUX_RX_BUFFERS][ETH_MCUX_BUFFER_SIZE];

here by default RAM for two full Ethernet frames is reserved.

In contrary eth_sam_gmac is a zero-copy networking implementation of an Ethernet driver as documented in the file header. It has no private RX buffer and places all the incoming data directly in RX fragment buffers provided by the networking stack. Therefore its requirements on the amount of available RX fragment buffers are higher than for the other drivers.

The minimum buffer requirements are ensured by the compile time checks, like the one on line 43. It is vital that the checks are active.

Since they were disabled for unit tests it resulted in the bug report: #8963.

[mnkp]

Of course, just because eth_sam_gmac requires higher amount of RX buffers we should not force remaining drivers to do the same. Maybe sane values could be set by a SoC specific _defconfig file?

[mnkp]

To resolve this issue we need to decide how to define a 'reasonable' amount of network buffers for a given Ethernet driver without forcing all the drivers to use the same values. This issue should be probably taken over by @jukkar or @tbursztyka. I removed myself from 'Assignees' list, won't be able to help here.

[mnkp]

PR #8966 merged commits which ensure that networking tests are run in qemu_x86 or native_posix only. But we probably still need a solution for all the sample projects? If not this issue can be closed.

[pfalcon]

@mnkp: Thanks for the responses! I definitely have a bunch of alternatives to present for this could be solved (addressed). But I'm just back from vacation and going thru the backlog, and probably straight into the 1.13 release rush, so please give me some time to prepare the response. Thanks!

[pfalcon]

Thanks for patience to all participants of this discussion, let me get back to this and answer comments so far:

@jukkar

I think the issue is not really in sam-e70 ethernet driver. It is just trying to set a lower limit of how many network buffers are configured so that the driver can receive a full ethernet frame. To me this sounds like a sane thing to do.

Please note that while full Ethernet frame length is 1500 bytes indeed, a particular end-to-end application does not have to use full length. It's perfectly possible to send only 200-bytes packets between 2 Ethernet peers, and while a definitely limit and peculiar setup, it may have its uses too. As a real-world example, default TCP MSS (max segment size) is 576, and unless another MSS is negotiated, that would be used, leading to 600-something bytes Ethernet packets.

With that in mind, the current situation we have is:

1. Zephyr IP stack doesn't enforce max size of Ethernet packet for Ethernet drivers (i.e. enables scenarios above).
2. Majority of Ethernet drivers doesn't do that either.
3. However eth_sam_gmac gets out of the line here, and forces requirement of full Ethernet frame. Worse, due to the way our config system works, this requirement propagates to the rest of Ethernet drivers. I.e., if you have an app config which works well for majority of Eth drivers, it may fail to even build for eth_sam_gmac. To make it even buildable on eth_sam_gmac, you need to increase number of net_buf's, which may lead to wasted memory for other hw drivers.

And the state objectives I'd like to achieve by opening this ticket are:

a) Preserve the current status quo of Zephyr IP stack not applying extra hard constraints, like minimum # of buffers based on
b) Find a way to avoid a backpressure of forcing such high constraints from drivers like eth_sam_gmac.

What is more questionable, is that various unit tests in tests/net are compiled and run in atmel board which then fails as there is not enough buffers. IMHO running these tests in this hw (or any other real hw) is not really needed as we are running these tests in qemu and native_posix which is enough.

Based on the above, this is somewhat orthogonal matter. Also, there're 2 parts in it: be able to compile sample for a particular hardware, and be able to actually run. I think compilability is very important, it's basis of our CI. If we consciously exclude some boards from build tests, it will lead to non-detactable cases like #9343 . Running on the other hand is "2nd level" goal (indeed, if it runs on common platforms like qemu, then it should run on nay platform, modulo peculiarities of platform's driver, which peculiarities may also include high requirements for buffers).

This gives one of the option on how to resolve the current issue: Move the net_buf check from compile time into runtime. Then we still will CI the drivers for buildability, but users of this particular driver will need to have suitable app config to actually use it on hardware.

But there're other alternatives to resolve this issue. Let me try to prepare more or less complete list, and I hope we with @mnkp can find the best compromise and solution.

[pfalcon]

Ok, list of different ways this issue could be solved (@mnkp listed a few of ways in this list already - I'm glad we're on the same line). They're roughly sorted from least preferred to the most preferred.

1. Exclude boards with eth_sam_gmac from default sanitycheck runs - that's of course something which everyone wants to avoid.
2. Set the "right" number of buffers in _defconfig for a SoC/board. This option was already mentioned by @mnkp in eth_sam_gmac driver (and BOARD=sam_e70_xplained using it) sets unduly high memory requirements on the IP stack overall #9015 (comment). The problem is that it's not flexible: ideally, we'd want to override default Kconfig setting with _defconfig, yet allow a user to override that with prj.conf, but I'm not sure it can work like that. (Ping to @ulfalizer for both question and an example of usecases users may be interested in.)
3. Move check for enough buffers from compile-time to runtime. I.e., when drivers initializes, it prints error with clear guidelines how to get it working. (This error would be disabled in "production" config build, e.g. with asserts disabled or whatever.)
4. Increase net_buf size just for eth_sam_gmac to accommodate larger size of Ethernet frame.
5. Zephyr networking stack has ability to define customer buffer pools. And that would be direct counterpart of what @mnkp writes in eth_sam_gmac driver (and BOARD=sam_e70_xplained using it) sets unduly high memory requirements on the IP stack overall #9015 (comment) : "Drivers like eth_dw or eth_mcux reserve a private RX buffer in RAM - one continuous chunk - large enough to hold at least one full Ethernet frame." Indeed, they do, effectively they has "customer buffer pool" as a flat memory. eth_sam_gmac, being more advanced driver, can achieve the same effect by defining its own receive buffer pool, and allocate buffers for received frames strictly from it. That will ensure it can always receive a complete Ethernet frame. No special care about send buffers is required - a user who wants to send enough data in a complete frame would have enough buffers in their configs, otherwise they want to send less data at once.

As can be seen, p.5 looks like the right way to solve this issue, and directly corresponds to the "machinery" used in other drivers (just more adapted to Zephyr's way of working, up to ensuring zero-copiedness).

@mnkp, I hope you would agree with this analysis, and add such a refactor of the driver to your queue. (I for example don't have required hardware for testing. But if you think that "I develop, you test" cooperation would be beneficial for this case, we surely can give it a try.) Thanks.