You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/01/19 08:17:53 UTC
[GitHub] [incubator-nuttx] a-lunev opened a new pull request #5272: net/tcp/sendfile: retransmit only one the earliest not acknowledged segment
a-lunev opened a new pull request #5272:
URL: https://github.com/apache/incubator-nuttx/pull/5272
## Summary
The existing TCP sendfile implementation does a full rewind from the most recent sent segment back to the earliest not acknowledged one, thus many TCP segments are re-sent every time when TCP retrasmission timeout occurs.
According to RFC 6298 (5.4) only one the earliest not acknowledged segment should be retransmitted instead.
This PR implements TCP retrasmission according to RFC 6298 (5.4).
It is the same issue as it was in tcp_send_unbuffered.c (PR #4659).
## Impact
TCP
## Testing
Activate emulating packet loss on Linux host:
`$ sudo iptables -A INPUT -p tcp --dport 5471 -m statistic --mode random --probability 0.01 -j DROP`
Build NuttX:
```
$ ./tools/configure.sh -l sim:tcpblaster
$ make menuconfig
(enable CONFIG_NETUTILS_NETCAT_SENDFILE,
disable CONFIG_NET_TCP_WRITE_BUFFERS)
$ make
```
Enable TUN/TAP on Linux host:
```
$ sudo setcap cap_net_admin+ep ./nuttx
$ sudo ./tools/simhostroute.sh wlan0 on
```
Run netcat server on Linux host:
`$ netcat -l -p 31337`
Run NuttX on Linux host:
```
$ ./nuttx
NuttShell (NSH) NuttX-10.2.0
nsh> ifconfig eth0 10.0.1.2
nsh> ifup eth0
ifup eth0...OK
```
Start Wireshark (or tcpdump) and capture appeared tap0 interface.
Run in NuttX:
```
nsh> dd if=/dev/zero of=/tmp/test.bin count=1000
nsh> netcat LINUX_HOST_IP_ADDRESS 31337 /tmp/test.bin
```
Observe packet loss -> TCP retransmissions in TCP dump.
Shutdown NuttX:
`nsh> poweroff`
Disable TUN/TAP on Linux host:
`$ sudo ./tools/simhostroute.sh wlan0 off`
Deactivate emulating packet loss on Linux host:
`$ sudo iptables -D INPUT -p tcp --dport 5471 -m statistic --mode random --probability 0.01 -j DROP`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #5272: net/tcp/sendfile: retransmit only one the earliest not acknowledged segment
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #5272:
URL: https://github.com/apache/incubator-nuttx/pull/5272
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev edited a comment on pull request #5272: net/tcp/sendfile: retransmit only one the earliest not acknowledged segment
Posted by GitBox <gi...@apache.org>.
a-lunev edited a comment on pull request #5272:
URL: https://github.com/apache/incubator-nuttx/pull/5272#issuecomment-1017523763
> LGTM. @a-lunev Seems the TCP stack adds too much of sendfile handling, I do not think this is a good trend, since tcp is inherently streaming, the stack should not need to know if a file is being to sent, could we try to hide these details in the tcp_sendfile.c?
@anchao If you mean the introduced conn->sendfile variable in #5239, unfortunately NET_TCP_WRITE_BUFFERS and NET_SENDFILE modes were totally inconsistent to each other (some part of tcp_sendfile relied on NET_TCP_WRITE_BUFFERS=n functionality and the other part of tcp_sendfile relied on NET_TCP_WRITE_BUFFERS=y functionality, however it is a nonsense).
I have analyzed NuttX git repo history and concluded that these two simultaneously enabled options (NET_TCP_WRITE_BUFFERS and NET_SENDFILE) have never worked in NuttX (never supported by design), however some of the boards (sama5 and sim based) has these two options simultaneously enabled. That means tcp_sendfile was broken for those boards and could not work for any other board if these two options got enabled in the same config.
In #5239 I supported the possibility to use NET_SENDFILE option if NET_TCP_WRITE_BUFFERS=y, and introducing conn->sendfile variable was a required measure to do that.
On another hand, I am going to rework tcp_send_buffered.c as well, or at least to do a mutual unification of all 3 parts: tcp_send_buffered.c, tcp_send_unbuffered.c and tcp_sendfile.c. As a result of the intended unification I expect that the conn->sendfile variable may be removed at a later step.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on pull request #5272: net/tcp/sendfile: retransmit only one the earliest not acknowledged segment
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #5272:
URL: https://github.com/apache/incubator-nuttx/pull/5272#issuecomment-1017523763
> LGTM. @a-lunev Seems the TCP stack adds too much of sendfile handling, I do not think this is a good trend, since tcp is inherently streaming, the stack should not need to know if a file is being to sent, could we try to hide these details in the tcp_sendfile.c?
If you mean the introduced conn->sendfile variable in #5239, unfortunately NET_TCP_WRITE_BUFFERS and NET_SENDFILE modes were totally inconsistent to each other (some part of tcp_sendfile relied on NET_TCP_WRITE_BUFFERS=n functionality and the other part of tcp_sendfile relied on NET_TCP_WRITE_BUFFERS=y functionality, however it is a nonsense).
I have analyzed NuttX git repo history and concluded that these two simultaneously enabled options have never worked in NuttX (never supported by design), however some of the boards (sama5 and sim based) has these two options simultaneously enabled. That means tcp_sendfile was broken for those boards and could not work for any other board if these two options got enabled in the same config.
In #5239 I supported the possibility to use NET_SENDFILE option if NET_TCP_WRITE_BUFFERS=y, and introducing conn->sendfile variable was a required measure to do that.
On another hand, I am going to rework tcp_send_buffered.c as well, or at least to do a mutual unification of all 3 parts: tcp_send_buffered.c, tcp_send_unbuffered.c and tcp_sendfile.c. As a result of the intended unification I expect that the conn->sendfile variable may be removed at a later step.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5272: net/tcp/sendfile: retransmit only one the earliest not acknowledged segment
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5272:
URL: https://github.com/apache/incubator-nuttx/pull/5272#issuecomment-1017526119
Great! Thanks for taking time to unify the code path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev edited a comment on pull request #5272: net/tcp/sendfile: retransmit only one the earliest not acknowledged segment
Posted by GitBox <gi...@apache.org>.
a-lunev edited a comment on pull request #5272:
URL: https://github.com/apache/incubator-nuttx/pull/5272#issuecomment-1017523763
> LGTM. @a-lunev Seems the TCP stack adds too much of sendfile handling, I do not think this is a good trend, since tcp is inherently streaming, the stack should not need to know if a file is being to sent, could we try to hide these details in the tcp_sendfile.c?
@anchao If you mean the introduced conn->sendfile variable in #5239, unfortunately NET_TCP_WRITE_BUFFERS and NET_SENDFILE modes were totally inconsistent to each other (some part of tcp_sendfile relied on NET_TCP_WRITE_BUFFERS=n functionality and the other part of tcp_sendfile relied on NET_TCP_WRITE_BUFFERS=y functionality, however it is a nonsense).
I have analyzed NuttX git repo history and concluded that these two simultaneously enabled options (NET_TCP_WRITE_BUFFERS and NET_SENDFILE) have never worked in NuttX (never supported by design), however some of the boards (sama5 and sim based) have these two options simultaneously enabled. That means tcp_sendfile was broken for those boards and could not work for any other board if these two options got enabled in the same config.
In #5239 I supported the possibility to use NET_SENDFILE option if NET_TCP_WRITE_BUFFERS=y, and introducing conn->sendfile variable was a required measure to do that.
On another hand, I am going to rework tcp_send_buffered.c as well, or at least to do a mutual unification of all 3 parts: tcp_send_buffered.c, tcp_send_unbuffered.c and tcp_sendfile.c. As a result of the intended unification I expect that the conn->sendfile variable may be removed at a later step.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on pull request #5272: net/tcp/sendfile: retransmit only one the earliest not acknowledged segment
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #5272:
URL: https://github.com/apache/incubator-nuttx/pull/5272#issuecomment-1017109991
@acassis I created PR #5285. Please, check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #5272: net/tcp/sendfile: retransmit only one the earliest not acknowledged segment
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #5272:
URL: https://github.com/apache/incubator-nuttx/pull/5272#issuecomment-1016825536
@a-lunev your Testing was so nice that I think you could transform it on a guide at https://nuttx.apache.org/docs/latest/guides/index.html to explain how to simulate package lost. What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org