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

[pfalcon]

We don't yet implement MSS negotiation for all directions (client vs
server), and yet have tests/samples which assume that it's possible
to transfer more than default MSS of 536 in a single packet.

So, until negotiation is fully implemented, bump default MSS to be
higher.

Signed-off-by: Paul Sokolovsky [email protected]

[pfalcon]

This augments #5015 to not touch some broken special samples for 1.10 release.

Note that I'll be happy to work on full impl of MSS negotiation, I'm just not sure it would be ready for 1.10.

[pfl]

So if MSS is not fully implemented, then an MSS larger than 536 cannot be used, see the mentioned RFC. Here one has to be conservative and keep that MSS of 536 until the MSS negotiation has completed successfully. In IPv6 the MSS cannot be larger than 1220, since there are 40 bytes of IPv6 headers and 20 bytes of TCP headers, so setting it to 1280 is not the correct value. Besides, if this function does not know whether TCP is going to be transported over IPv4 or IPv6, the conservative estimate of 536 needs to be used.

[pfalcon]

@pfl thanks, but:

1. What you say is pretty obvious and known to the author of the patch.
2. Zephyr IP stack has multiple violations of IP standards.
3. There's incremental work to make it compliant, and fixing one issue often leads to exposing issues in other parts of code.
4. This patch is part of the work to get more overall p.1 fixes into the 1.10 release, rather than less, which requires quite extensive negotiations and compromises among all the parties involves, see net: net_pkt_append: Take into account MSS/MTU when adding data to a packet #119, BSD Sockets API: Offloading Support #4821, net: 15.4 network interfaces use incorrect MTU setting of 127  #4934, net: pkt: net_pkt_append: Use only TCP MSS to cap packet size #5015, etc.

[pfl]

I can agree with points 1.-4. above. But in order to fix things, let's not break the ones that actually implement the standards properly.

[pfalcon]

I am the author of MSS handling/negotiation support in Zephyr (there was none before I started to implement, I haven't finished yet). And for the sake of impeding 1.10 release, I find it acceptable to effectively disable the MSS handling functionality I added, because there're a lost of code in Zephyr which isn't ready for proper MSS handling (actually, only 2 pieces are guaranteedly ready: BSD Sockets, authors by me, and new HTTP API, for which @jukkar kindly accepted my suggestions).

[tbursztyka]

Sounds like a wrong hack to me as well.

[pfalcon]

Sounds like a wrong hack to me as well.

Then let me be straight: this is the final "litmus test" to figure out whether there's more interest in a) just reverting the functionality I added, or b) resolving the issues. For b) part, I offered 3-4 solutions, with ones not immediately shot down, backed by patches. I guess the rest are for the maintainers to decide.

[pfl]

I'm all for point b), resolving the issues. For this change, it apparently means fixing MSS handling, if found to be broken. Question is, whether it is totally broken or works somewhat to the extent that the applications can work "mostly". In the latter case it is not in need of immediate fixing, in both cases let's fix the buggy parts and not add a temporary hack - especially since this change is not visibly part of a bigger patch set and by itself introduces yet another thing that is afterwards violating TCP specifications.

[pfalcon]

For this change, it apparently means fixing MSS handling, if found to be broken.

It was found to be non-existent, and I started to implement it (as recursive task to allow POSIX-style model of network sends). I'm sad that I didn't finish it, but "disabling" it definitely feels better than throwing away even more work done during last month.

[jukkar]

There are now too many patches floating around fixing various things and we seem to loose the track.
I try to summarize the issues here:

• echo-client does not currently work as described in net: 15.4 network interfaces use incorrect MTU setting of 127  #4934 This needs to be fixed and all the other clients/servers that use net_pkt_append() functions so that the caller should only try to send max number of bytes (no patch exists for this yet)

• The Revert "net: pkt: Take into account MTU when adding data to a packet" #4972 would revert the patch that caused the echo-client regression. But this is not ultimate solution as the TCP MSS is then not taken into account. Currently this does not seem to cause any issues but should be fixed of course.

• The net: pkt: net_pkt_append: Use only TCP MSS to cap packet size #5015 fixes the UDP connection issue which is good, but the TCP side in echo-client is still not working properly

• The net: tcp: Use larger default MSS until its negotiation is fully implemented (1.10 release workaround) #5034 (this PR) tries to increase mss so that echo-client would work, but I think we all agree that this hack is not the proper fix

[pfalcon]

That's good summary, there're 2 options:

1. Either apply revert in Revert "net: pkt: Take into account MTU when adding data to a packet" #4972, which has a cascading effect (affects new HTTP API), and for which recovery plan is not clear.
2. Or apply both net: pkt: net_pkt_append: Use only TCP MSS to cap packet size #5015 and net: tcp: Use larger default MSS until its negotiation is fully implemented (1.10 release workaround) #5034 which effectively set Zephyr-wide default behavior to what it was before (1.9-level), but keep the new code relying on it working, and allow to continue, not restart, work on elaborating the matters post 1.10 release.

[jukkar]

Either apply revert in #4972, which has a cascading effect (affects new HTTP API), and for which recovery plan is not clear.

I do not think this would cause new HTTP API revertion, if there would be some issues in HTTP API, those could be fixed.

Or apply both #5015 and #5034 which effectively set Zephyr-wide default behavior to what it was before (1.9-level), but keep the new code relying on it working, and allow to continue, not restart, work on elaborating the matters post 1.10 release.

If we do not revert the MTU changes, then we should apply #5015 but should not apply #5034 but fix the callers of net_pkt_append() instead (option 3).

Adding new code might regress things more so safest option might be to apply #4972 This needs more testing if it would cause some issues in HTTP API.

[pfalcon]

If we do not revert the MTU changes, then we should apply #5015 but should not apply #5034 but fix the callers of net_pkt_append() instead (option 3).

That would be a big change. Literally, all of them (TCP ones, of course) would need to follow the same pattern as new HTTP API - loop over get_pkt_tx() and pkt_append() (because MSS can be 1, send window can be 1).

This needs more testing if it would cause some issues in HTTP API.

Well, handling of large amounts of data (more than fits in one packet (actually MSS, soon send window)) in the new HTTP API depends on net_pkt_append() adding only as much as possible, and returning how much was added.

Adding new code might regress things more so safest option might be to apply #4972

#5015 doesn't add new code, it removes UDP branch. This patch (#5034) doesn't add code, just changes value of a constant (we can set it to 1220 as @pfl suggested, or to 1500/1460/1440 to cover Eth case). The cumulative effect should be that net_pkt_append() will do pkt size limiting, but only for TCP, and using optimistic defaults, similar to how IP stack worked before.

IMHO, that's a pretty safe approach, but you're the maintainer to make a decision.

[pfl]

Added RFC #5038 to address this issue. Please comment and let's continue after the weekend. Applications need also to be updated to take into account how many octets can be and were sent to the other end.

[jukkar]

Added #5072 to address the issue mentioned in #4934

[pfalcon]

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