net: context: tcp: set the pkt appdatalen info when sending packets #73

simple2017 · 2017-05-03T17:04:30Z

when received an ACK, we need the pkt appdatalen info to calculate seq which determine these packets should be released.

This is related to commit "net: tcp: Make sure ACK timer is not run if cancelled" which did not set the cancel flag when the timer was cancelled from tcp.c. Signed-off-by: Jukka Rissanen <[email protected]>

When adding link-local address to the cache the type needs to be properly set as net_ipv6_addr_create_iid will attempt to use it when generating the IPv6 address. Jira: ZEP-2077 Signed-off-by: Luiz Augusto von Dentz <[email protected]>

pfalcon · 2017-05-03T18:10:50Z

@simple2017 : Thanks for resubmitting this from gerrit! Perhaps you could fill in your name in Github profile, so it was easier to recognize you ;-).

@jukkar , @tbursztyka : So, do you guys agree with @nashif's RFC that all patches should be submitted against master? Otherwise, we'll keeping having issues with stray commits like this.

jukkar · 2017-05-03T20:05:04Z

So, do you guys agree with @nashif's RFC that all patches should be submitted against master? Otherwise, we'll keeping having issues with stray commits like this.

This is submitted against master which is fine, we all agree on this.

jukkar

The patch itself looks ok. I would like to get the commit message a bit better, perhaps something like this (feel free to edit the message as needed and remember not to exceed the max line length in commit msg)

net: context: Set TCP app data len when sending packet

When we send TCP data segment, we need to set the length of the application data by calling net_pkt_set_appdatalen(). This is done so that sequence number can be properly advanced when we receive ACK to that pending packet.

pfalcon · 2017-05-03T20:44:12Z

@jukkar :

This is submitted against master which is fine, we all agree on this.

Yes, just seems to be submitted from net branch against master, why a couple of extra commits. Hopefully net branch will be fully flushed to master.

simple2017 · 2017-05-04T02:48:30Z

Thanks all, @pfalcon @jukkar Maybe I should close this pull request and follow your advice to reopen a pull request against the master branch. Because I found some extra commits included in my single commit when using net branch. In the new pull request I will modify the commit message format errors and patch-check problem.

jukkar · 2017-05-04T07:28:59Z

Yes, just seems to be submitted from net branch against master, why a couple of extra commits. Hopefully net branch will be fully flushed to master.

Those extra commits are really annoying, but I think we can safely ignore them at this point.

jukkar · 2017-05-04T07:30:31Z

Thanks all, @pfalcon @jukkar Maybe I should close this pull request and follow your advice to reopen a pull request against the master branch. Because I found some extra commits included in my single commit when using net branch. In the new pull request I will modify the commit message format errors and patch-check problem.

No need to close this PR. Just fix the commit message issues and whatever is needed to make Shippable happy. So please re-submit this PR with fixes.

simple2017 · 2017-05-04T09:26:13Z

@jukkar OK, I got it, thanks a lot.

pfalcon · 2017-05-04T15:34:17Z

@simple2017 : Please squash your 2 commits together (but use the most recent commit message of course).

When we send TCP data segment, we need to set the length of the application data by calling net_pkt_set_appdatalen(). This is done so that sequence number can be properly advanced when we receive ACK to that pending packet. Signed-off-by: xiaorui hu <[email protected]>

pfalcon · 2017-05-05T18:08:24Z

Ok, based on IRC discussion with @jukkar that it would be nice to understand effect of this change better (i.e. know what it fixes and what doesn't), I played with it today. I first tried to do fault injection on packet receive path, verifying that it works as expected for HTTP client type load. Then my idea was that maybe it would work for HTTP server style too, by dropping peer's ACKs, thus triggering Zephyr to do rexmits, and trigger bad things. But that didn't really work.

So, I went for fault inject on send path too, and that immediately showed the effect of the change. So, without this change, with send pkt drop I saw with a test server in MicroPython:

Connected to 192.0.2.1.
Escape character is '^]'.
GET / HTTP/1.0

HTTP/1.0 200 OK

Hello #0 from MicroPython!

At which point no further data were received, connection just hanged, and Wireshark showed rexmits storm, but all in vain.

With this patch, the output was below, as expected from the server. For reference, my test patch is at https://github.com/pfalcon/zephyr/commits/net-pkt-fault-inject

Connected to 192.0.2.1.
Escape character is '^]'.
GET / HTTP/1.0

HTTP/1.0 200 OK

Hello #0 from MicroPython!
HTTP/1.0 200 OK

Hello #1 from MicroPython!
HTTP/1.0 200 OK

Hello #2 from MicroPython!
HTTP/1.0 200 OK

Hello #3 from MicroPython!
HTTP/1.0 200 OK

Hello #4 from MicroPython!
HTTP/1.0 200 OK

Hello #5 from MicroPython!
HTTP/1.0 200 OK

Hello #6 from MicroPython!
HTTP/1.0 200 OK

Hello #7 from MicroPython!
HTTP/1.0 200 OK

Hello #8 from MicroPython!
HTTP/1.0 200 OK

Hello #9 from MicroPython!
HTTP/1.0 200 OK

Hello #10 from MicroPython!
HTTP/1.0 200 OK

Hello #11 from MicroPython!
HTTP/1.0 200 OK

Hello #12 from MicroPython!
HTTP/1.0 200 OK

Hello #13 from MicroPython!
HTTP/1.0 200 OK

Hello #14 from MicroPython!
HTTP/1.0 200 OK

Hello #15 from MicroPython!
HTTP/1.0 200 OK

Hello #16 from MicroPython!
HTTP/1.0 200 OK

Hello #17 from MicroPython!
HTTP/1.0 200 OK

Hello #18 from MicroPython!
HTTP/1.0 200 OK

Hello #19 from MicroPython!
Connection closed by foreign host.

pfalcon · 2017-05-05T20:52:14Z

Next test - problem I repeated with HTTP client connection to http://archive.ubuntu.com - expectedly, doesn't help, as it's client (recv) application.

pfalcon · 2017-05-06T11:28:46Z

Final test I wanted to perform. It's my understanding that @jukkar wondered whether this change would improve TCP behavior of https://gerrit.zephyrproject.org/r/#/c/13077/ . Though I may miss something, and he meant another test. Anyway, I tried it, and well, for it patch doesn't have any visible effect, after few packets, processing still deadlocks.

Anyway, I see that @simple2017 has squashed 2 commits together, so this should be ready to merge, and make pretty good improvements already.

pfalcon · 2017-05-08T12:42:53Z

@simple2017 : Great work, congrats on your first patch merged! ;-) Hope it will be easier next time, looking forward to new IP stack fixes that you found.

# Conflicts: # dts/bindings/serial/st,stm32-uart-base.yaml # subsys/net/ip/net_if.c

jukkar and others added 2 commits May 2, 2017 10:21

net: tcp: Mark ACK timer as cancelled

464545e

This is related to commit "net: tcp: Make sure ACK timer is not run if cancelled" which did not set the cancel flag when the timer was cancelled from tcp.c. Signed-off-by: Jukka Rissanen <[email protected]>

net: bt: Fix not setting lladdr type

6b62d2f

When adding link-local address to the cache the type needs to be properly set as net_ipv6_addr_create_iid will attempt to use it when generating the IPv6 address. Jira: ZEP-2077 Signed-off-by: Luiz Augusto von Dentz <[email protected]>

jukkar self-requested a review May 3, 2017 20:05

jukkar requested changes May 3, 2017

View reviewed changes

jukkar approved these changes May 8, 2017

View reviewed changes

pfalcon approved these changes May 8, 2017

View reviewed changes

nashif merged commit cffb79a into zephyrproject-rtos:master May 8, 2017

simple2017 deleted the net_ip_fix branch May 9, 2017 07:47

This was referenced Sep 18, 2017

Fault injection framework for Zephyr zephyriot/zep-jira14#1959

Open

Fault injection framework for Zephyr zephyriot/zephyr-issues#1959

Open

zephyrbot mentioned this pull request Sep 23, 2017

Fault injection framework for Zephyr #3559

Open

MariuszSkamra mentioned this pull request Jul 15, 2019

L2CAP: On insufficient authentication error received, Zephyr does unauthenticated pairing #17525

Closed

piotrParf mentioned this pull request Feb 9, 2022

BLE Testing functions do not work properly #42534

Closed

ndrs-pst added a commit to DDC-NDRS/zephyr_rtos that referenced this pull request Sep 18, 2025

merge: 'upstream' into pst-v4.2.99 zephyrproject-rtos#73

3c2b1a7

# Conflicts: # dts/bindings/serial/st,stm32-uart-base.yaml # subsys/net/ip/net_if.c

Uh oh!

net: context: tcp: set the pkt appdatalen info when sending packets #73

net: context: tcp: set the pkt appdatalen info when sending packets #73

Uh oh!

Conversation

simple2017 commented May 3, 2017

Uh oh!

pfalcon commented May 3, 2017

Uh oh!

jukkar commented May 3, 2017

Uh oh!

jukkar left a comment

Choose a reason for hiding this comment

Uh oh!

pfalcon commented May 3, 2017

Uh oh!

simple2017 commented May 4, 2017

Uh oh!

jukkar commented May 4, 2017

Uh oh!

jukkar commented May 4, 2017

Uh oh!

simple2017 commented May 4, 2017

Uh oh!

pfalcon commented May 4, 2017

Uh oh!

pfalcon commented May 5, 2017

Uh oh!

pfalcon commented May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pfalcon commented May 6, 2017

Uh oh!

pfalcon commented May 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pfalcon commented May 5, 2017 •

edited

Loading