net_pkt buffer management and API changes

[tbursztyka]

This is in a early stage, some stuff will change in these patches still.

In order not to break anything (besides some test due to ROM/RAM being bloated for the time being), the API is made in parallel to the existing one (thus the bloat). Currently, as the patches can tell you, only part of ipv4/Ethernet are moved towards this new API. The good thing with this approach is that old API works on packet generated with the new API and vice versa so changes can be made iteratively.

[zephyrbot]

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@941f435). Click here to learn what that means.
The diff coverage is 51.62%.

@@            Coverage Diff            @@
##             master   #11775   +/-   ##
=========================================
  Coverage          ?    54.6%           
=========================================
  Files             ?      235           
  Lines             ?    26570           
  Branches          ?     6374           
=========================================
  Hits              ?    14509           
  Misses            ?     9404           
  Partials          ?     2657

Impacted Files	Coverage Δ
subsys/net/ip/net_private.h	67.74% <ø> (ø)
subsys/net/lib/dns/resolve.c	33.57% <ø> (ø)
include/net/net_ip.h	68.13% <ø> (ø)
include/net/net_context.h	78.84% <ø> (ø)
subsys/net/ip/dhcpv4.c	6.14% <0%> (ø)
subsys/net/ip/tcp_internal.h	45.71% <0%> (ø)
drivers/ethernet/eth_native_posix.c	30.3% <0%> (ø)
subsys/net/ip/net_core.c	63.87% <100%> (ø)
include/net/net_pkt.h	85.08% <100%> (ø)
subsys/net/ip/net_if.c	60.4% <100%> (ø)
... and 17 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 941f435...76fc11e. Read the comment docs.

[pfalcon]

@tbursztyka, how this relates to #11374 ?

[tbursztyka]

@pfalcon There are 2 relations and 1 big reason
1 indirect relation: In current buffer management, it's up to L3 to allocate L2 buffer space. That could be fine if all bearers had a fixed amount of space required. But it's not. 802.15.4 has a variable buffer space requirement, depending on many parameters which I am not listing here, such buffer space cannot be predicted until you know exactly where the packet is meant to go. Which information is known only after IPv6 ND process. And thus forces you to update this space (it might shrink or grow).
So properly splitting L2 to L3 fixes this. Once a packet reaches L2, L3 has all the info where it is supposed to go (it ran ND already etc..). So L2 will be able to compute, once, the exact amount of buffer required and do its job. For this new buffer management, it was necessary to fix this to simplify the whole process a lot. L3 has to take care about L3 only.
1 direct relation: the compatibility of old vs new API on complete packet (received or freshly created ones for tx). Due to this ll buffer space being allocated on L3, and because then L3 did not care about it, it required to "hide" this space via net_buf_pull(). But then on l2 and in each and every drivers, it of course needed to retrieve that space (via the pkt->frags->data - net_pkt_ll_reserve(pkt). And only the first "frag" had its reserved space used relevantly, so it need to be handled specifically (some ethernet drivers where having comments describing this). And that would have been an issue for using new API net_pkt_read/net_pkt_write or else, since these do not know if you need to access this "hidden space" or not.

And 1 big and obvious reason: Optimizing buffer usage. This ll reserve thingy was thus mixed in L3, which architecturally speaking was bad already. But it was even worse in net_buf allocation mechanism: all struct net_buf had it's size shrunk by the ll_reserve amount. So if your frag size is 128bytes, used to built a big ethernet frame you would loose 14 bytes on all fragments but the first one.

Side note: the "frag" size was also, due to this weird L3/L2 mix-up tight to the bearer which had the smallest MTU. And that was again a weird thing, since buffer management is memory management: it should not have any relationship with what bearers is used or else. Theoretically you should be able to go as low as 1 byte per-net_buf and all should work just the same. (That would not make any sense in practice obviously, having a descriptor such as a struct net_buf n times bigger than the buffer it points to)

Last but not least: doing that split, offers other optimizations possibilities like in ethernet: you could have a dedicated net_buf for the ethernet header part, allocated on stack, with just 14 bytes as buffer etc... On 15.4 it will be also possible then to avoid modifying the original packet (6lo and 15.4 fragmentation were doing so before), and thus avoid - when TCP is used - to copy the whole packet in TCP for possible needed retries etc...

Also, an additional reason of doing this PR is to hide all the ugly buffer details to most of the code. We have these ugly stuff everywhere:
frag = pkt->frags;
while (frags) {
...
frag = frag->frags;
}

Instead we should not do any of it anywhere, you just write/read stuff from your packet. (and in case of network headers, you just access a header pointer directly).

[jukkar]

Lot of commits but fortunately most of them were small and readable.

[jukkar]

typo s/reserver/reserve/ in commit "net/pkt: Let's ignore ll reserve and use 0 instead"

[jukkar]

Hmm, the code above will check ll headers. I wonder if things change after ll reserve has been removed. I can try to run this sample to see if any other changes are needed.

[jukkar]

Same with this one.

[jukkar]

What about making the function name a bit shorter so I propose we change this to net_pkt_alloc()

[tbursztyka]

true. I had that name at first, I don't know why I went for the plain allocate word.

[jukkar]

Shouldn't we call this net_pkt_tx_allocate() in order to follow the same syntax as the rx allocator one?

[tbursztyka]

the rx stuff is an in-between name. Idea would be to have only 1 allocator for all pkt (thus one slab, etc...). However, we will still be able to know (for stats, net mem in shell etc...) if a pkt was allocated for tx or rx, there will be a bit in net_pkt to give that info and so on (net_recv_data will set it for rx, tx will be default). That's not going to be part of that PR though,

[jukkar]

what about getting max_len like this

	size_t max_len = net_if_get_mtu(net_pkt_iface(pkt));

	if (IS_ENABLED(CONFIG_NET_IPV6) && family == AF_INET6) {
		max_len = max(max_len, NET_IPV6_MTU);
	} else if (IS_ENABLED(CONFIG_NET_IPV4) && family == AF_INET) {
		max_len = max(max_len, NET_IPV4_MTU);
	} 

then we do not need the AF_UNSPEC branch at all.

[tbursztyka]

indeed

[jukkar]

Offering this kind of option is imho dangerous, someone will misuse it anyway. How much memory we are saving with this, what does ram_report say if we enable/disable it?

[tbursztyka]

for udp/ipv4 setup, even on 128bytes frag size this will be just fine option (it is dangerous at the same level as wrongly teawking frag size: user has to know what he is doing). It saves stack actually.

[jukkar]

Sure, it probably save something. Just wondering do you have numbers about the savings.

[jukkar]

Why do we need to have next_header_proto parameter here and in finalize function, it looks like one should be enough.

[tbursztyka]

you mean we would inform ipv4 header about the protocol only at finalize? why not indeed.

[jukkar]

I suggest we change the name to net_pkt_read_u8_new() in order to be consistent. Same with the other value reader helpers.

[tbursztyka]

will be easier to remove the _new when necessary yes

[pfalcon]

@tbursztyka: I appreciate the detailed write up in #11775 (comment).

My original question re: relation to #11374 was simpler actually, to confirm that #11374 precedes this patch, instead of e.g. be superseded by this one, or a case when they largely unrelated. So, I just assume that #11374 is the next in queue, and not anything else.

[tbursztyka]

@pfalcon yes PR #11374 is the first in queue.

[pfalcon]

In one of recent commits:

c965644: net/core: Each and every received packet are bein set to overwrite

"is being"