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: tcp: Check TCP ACK flag properly during conn establishment #13957

Merged

Conversation

jukkar
Copy link
Member

@jukkar jukkar commented Feb 28, 2019

Multiple flag bits were set so the ACK flag set was not checked
properly which meant that connection establishment was not
successfull.

Fixes #13943

Signed-off-by: Jukka Rissanen jukka.rissanen@linux.intel.com

@jukkar
Copy link
Member Author

jukkar commented Feb 28, 2019

@pfalcon can you try this if it helps with #13943

Multiple flag bits were set so the ACK flag set was not checked
properly which meant that connection establishment was not
successfull.

Fixes zephyrproject-rtos#13943

Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
@jukkar jukkar force-pushed the bug-13943-tcp-ack-flag-check branch from 25925ef to 712c0cd Compare February 28, 2019 17:35
@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #13957   +/-   ##
=========================================
  Coverage          ?   52.29%           
=========================================
  Files             ?      307           
  Lines             ?    45431           
  Branches          ?    10507           
=========================================
  Hits              ?    23756           
  Misses            ?    16888           
  Partials          ?     4787
Impacted Files Coverage Δ
subsys/net/ip/tcp.c 55.95% <0%> (ø)

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 846dfd4...712c0cd. Read the comment docs.

@pfalcon pfalcon requested review from rlubos and rveerama1 February 28, 2019 18:58
Copy link
Contributor

@pfalcon pfalcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jukkar, It definitely has a positive effect. Now qemu_x86+e1000 works like:

Booting from ROM..***** Booting Zephyr OS zephyr-v1.13.0-5246-g6573ea298eee *****
[00:00:00.037,808] <inf> net_config: Initializing network
[00:00:00.043,472] <inf> net_config: IPv4 address: 192.0.2.1
Single-threaded dumb HTTP server waits for a connection on port 8080...
[00:01:41.863,224] <err> net_arp: Gateway not set for iface 0x00408140
Connection #0 from 192.0.2.2
Connection from 192.0.2.2 closed
Connection #1 from 192.0.2.2
Connection from 192.0.2.2 closed
Connection #2 from 192.0.2.2
[00:02:35.924,776] <err> eth_e1000: Out of memory for received frame
[00:02:37.563,814] <err> eth_e1000: Out of memory for received frame
Connection from 192.0.2.2 closed
Connection #3 from 192.0.2.2
Connection from 192.0.2.2 closed
Connection #4 from 192.0.2.2
[00:02:39.242,195] <err> eth_e1000: Out of memory for received frame

Despite those "OOMs", it crawls on, and was able to finish ab -n1000 http://192.0.2.1:8080/.

This is completely crazy, because this code was around like, forever, as #13943 says, smsc911x was able to process a hundred of requests even without it. And most importantly - dump_http_server + ab -n1000 is my default smoke test, and I definitely tested all 3 qemu eth drivers like that when they just landed (== before "net_buf refactor").

@pfalcon pfalcon added the priority: medium Medium impact/importance bug label Feb 28, 2019
@pfalcon pfalcon added this to the v1.14.0 milestone Feb 28, 2019
@jukkar
Copy link
Member Author

jukkar commented Mar 1, 2019

Despite those "OOMs", it crawls on, and was able to finish ab -n1000 http://192.0.2.1:8080/.

The errors are not actually out-of-memory issues even thou the driver claims that. I did not managed to investigate it more deeply what is causing the function to return an error. @tbursztyka perhaps you have an idea?

What is a bit weird, that the code this PR is changing has not been changed for a long time. So it is some other part of the system that was changed that caused the issue. Last change that touches code around the ACK check is this commit baa9784

@nashif nashif merged commit ea1e4fd into zephyrproject-rtos:master Mar 1, 2019
@jukkar jukkar deleted the bug-13943-tcp-ack-flag-check branch March 9, 2019 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Networking priority: medium Medium impact/importance bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants