zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.66k stars 6.52k forks source link

Avoid using extra net buffer for L2 header #50188

Open krish2718 opened 2 years ago

krish2718 commented 2 years ago

https://github.com/zephyrproject-rtos/zephyr/commit/94e89a11b7151c9f8dd8c22007e64bd30ee8b809 added change to use a dedicated net buf for the L2 header (ethernet) in this case, and as commit says it was meant to be temporary, as this unnecessarily wastes a net buffer (esp. if net buffer size is increase to 1024, we end up using only 14 bytes for Ethernet L2 header).

This should be optimized use a Linux headroom style concept to reserve memory to manipulate headers. A packet should only use a single net buffer to avoid memcpy when we read the packet in the L2 driver.

krish2718 commented 2 years ago

@sachinthegreen @rlubos FYI.

rlubos commented 2 years ago

@jukkar The commit message states that "this is the first step towards getting rid of that ll reserve concept everywhere.". I see the commit is pretty old, but do you recall perhaps what was the reasoning behind such an approach?

jukkar commented 2 years ago

Before that commit we tried to have Ethernet header in front of the L3 headers in the same net_buf. This is a bit problematic as we would need to calculate how many bytes the L2 header needs, and that is difficult to know in all the cases and then reallocate / reshuffle the buffers etc (see that commit how it re-allocates the buffers before the change). Easiest solution was to allocate a new net_buf just for L2 header and place that as a first net_buf in the net_pkt frag chain. Of course this wastes lot of memory if you have 1k net_buf's. In this case one could use variable size data buffers by enabling CONFIG_NET_BUF_VARIABLE_DATA_SIZE option. This way the net_buf allocator would allocate the data part of net_buf using just enough space that the data requires.

krish2718 commented 2 years ago

I tried using CONFIG_NET_BUF_VARIABLE_DATA_SIZE but it doesn't event build as CONFIG_NET_BUF_DATA_SIZE is used in file tcp.c, net_pkt.c..

krish2718 commented 1 year ago

I tried to get the build going by hardcoding the CONFIG_NET_BUF_DATA_SIZE but still the packets are being dropped at L2 Ethernet at https://github1s.com/zephyrproject-rtos/zephyr/blob/HEAD/include/zephyr/net/buf.h#L1269-L1270 called from https://github1s.com/zephyrproject-rtos/zephyr/blob/HEAD/subsys/net/l2/ethernet/ethernet.c#L512 . Shouldn't we be using variable net_buf here instead of fixed?

rlubos commented 1 year ago

I tried to get the build going by hardcoding the CONFIG_NET_BUF_DATA_SIZE but still the packets are being dropped at L2 Ethernet at https://github1s.com/zephyrproject-rtos/zephyr/blob/HEAD/include/zephyr/net/buf.h#L1269-L1270 called from https://github1s.com/zephyrproject-rtos/zephyr/blob/HEAD/subsys/net/l2/ethernet/ethernet.c#L512 . Shouldn't we be using variable net_buf here instead of fixed?

I've been investigating the issue recently and I have similar initial conclusions, the Ethernet layer appears to be using net_pkt_get_frag() for header fragment allocation, which internally relies on fixed buffers, it obviously seems wrong for the variable buffer length configuration. I don't have a fix yet, need to investigate further.

krish2718 commented 1 year ago

@jukkar @rlubos thanks for fixing the variable length net buf configuration, but we still need to discuss about using a separate net buf for L2, as this forces the drivers to make a memcpy causing performance degradation, we need a linux skb headroom style implementation.

rlubos commented 1 year ago

Putting the case of how to allocate space for L2 header aside for a moment, I think that the driver always needs to be prepared to receive fragmented data, having the buffer large enough to fit network MTU will not always be the case. It might make sense to declare large buffers for throughput testing, but imagine a scenario where Wi-Fi runs along some 802.15.4 protocol. The networking net_buf pool is global for the system, so it doesn't make sense to set large fragment size as 802.15.4 protocol would waste ~90% of the buffer space. In this case, we need to tradeoff performance over memory usage, and the driver should be prepared for such a scenario.

krish2718 commented 1 year ago

Agreed, in the specific case we can sacrifice the performance over compatibility, but for a standalone Wi-Fi case we should aim for zero-copy (which is what we get in Linux), esp. on a low end CPU it takes about 20-40us for ~1K buffer. It's easier to check for number of fragments and then decide whether to do memcpy or not to support both cases, but we should be aiming for zero-copy in the primary usecase.

krish2718 commented 6 months ago

This feature is now being worked on in https://github.com/zephyrproject-rtos/zephyr/pull/71400