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/16 05:18:15 UTC
[GitHub] [incubator-nuttx] a-lunev opened a new pull request #5239: net/tcp/sendfile: NET_TCP_WRITE_BUFFERS and NET_SENDFILE were inconsistent with each other
a-lunev opened a new pull request #5239:
URL: https://github.com/apache/incubator-nuttx/pull/5239
## Summary
tcp_sendfile() reads data directly from a file and does not use NET_TCP_WRITE_BUFFERS data flow even if CONFIG_NET_TCP_WRITE_BUFFERS option is enabled.
Despite this, tcp_sendfile relied on NET_TCP_WRITE_BUFFERS specific flow control variables that were idle during sendfile operation. Thus it was a total inconsistency.
E.g. because of the issue, TCP socket used by sendfile() operation never issued FIN packet on close() command, and the TCP connection hung up.
This PR fixes the inconsistency issues. Now simultaneously enabled CONFIG_NET_TCP_WRITE_BUFFERS and CONFIG_NET_SENDFILE options can coexist.
## Impact
TCP
## Testing
Build NuttX:
```
$ ./tools/configure.sh -l sim:tcpblaster
$ make menuconfig (enable CONFIG_NETUTILS_NETCAT_SENDFILE, enable 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 TCP dump.
Shutdown NuttX:
`nsh> poweroff`
Disable TUN/TAP on Linux host:
`$ sudo ./tools/simhostroute.sh wlan0 off`
--
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 a change in pull request #5239: net/tcp/sendfile: NET_TCP_WRITE_BUFFERS and NET_SENDFILE were inconsistent with each other
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5239:
URL: https://github.com/apache/incubator-nuttx/pull/5239#discussion_r785444768
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -495,6 +495,9 @@ ssize_t tcp_sendfile(FAR struct socket *psock, FAR struct file *infile,
*/
net_lock();
+#ifdef CONFIG_NET_TCP_WRITE_BUFFERS
+ conn->sendfile = true;
Review comment:
I have the following plan concerning TCP rework:
1. Fix already found issues in tcp_sendfile. One of the fix is the current PR #5239.
2. Also I have almost prepared the next patch that fixes another issue in tcp_sendfile (+ code optimization), and at the same time the TCP payload throughput of tcp_sendfile is increased from 234 Mbits/s up to about 432 Mbit/s (sim:tcpblaster config on my Linux setup).
3. Fix the rest of already found issues in tcp_sendfile.
4. Try to modify tcp_send_unbuffered.c to achieve similar TCP payload throughput as already achieved for tcp_sendfile.
5. Try to modify tcp_send_buffered.c to achieve similar TCP payload throughput as already achieved for tcp_sendfile.
6. If TCP payload throughput for tcp_send_buffered.c is achieved at least as tcp_sendfile already shows, then I could try to simulate tcp_sendfile over tcp_send_buffered.c (CONFIG_NET_TCP_WRITE_BUFFERS=y) and check if the throughput is not reduced because of the simulation.
Right now in case of sim:tcpblaster config the TCP throughput of tcp_send_buffered.c is only 36.7 Kbit/s. If I try to simulate tcp_sendfile over tcp_send_buffered.c right now, the throughput of tcp_sendfile will not be higher 36.7 Kbit/s for sure.
Thus I'm suggesting to go step by step. I'm going to prepare the patches as I outlined.
--
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 #5239: net/tcp/sendfile: NET_TCP_WRITE_BUFFERS and NET_SENDFILE were inconsistent with each other
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #5239:
URL: https://github.com/apache/incubator-nuttx/pull/5239
--
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 a change in pull request #5239: net/tcp/sendfile: NET_TCP_WRITE_BUFFERS and NET_SENDFILE were inconsistent with each other
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5239:
URL: https://github.com/apache/incubator-nuttx/pull/5239#discussion_r785413378
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -495,6 +495,9 @@ ssize_t tcp_sendfile(FAR struct socket *psock, FAR struct file *infile,
*/
net_lock();
+#ifdef CONFIG_NET_TCP_WRITE_BUFFERS
+ conn->sendfile = true;
Review comment:
should we let sendfile simulate the normal send behavior to avoid adding a new flag?
--
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 a change in pull request #5239: net/tcp/sendfile: NET_TCP_WRITE_BUFFERS and NET_SENDFILE were inconsistent with each other
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5239:
URL: https://github.com/apache/incubator-nuttx/pull/5239#discussion_r785457753
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -495,6 +495,9 @@ ssize_t tcp_sendfile(FAR struct socket *psock, FAR struct file *infile,
*/
net_lock();
+#ifdef CONFIG_NET_TCP_WRITE_BUFFERS
+ conn->sendfile = true;
Review comment:
Concerning PR #5138 I think there is another situation. I expect that controlling sndseq can be done the same way (unified) without dependence on CONFIG_NET_TCP_WRITE_BUFFERS option. So far I have not touched the buffered mode concerning sndseq control as it seems working. However, I'm going to consider unification of how sndseq is controlled as well.
--
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 a change in pull request #5239: net/tcp/sendfile: NET_TCP_WRITE_BUFFERS and NET_SENDFILE were inconsistent with each other
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5239:
URL: https://github.com/apache/incubator-nuttx/pull/5239#discussion_r785433938
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -495,6 +495,9 @@ ssize_t tcp_sendfile(FAR struct socket *psock, FAR struct file *infile,
*/
net_lock();
+#ifdef CONFIG_NET_TCP_WRITE_BUFFERS
+ conn->sendfile = true;
Review comment:
Do you mean to simulate it only in case of "CONFIG_NET_TCP_WRITE_BUFFERS=y" mode?
--
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 a change in pull request #5239: net/tcp/sendfile: NET_TCP_WRITE_BUFFERS and NET_SENDFILE were inconsistent with each other
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5239:
URL: https://github.com/apache/incubator-nuttx/pull/5239#discussion_r785473882
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -495,6 +495,9 @@ ssize_t tcp_sendfile(FAR struct socket *psock, FAR struct file *infile,
*/
net_lock();
+#ifdef CONFIG_NET_TCP_WRITE_BUFFERS
+ conn->sendfile = true;
Review comment:
Thanks for taking the effort to unify the code path to simplify the maintain in the future.
--
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 a change in pull request #5239: net/tcp/sendfile: NET_TCP_WRITE_BUFFERS and NET_SENDFILE were inconsistent with each other
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5239:
URL: https://github.com/apache/incubator-nuttx/pull/5239#discussion_r785449369
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -495,6 +495,9 @@ ssize_t tcp_sendfile(FAR struct socket *psock, FAR struct file *infile,
*/
net_lock();
+#ifdef CONFIG_NET_TCP_WRITE_BUFFERS
+ conn->sendfile = true;
Review comment:
So, the different code path from this PR and #5138 will merge again in the final?
--
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 a change in pull request #5239: net/tcp/sendfile: NET_TCP_WRITE_BUFFERS and NET_SENDFILE were inconsistent with each other
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5239:
URL: https://github.com/apache/incubator-nuttx/pull/5239#discussion_r785457753
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -495,6 +495,9 @@ ssize_t tcp_sendfile(FAR struct socket *psock, FAR struct file *infile,
*/
net_lock();
+#ifdef CONFIG_NET_TCP_WRITE_BUFFERS
+ conn->sendfile = true;
Review comment:
Concerning PR #5138 I think there is another situation. I expect that controlling sndseq can be done the same way (unified) without dependence on CONFIG_NET_TCP_WRITE_BUFFERS option. So far I have not touched the buffered mode concerning sndseq control as it seems work. However, I'm going to consider unification of how sndseq is controlled as well.
--
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 a change in pull request #5239: net/tcp/sendfile: NET_TCP_WRITE_BUFFERS and NET_SENDFILE were inconsistent with each other
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5239:
URL: https://github.com/apache/incubator-nuttx/pull/5239#discussion_r785444768
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -495,6 +495,9 @@ ssize_t tcp_sendfile(FAR struct socket *psock, FAR struct file *infile,
*/
net_lock();
+#ifdef CONFIG_NET_TCP_WRITE_BUFFERS
+ conn->sendfile = true;
Review comment:
I have the following plan concerning TCP rework:
1. Fix already found issues in tcp_sendfile. One of the fix is the current PR #5239.
2. Also I have almost prepared the next patch that fixes another issue in tcp_sendfile (+ code optimization), and at the same time the TCP payload throughput of tcp_sendfile is increased from 234 Mbits/s up to about 432 Mbit/s (sim:tcpblaster config on my Linux setup).
3. Fix the rest of already found issues in tcp_sendfile.
4. Try to modify tcp_send_unbuffered.c to achieve similar TCP payload throughput as already achieved for tcp_sendfile.
5. Try to modify tcp_send_buffered.c to achieve similar TCP payload throughput as already achieved for tcp_sendfile.
6. If TCP payload throughput for tcp_send_buffered.c is achieved at least as tcp_sendfile already shows, then I could try to simulate tcp_sendfile over tcp_send_buffered.c (CONFIG_NET_TCP_WRITE_BUFFERS=y) and check if the throughput is not reduced because of the simulation.
Right now in case of sim:tcpblaster config the TCP throughput is only 36.7 Kbit/s. If I try to simulate tcp_sendfile over tcp_send_buffered.c right now, the throughput of tcp_sendfile will not be higher 36.7 Kbit/s for sure.
Thus I'm suggesting to go step by step. I'm going to prepare the patches as I outlined.
--
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 a change in pull request #5239: net/tcp/sendfile: NET_TCP_WRITE_BUFFERS and NET_SENDFILE were inconsistent with each other
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5239:
URL: https://github.com/apache/incubator-nuttx/pull/5239#discussion_r785457753
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -495,6 +495,9 @@ ssize_t tcp_sendfile(FAR struct socket *psock, FAR struct file *infile,
*/
net_lock();
+#ifdef CONFIG_NET_TCP_WRITE_BUFFERS
+ conn->sendfile = true;
Review comment:
This PR (#5239) and PR #5138 are unrelated issues.
Concerning PR #5138 there is another situation. I expect that controlling sndseq can be done the same way (unified) without dependence on CONFIG_NET_TCP_WRITE_BUFFERS option. So far I have not touched the buffered mode concerning sndseq control as it seems working. However, I'm going to consider unification of how sndseq is controlled as well.
--
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 a change in pull request #5239: net/tcp/sendfile: NET_TCP_WRITE_BUFFERS and NET_SENDFILE were inconsistent with each other
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5239:
URL: https://github.com/apache/incubator-nuttx/pull/5239#discussion_r785471450
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -495,6 +495,9 @@ ssize_t tcp_sendfile(FAR struct socket *psock, FAR struct file *infile,
*/
net_lock();
+#ifdef CONFIG_NET_TCP_WRITE_BUFFERS
+ conn->sendfile = true;
Review comment:
> 2\. Also I have almost prepared the next patch that fixes another issue in tcp_sendfile (+ code optimization), and at the same time the TCP payload throughput of tcp_sendfile is increased from 234 Mbits/s up to about 432 Mbit/s (sim:tcpblaster config on my Linux setup).
I created PR #5242.
--
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