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 17:14:37 UTC
[GitHub] [incubator-nuttx] a-lunev opened a new pull request #5242: net/tcp/sendfile: TCP retransmission could not start because of incorrect snd_ackcb callback handling
a-lunev opened a new pull request #5242:
URL: https://github.com/apache/incubator-nuttx/pull/5242
## Summary
Both the snd_ackcb and snd_datacb callbacks were created and destroyed right after sending every packet.
Whenever TCP_REXMIT event occurred due to TCP send timeout, TCP_REXMIT was ignored because
snd_ackcb callback had been destroyed by the time.
The issue is fixed as follows:
- both the snd_ackcb and snd_datacb callbacks are combined into one snd_cb callback
(the same way as in tcp_send_unbuffered.c).
- the snd_cb callback lives until all requested data (via sendfile) is sent,
including all ACKs and possible retransmissions.
As a positive side effect of the code optimization / fix, sendfile TCP payload throughput is increased (the TCP payload throughput of tcp_sendfile is increased from 234 Mbit/s up to about 432 Mbit/s (sim:tcpblaster config on my Linux setup)).
## Impact
TCP sendfile
## 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
```
Start iperf on Linux host:
`$ iperf -s -p 5471 -i 1 -w 416K`
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 netcat in NuttX:
```
nsh> dd if=/dev/zero of=/tmp/test.bin count=1000
nsh> netcat LINUX_HOST_IP_ADDRESS 5471 /tmp/test.bin
```
Observe packet loss -> TCP Retransmissions in TCP dump.
Deactivate emulating packet loss on Linux host:
`$ sudo iptables -D INPUT -p tcp --dport 5471 -m statistic --mode random --probability 0.01 -j DROP`
Run netcat in NuttX:
```
nsh> dd if=/dev/zero of=/tmp/test.bin count=10000
nsh> netcat LINUX_HOST_IP_ADDRESS 5471 /tmp/test.bin
nsh> poweroff
```
iperf on Linux host:
```
$ iperf -s -p 5471 -i 1 -w 416K
------------------------------------------------------------
Server listening on TCP port 5471
TCP window size: 416 KByte
------------------------------------------------------------
[ 4] local 192.168.1.68 port 5471 connected with 10.0.1.2 port 25367
[ ID] Interval Transfer Bandwidth
[ 4] 0.0- 0.1 sec 4.88 MBytes 432 Mbits/sec
```
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 #5242: net/tcp/sendfile: TCP retransmission could not start because of incorrect snd_ackcb callback handling
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5242:
URL: https://github.com/apache/incubator-nuttx/pull/5242#discussion_r786005125
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -320,41 +298,38 @@ static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
tcp_setsequence(conn->sndseq, seqno);
+ /* Notify the device driver of the availability of TX data */
+
+ sendfile_txnotify(psock, conn);
+
/* Update the amount of data sent (but not necessarily ACKed) */
pstate->snd_sent += sndlen;
ninfo("pid: %d SEND: acked=%" PRId32 " sent=%zd flen=%zu\n",
getpid(),
pstate->snd_acked, pstate->snd_sent, pstate->snd_flen);
}
- else
- {
- nwarn("WARNING: Window full, wait for ack\n");
- goto wait;
- }
}
- if (pstate->snd_sent >= pstate->snd_flen
Review comment:
I have just forced-pushed the changes concerning your remarks above. So the check for outstanding acks is moved from line 186 to 251 in the changed code.
--
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 #5242: net/tcp/sendfile: TCP retransmission could not start because of incorrect snd_ackcb callback handling
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5242:
URL: https://github.com/apache/incubator-nuttx/pull/5242#discussion_r786006998
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -87,17 +86,63 @@ struct sendfile_s
uint32_t snd_acked; /* The number of bytes acked */
};
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static inline void sendfile_txnotify(FAR struct socket *psock,
+ FAR struct tcp_conn_s *conn);
+
/****************************************************************************
* Private Functions
****************************************************************************/
-static uint16_t ack_eventhandler(FAR struct net_driver_s *dev,
- FAR void *pvconn,
- FAR void *pvpriv, uint16_t flags)
+/****************************************************************************
+ * Name: sendfile_eventhandler
+ *
+ * Description:
+ * This function is called to perform the actual send operation when
+ * polled by the lower, device interfacing layer.
+ *
+ * Input Parameters:
+ * dev The structure of the network driver that caused the event
+ * conn The connection structure associated with the socket
+ * flags Set of events describing why the callback was invoked
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ * The network is locked
+ *
+ ****************************************************************************/
+
+static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
+ FAR void *pvconn, FAR void *pvpriv,
+ uint16_t flags)
{
+ FAR struct tcp_conn_s *conn = (FAR struct tcp_conn_s *)pvconn;
FAR struct sendfile_s *pstate = (FAR struct sendfile_s *)pvpriv;
+ FAR struct socket *psock = pstate->snd_sock;
+ int ret;
+
+ /* The TCP socket is connected and, hence, should be bound to a device.
+ * Make sure that the polling device is the own that we are bound to.
+ */
+
+ DEBUGASSERT(conn);
Review comment:
Done. Thank you.
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -148,14 +206,15 @@ static uint16_t ack_eventhandler(FAR struct net_driver_s *dev,
*/
pstate->snd_sent = pstate->snd_acked;
+#ifndef CONFIG_NET_TCP_WRITE_BUFFERS
+ conn->rexmit_seq = pstate->snd_sent + pstate->snd_isn;
+#endif
}
/* Check for a loss of connection */
else if ((flags & TCP_DISCONN_EVENTS) != 0)
Review comment:
Done. Thank you.
--
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] anchao commented on a change in pull request #5242: net/tcp/sendfile: TCP retransmission could not start because of incorrect snd_ackcb callback handling
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5242:
URL: https://github.com/apache/incubator-nuttx/pull/5242#discussion_r785653572
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -320,41 +298,38 @@ static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
tcp_setsequence(conn->sndseq, seqno);
+ /* Notify the device driver of the availability of TX data */
+
+ sendfile_txnotify(psock, conn);
+
/* Update the amount of data sent (but not necessarily ACKed) */
pstate->snd_sent += sndlen;
ninfo("pid: %d SEND: acked=%" PRId32 " sent=%zd flen=%zu\n",
getpid(),
pstate->snd_acked, pstate->snd_sent, pstate->snd_flen);
}
- else
- {
- nwarn("WARNING: Window full, wait for ack\n");
Review comment:
let us keep the log on waiting case
--
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 #5242: net/tcp/sendfile: TCP retransmission could not start because of incorrect snd_ackcb callback handling
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5242:
URL: https://github.com/apache/incubator-nuttx/pull/5242#discussion_r786008604
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -87,17 +86,63 @@ struct sendfile_s
uint32_t snd_acked; /* The number of bytes acked */
};
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static inline void sendfile_txnotify(FAR struct socket *psock,
Review comment:
I did not do that to reduce number of lines relocations (avoid the whole sendfile_txnotify() function relocation) to simplify the source code diff / git history.
Though, I have done this now as you requested.
--
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 #5242: net/tcp/sendfile: TCP retransmission could not start because of incorrect snd_ackcb callback handling
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5242:
URL: https://github.com/apache/incubator-nuttx/pull/5242#discussion_r785992413
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -320,41 +298,38 @@ static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
tcp_setsequence(conn->sndseq, seqno);
+ /* Notify the device driver of the availability of TX data */
+
+ sendfile_txnotify(psock, conn);
+
/* Update the amount of data sent (but not necessarily ACKed) */
pstate->snd_sent += sndlen;
ninfo("pid: %d SEND: acked=%" PRId32 " sent=%zd flen=%zu\n",
getpid(),
pstate->snd_acked, pstate->snd_sent, pstate->snd_flen);
}
- else
- {
- nwarn("WARNING: Window full, wait for ack\n");
- goto wait;
- }
}
- if (pstate->snd_sent >= pstate->snd_flen
Review comment:
The check for outstanding acks is in line 186 in the changed code.
--
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] anchao commented on a change in pull request #5242: net/tcp/sendfile: TCP retransmission could not start because of incorrect snd_ackcb callback handling
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5242:
URL: https://github.com/apache/incubator-nuttx/pull/5242#discussion_r785654496
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -320,41 +298,38 @@ static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
tcp_setsequence(conn->sndseq, seqno);
+ /* Notify the device driver of the availability of TX data */
+
+ sendfile_txnotify(psock, conn);
+
/* Update the amount of data sent (but not necessarily ACKed) */
pstate->snd_sent += sndlen;
ninfo("pid: %d SEND: acked=%" PRId32 " sent=%zd flen=%zu\n",
getpid(),
pstate->snd_acked, pstate->snd_sent, pstate->snd_flen);
}
- else
- {
- nwarn("WARNING: Window full, wait for ack\n");
- goto wait;
- }
}
- if (pstate->snd_sent >= pstate->snd_flen
Review comment:
how about outstanding ack case?
--
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] anchao commented on a change in pull request #5242: net/tcp/sendfile: TCP retransmission could not start because of incorrect snd_ackcb callback handling
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5242:
URL: https://github.com/apache/incubator-nuttx/pull/5242#discussion_r785651366
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -87,17 +86,63 @@ struct sendfile_s
uint32_t snd_acked; /* The number of bytes acked */
};
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static inline void sendfile_txnotify(FAR struct socket *psock,
+ FAR struct tcp_conn_s *conn);
+
/****************************************************************************
* Private Functions
****************************************************************************/
-static uint16_t ack_eventhandler(FAR struct net_driver_s *dev,
- FAR void *pvconn,
- FAR void *pvpriv, uint16_t flags)
+/****************************************************************************
+ * Name: sendfile_eventhandler
+ *
+ * Description:
+ * This function is called to perform the actual send operation when
+ * polled by the lower, device interfacing layer.
+ *
+ * Input Parameters:
+ * dev The structure of the network driver that caused the event
+ * conn The connection structure associated with the socket
+ * flags Set of events describing why the callback was invoked
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ * The network is locked
+ *
+ ****************************************************************************/
+
+static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
+ FAR void *pvconn, FAR void *pvpriv,
+ uint16_t flags)
{
+ FAR struct tcp_conn_s *conn = (FAR struct tcp_conn_s *)pvconn;
FAR struct sendfile_s *pstate = (FAR struct sendfile_s *)pvpriv;
+ FAR struct socket *psock = pstate->snd_sock;
+ int ret;
+
+ /* The TCP socket is connected and, hence, should be bound to a device.
+ * Make sure that the polling device is the own that we are bound to.
+ */
+
+ DEBUGASSERT(conn);
Review comment:
move after check TCP_DISCONN_EVENTS
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -148,14 +206,15 @@ static uint16_t ack_eventhandler(FAR struct net_driver_s *dev,
*/
pstate->snd_sent = pstate->snd_acked;
+#ifndef CONFIG_NET_TCP_WRITE_BUFFERS
+ conn->rexmit_seq = pstate->snd_sent + pstate->snd_isn;
+#endif
}
/* Check for a loss of connection */
else if ((flags & TCP_DISCONN_EVENTS) != 0)
Review comment:
check the disconnect event at first segment, the conn instance could be NULL:
https://github.com/apache/incubator-nuttx/blob/master/net/netdev/netdev_carrier.c#L97
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -87,17 +86,63 @@ struct sendfile_s
uint32_t snd_acked; /* The number of bytes acked */
};
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static inline void sendfile_txnotify(FAR struct socket *psock,
Review comment:
move the sendfile_txnotify() implement before caller
--
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 #5242: net/tcp/sendfile: TCP retransmission could not start because of incorrect snd_ackcb callback handling
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5242:
URL: https://github.com/apache/incubator-nuttx/pull/5242#discussion_r786005125
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -320,41 +298,38 @@ static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
tcp_setsequence(conn->sndseq, seqno);
+ /* Notify the device driver of the availability of TX data */
+
+ sendfile_txnotify(psock, conn);
+
/* Update the amount of data sent (but not necessarily ACKed) */
pstate->snd_sent += sndlen;
ninfo("pid: %d SEND: acked=%" PRId32 " sent=%zd flen=%zu\n",
getpid(),
pstate->snd_acked, pstate->snd_sent, pstate->snd_flen);
}
- else
- {
- nwarn("WARNING: Window full, wait for ack\n");
- goto wait;
- }
}
- if (pstate->snd_sent >= pstate->snd_flen
Review comment:
I have just forced-pushed the changes concerning your other remarks. So the check for outstanding acks is moved from line 186 to 251 in the changed code.
--
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 #5242: net/tcp/sendfile: TCP retransmission could not start because of incorrect snd_ackcb callback handling
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #5242:
URL: https://github.com/apache/incubator-nuttx/pull/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
[GitHub] [incubator-nuttx] a-lunev commented on pull request #5242: net/tcp/sendfile: TCP retransmission could not start because of incorrect snd_ackcb callback handling
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #5242:
URL: https://github.com/apache/incubator-nuttx/pull/5242#issuecomment-1013926089
Just now I have removed temporary lines that I used for debug and forgot to remove.
--
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 #5242: net/tcp/sendfile: TCP retransmission could not start because of incorrect snd_ackcb callback handling
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5242:
URL: https://github.com/apache/incubator-nuttx/pull/5242#discussion_r786009323
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -320,41 +298,38 @@ static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
tcp_setsequence(conn->sndseq, seqno);
+ /* Notify the device driver of the availability of TX data */
+
+ sendfile_txnotify(psock, conn);
+
/* Update the amount of data sent (but not necessarily ACKed) */
pstate->snd_sent += sndlen;
ninfo("pid: %d SEND: acked=%" PRId32 " sent=%zd flen=%zu\n",
getpid(),
pstate->snd_acked, pstate->snd_sent, pstate->snd_flen);
}
- else
- {
- nwarn("WARNING: Window full, wait for ack\n");
Review comment:
Done.
--
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