net: pkt: Compute TX payload data length #5038

pfl · 2017-11-17T13:49:20Z

Compute the length of the TX payload that is transported in one IPv4 or IPv6 datagram taking into account UDP, ICMP or TCP headers in addition to any IPv6 extension headers added by RPL. The TCP implementation in Zephyr is known to currently carry at maximum 8 bytes of options. If the protocol is not known to the stack, assume that the application handles any protocol headers as well as the data. Also, if the net_pkt does not have a context associated, length check on the data is omitted when appending.

Although payload length is calculated also for TCP, the TCP MSS value is used as before.

Define IPv4 minimum MTU as 576 octets, See RFC 791, Sections 3.1. and 3.2.

Signed-off-by: Patrik Flykt patrik.flykt@intel.com

pfl · 2017-11-17T13:51:12Z

This is definitely a Request For Comments patch, let's continue coding on monday when everybody is back. The IPv6 upper layer header should be stored in advance, i.e. when the extension header lengths are computed.

pfl · 2017-11-20T14:14:09Z

Need to compute header lengths already at packet allocation times, updated RFC patch accordingly.

tbursztyka

Maybe we should try to avoid adding another attribute to net_pkt

tbursztyka · 2017-11-20T14:41:27Z

include/net/net_pkt.h

@@ -72,6 +72,8 @@ struct net_pkt {
 	struct net_linkaddr lladdr_src;
 	struct net_linkaddr lladdr_dst;

+	u16_t data_len;         /* amount of payload data that can be added */


I think this could be avoided, see below

+1 . From my comment #119 (comment) :

As far as I can tell, for TX packet, net_pkt->appdatalen isn't used until packet construction is finished and ready to be sent. So, it can be used to cache "remaining data size in the packet" (initialized in net_pkt_get_tx()).

tbursztyka · 2017-11-20T14:42:28Z

subsys/net/ip/net_pkt.c

+		if (proto == IPPROTO_ICMP || proto == IPPROTO_ICMPV6) {
+			data_len -= NET_ICMPH_LEN;
+		}
+


all the code block here is not that much time/performance consuming so (see below)

tbursztyka · 2017-11-20T14:43:11Z

subsys/net/ip/net_pkt.c

 	 */
-	max_len = 0x10000;
+	max_len = pkt->data_len;


it could anyway be called here everytime and available space would be computed from that minus current total length.

no?

We need to remember the amount of space remaining so that two consecutive calls to net_pkt_append() succeed, unless there is another application data counter somewhere in the struct already?

As @pfl mentioned, we need to support the case where multiple append/insert calls are done, so the amount of bytes needs to be remembered.

But we know that: we can get the actual occupied length in the chain of net_buf.

What variables would we need to look at?

I thought the whole length of fragment list, net_buf_frags_len(pkt->frags), could help. But as the ll reserve part is badly managed for now, it won't work.

jukkar · 2017-11-20T15:27:58Z

Briefly tested this with echo-server and http-server running in zephyr side. Seemed to work just fine. Will test more tomorrow.

pfl · 2017-11-21T11:44:22Z

Coap tests succeed with the latest update.

rveerama1 · 2017-11-21T12:26:28Z

subsys/net/ip/net_pkt.c

+		if (IS_ENABLED(CONFIG_NET_UDP) && proto == IPPROTO_UDP) {
+			data_len -= NET_UDPH_LEN;
+
+#if defined(CONFIG_NET_RPL_INSERT_HBH_OPTION)


How about considering net_pkt_set_ipv6_ext_len() in case ipv6 and move away RPL check here. RPL setting this value when insert is success "net_pkt_set_ipv6_ext_len(pkt, ext_len + NET_RPL_HOP_BY_HOP_LEN);".

RPL has not yet set the ext header length when this function is called.

pfalcon · 2017-11-21T12:33:06Z

Inspired by @jukkar's comment at #5015 (comment) :

It looks like #5038 could replace this (#5015) one, also then we would not need to have #5034 either.

I definitely appreciate this patch, the questions is its scope: #5015/#5034 were proposed as safe (IMHO) and acceptable workarounds to get the 1.10 out. This patch is a bigger changes in itself, and based on the comments, there're various things which could be tweaked/improved. Up to @jukkar / @tbursztyka then whether this could fit into 1.10. (A possible compromise might be merge mostly as is as long as it works, and leave further refactors/optimizations until new merge window opens).

pfl · 2017-11-21T13:30:34Z

Commit message and its header updated, unnecessary setting variable value on initialization removed.

jukkar

I have been testing this and everything has been working just fine so +1 from me.

pfalcon · 2017-11-23T08:46:29Z

Ok, if this is considered for merging, I'd like to ask to update the commit message to explicitly state that the change is for TX packets. E.g., the title can be (at the very least) net: pkt: Compute TX payload data length (better net: pkt: Compute payload data length for TX packets), and corresponding additions in the commit description body.

pfalcon · 2017-11-23T08:55:38Z

More of commit message:

TCP is known to carry at maximum 8 bytes of options.

Well, that's to broad a statement to make. The TCP (RFC 793) definitely can carry more than 8 bytes of options. We have the NET_TCP_MAX_OPT_SIZE define with a random value, apparently inherited from FNET, removing of which (FNET cruft) is on my TODO.

So, suggestion: keep the code as is (as it's just first iteration of the matter, will be refactoring/optimizing it later based on all suggestions), but remove that sentence from the commit message.

I'd even ask why all that is at all relevant for the TCP case, if

TCP payload length is not changed, TCP MSS is used as before.

But don't let me go that rabbit hole, again, pretty good for the first iteration.

pfalcon

I'd really appreciate the commit message changes as suggested above, to not create unneeded confusion. Thanks.

pfl · 2017-11-23T09:04:09Z

Well, that's to broad a statement to make. The TCP (RFC 793) definitely can carry more than 8 bytes of options.

This patch relates to Zephyr's implementation of TCP, so that is an accurate statement to make, actually.

I'd even ask why all that is at all relevant for the TCP case, if

It is relevant, since it computes lengths already for the TCP case, even though tcp->mss is still being used. That again is for another patch, but not for 1.10, since no release critical bugs are reported for TCP right now. TCP works fine, AFAIK by glancing at Jukka's screen where he's sending and receiving something continuosly.

pfalcon · 2017-11-23T09:27:29Z

This patch relates to Zephyr's implementation of TCP, so that is an accurate statement to make, actually.

As an outside reader, I find the phrase "TCP is known to carry at maximum 8 bytes of options." to be highly ambiguous/confusing, that's why I ask that to be dealt with.

Compute the length of the TX payload that is transported in one IPv4 or IPv6 datagram taking into account UDP, ICMP or TCP headers in addition to any IPv6 extension headers added by RPL. The TCP implementation in Zephyr is known to currently carry at maximum 8 bytes of options. If the protocol is not known to the stack, assume that the application handles any protocol headers as well as the data. Also, if the net_pkt does not have a context associated, length check on the data is omitted when appending. Although payload length is calculated also for TCP, the TCP MSS value is used as before. Define IPv4 minimum MTU as 576 octets, See RFC 791, Sections 3.1. and 3.2. Signed-off-by: Patrik Flykt <patrik.flykt@intel.com>

pfl · 2017-11-23T13:08:00Z

Updated commit message.

pfalcon

Thanks!

pfl · 2017-11-24T11:04:01Z

AFAIK, together with the already merged #5072, this PR fixes #4934, and obsoletes #5034, #4972 and #5015.

pfalcon · 2017-11-24T14:35:03Z

Thanks for merging.

AFAIK, together with the already merged #5072, this PR fixes #4934, and obsoletes #5034, #4972 and #5015.

@jukkar closed #4972, and I closed #5015 & #5034.

pfl requested review from jukkar and tbursztyka as code owners November 17, 2017 13:49

pfl mentioned this pull request Nov 17, 2017

net: tcp: Use larger default MSS until its negotiation is fully implemented (1.10 release workaround) #5034

Closed

nashif added RFC Request For Comments: want input from the community and removed RFC Request For Comments: want input from the community labels Nov 18, 2017

pfl force-pushed the ip_mtu branch from 230f22b to f5876af Compare November 20, 2017 14:12

pfl force-pushed the ip_mtu branch from f5876af to 6e5cc94 Compare November 20, 2017 14:38

tbursztyka reviewed Nov 20, 2017

View reviewed changes

pfl force-pushed the ip_mtu branch from 6e5cc94 to 7128c8f Compare November 21, 2017 07:53

jukkar added the area: Networking label Nov 21, 2017

pfl force-pushed the ip_mtu branch from 7128c8f to b0c6a4b Compare November 21, 2017 11:38

jukkar mentioned this pull request Nov 21, 2017

net: pkt: net_pkt_append: Use only TCP MSS to cap packet size #5015

Closed

rveerama1 reviewed Nov 21, 2017

View reviewed changes

pfl force-pushed the ip_mtu branch from b0c6a4b to 0f7a3c0 Compare November 21, 2017 13:27

jukkar added this to the v1.10.0 milestone Nov 22, 2017

pfl changed the title ~~RFC: Compute IP payload length~~ net: pkt: Compute payload data length Nov 23, 2017

jukkar approved these changes Nov 23, 2017

View reviewed changes

pfalcon requested changes Nov 23, 2017

View reviewed changes

jukkar removed the RFC Request For Comments: want input from the community label Nov 23, 2017

pfl force-pushed the ip_mtu branch from 0f7a3c0 to f6d31d6 Compare November 23, 2017 13:07

pfl changed the title ~~net: pkt: Compute payload data length~~ net: pkt: Compute TX payload data length Nov 23, 2017

pfalcon approved these changes Nov 23, 2017

View reviewed changes

tbursztyka approved these changes Nov 24, 2017

View reviewed changes

nashif merged commit 753daa6 into zephyrproject-rtos:master Nov 24, 2017

This was referenced Jan 29, 2018

net: 15.4 network interfaces use incorrect MTU setting of 127 #4934

Closed

IP stack: No MTU handling on send() #3439

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: pkt: Compute TX payload data length #5038

net: pkt: Compute TX payload data length #5038

pfl commented Nov 17, 2017 •

edited

Loading

pfl commented Nov 17, 2017

pfl commented Nov 20, 2017

tbursztyka left a comment

tbursztyka Nov 20, 2017

pfalcon Nov 21, 2017

tbursztyka Nov 20, 2017

tbursztyka Nov 20, 2017

pfl Nov 20, 2017

jukkar Nov 20, 2017

tbursztyka Nov 20, 2017

pfl Nov 21, 2017

tbursztyka Nov 21, 2017 •

edited

Loading

jukkar commented Nov 20, 2017

pfl commented Nov 21, 2017

rveerama1 Nov 21, 2017

pfl Nov 21, 2017

pfalcon commented Nov 21, 2017

pfl commented Nov 21, 2017

jukkar left a comment

pfalcon commented Nov 23, 2017

pfalcon commented Nov 23, 2017

pfalcon left a comment

pfl commented Nov 23, 2017

pfalcon commented Nov 23, 2017

pfl commented Nov 23, 2017

pfalcon left a comment

pfl commented Nov 24, 2017

pfalcon commented Nov 24, 2017

net: pkt: Compute TX payload data length #5038

net: pkt: Compute TX payload data length #5038

Conversation

pfl commented Nov 17, 2017 • edited Loading

pfl commented Nov 17, 2017

pfl commented Nov 20, 2017

tbursztyka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbursztyka Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

jukkar commented Nov 20, 2017

pfl commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfalcon commented Nov 21, 2017

pfl commented Nov 21, 2017

jukkar left a comment

Choose a reason for hiding this comment

pfalcon commented Nov 23, 2017

pfalcon commented Nov 23, 2017

pfalcon left a comment

Choose a reason for hiding this comment

pfl commented Nov 23, 2017

pfalcon commented Nov 23, 2017

pfl commented Nov 23, 2017

pfalcon left a comment

Choose a reason for hiding this comment

pfl commented Nov 24, 2017

pfalcon commented Nov 24, 2017

pfl commented Nov 17, 2017 •

edited

Loading

tbursztyka Nov 21, 2017 •

edited

Loading