Skip to content
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

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

Closed

Conversation

pfalcon
Copy link
Contributor

@pfalcon pfalcon commented Nov 16, 2017

Previously, there was also check for UDP case using an interface MTU,
but that led to problems due to unclear definition of MTU in Zephyr
interfaces. The original patch assumed it's IP-level MTU, as
prescribed by RFC 791 (IPv4) and RFC 2460 (IPv6). But turns out,
it was originally intended to be a max frame size for the
underlying hardware driver (which for 6LoWPAN technologies may be
smaller than IP-level MTU).

Signed-off-by: Paul Sokolovsky paul.sokolovsky@linaro.org

Previously, there was also check for UDP case using an interface MTU,
but that led to problems due to unclear definition of MTU in Zephyr
interfaces. The original patch assumed it's IP-level MTU, as
prescribed by RFC 791 (IPv4) and RFC 2460 (IPv6). But turns out,
it was originally intended to be a max frame size for the
underlying hardware driver (which for 6LoWPAN technologies may be
smaller than IP-level MTU).

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
@pfalcon
Copy link
Contributor Author

pfalcon commented Nov 16, 2017

This is an alternative to #4972 .

@jukkar
Copy link
Member

jukkar commented Nov 16, 2017

Tested this with echo-client and some other programs. Using UDP works with this but TCP does not. As the 1.10 release is coming very fast and in order not to introduce this regression, I suggest we do the #4972 and then later fix this mess for 1.11.

@pfalcon
Copy link
Contributor Author

pfalcon commented Nov 16, 2017

Using UDP works with this but TCP does not.

One explanation would be that it never worked correctly, and implementation of the proper MSS handling (per the requirements of the TCP standard) exposed that. Specifically, MSS negotiation is not completely implemented and/or echo_client assumes that it can put ~arbitrary amount of data into a single packet, which is a violation of TCP protocol. At most MSS bytes can be put into into one packet, and if MSS is not available (not negotiated), the standard default value of 536 should be used.

The fix for echo_client is simple:

--- a/samples/net/echo_client/src/echo-client.c
+++ b/samples/net/echo_client/src/echo-client.c
@@ -48,7 +48,8 @@ const char lorem_ipsum[] =
        "mattis, nec faucibus dui rutrum. Ut mollis orci in iaculis "
        "consequat. Nulla volutpat nibh eu velit sagittis, a iaculis dui "
        "aliquam."
-       "\n"
+       "\n";
+#if 0
        "Quisque interdum consequat eros a eleifend. Fusce dapibus nisl "
        "sit amet velit posuere imperdiet. Quisque accumsan tempor massa "
        "sit amet tincidunt. Integer sollicitudin vehicula tristique. Nulla "
@@ -64,6 +65,7 @@ const char lorem_ipsum[] =
        "quis egestas nulla. Quisque ac risus quis elit mollis finibus. "
        "Phasellus efficitur imperdiet metus."
        "\n";
+#endif

I.e. limiting amount of data to 536.


Otherwise, are you going to also revert your #4243? It depends on #119.

@jukkar
Copy link
Member

jukkar commented Nov 17, 2017

The fix for echo_client is simple:

This is not a fix but a workaround. The echo-client is suppose to send max 1280 payload data in order to verify IPv6 functionality and we will not have this kind of hack here.

@pfalcon
Copy link
Contributor Author

pfalcon commented Nov 17, 2017

Sorry, but there're many hacks and workarounds in the Zephyr IP stack. To send 1280 payload over TCP, you need to negotiate corresponding MSS, which is not done by the current IP stack (for all cases). As a hack, you sent 1280 payload without negotiation.

What's important is direction we're going in - either trying to improve and resolve things (incrementally as the only realistic way), or go in circles.

(Please consider all the ticket discussion happening since the end of last week as a kind of replacement for the TSC sockets-vs-not-sockets discussion which I couldn't join. I'm sorry if my comments aren't helpful; from my side, I'd like to make sure that all options for "staying close" were considered before diverging split happened. E.g. upstream upgrade broke sockets: #5024 , and surprisingly, no talks about reverts).

@pfalcon
Copy link
Contributor Author

pfalcon commented Nov 17, 2017

Btw, with #4972 (i.e. revert) applied, for echo_server+echo_client QEMU combo stops or crashes (in echo_server it seems) pretty soon after start. With #5015 (comment) applied (and no revert), it runs for much longer and doesn't crash.

That's re: how well stuff in the mainline works. (While preparing another patch, an alternative to revert.)

@jukkar
Copy link
Member

jukkar commented Nov 20, 2017

We need this one so that UDP starts to work in echo-client. Also fix to echo-client in #5072 is needed.

@jukkar
Copy link
Member

jukkar commented Nov 21, 2017

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

@pfalcon
Copy link
Contributor Author

pfalcon commented Nov 21, 2017

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

Replied here: #5038 (comment)

@pfalcon
Copy link
Contributor Author

pfalcon commented Nov 24, 2017

Per the #5038 (comment) , closing this, as long as #4972 is closed.

@pfalcon pfalcon closed this Nov 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants