This is an automated email from the ASF dual-hosted git repository. xiaoxiang pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/incubator-nuttx.git
The following commit(s) were added to refs/heads/master by this push: new 1e25602 net/can,icmp,icmpv6,tcp,tcp_timer,udp: device should poll only those connections that are bound to the device. tcp_timer: eliminated false decrements of conn->timer in case of multiple network adapters. The false timer decrements sometimes provoked TCP spurious retransmissions due to premature timeouts. 1e25602 is described below commit 1e2560267898f413c93c5fb616ddc5b1d4d07184 Author: Alexander Lunev <alexanderlu...@mail.ru> AuthorDate: Sat Oct 2 03:47:08 2021 +0300 net/can,icmp,icmpv6,tcp,tcp_timer,udp: device should poll only those connections that are bound to the device. tcp_timer: eliminated false decrements of conn->timer in case of multiple network adapters. The false timer decrements sometimes provoked TCP spurious retransmissions due to premature timeouts. --- net/can/can_poll.c | 29 ++--- net/devif/devif_poll.c | 101 +++++++++++------- net/icmp/icmp_poll.c | 5 + net/icmpv6/icmpv6_poll.c | 6 ++ net/tcp/tcp_devpoll.c | 50 ++++----- net/tcp/tcp_timer.c | 267 ++++++++++++++++++++++------------------------- net/udp/udp_devpoll.c | 11 ++ 7 files changed, 247 insertions(+), 222 deletions(-) diff --git a/net/can/can_poll.c b/net/can/can_poll.c index c968fe2..3caed22 100644 --- a/net/can/can_poll.c +++ b/net/can/can_poll.c @@ -26,6 +26,7 @@ #include <nuttx/config.h> #if defined(CONFIG_NET) && defined(CONFIG_NET_CAN) +#include <assert.h> #include <debug.h> #include <nuttx/net/netconfig.h> @@ -53,31 +54,31 @@ * * Assumptions: * The network is locked. + * dev is not NULL. + * conn is not NULL. + * The connection (conn) is bound to the polling device (dev). * ****************************************************************************/ void can_poll(FAR struct net_driver_s *dev, FAR struct can_conn_s *conn) { - /* Verify that the packet connection is valid */ + DEBUGASSERT(dev != NULL && conn != NULL && dev == conn->dev); - if (conn != NULL) - { - /* Setup for the application callback */ + /* Setup for the application callback */ - dev->d_appdata = &dev->d_buf[NET_LL_HDRLEN(dev)]; - dev->d_len = 0; - dev->d_sndlen = 0; + dev->d_appdata = &dev->d_buf[NET_LL_HDRLEN(dev)]; + dev->d_len = 0; + dev->d_sndlen = 0; - /* Perform the application callback */ + /* Perform the application callback */ - can_callback(dev, conn, CAN_POLL); + can_callback(dev, conn, CAN_POLL); - /* Check if the application has data to send */ + /* Check if the application has data to send */ - if (dev->d_sndlen > 0) - { - return; - } + if (dev->d_sndlen > 0) + { + return; } /* Make sure that d_len is zero meaning that there is nothing to be sent */ diff --git a/net/devif/devif_poll.c b/net/devif/devif_poll.c index 6865742..4b14c73 100644 --- a/net/devif/devif_poll.c +++ b/net/devif/devif_poll.c @@ -248,10 +248,12 @@ static int devif_poll_can_connections(FAR struct net_driver_s *dev, while (!bstop && (can_conn = can_nextconn(can_conn))) { - /* Perform the packet TX poll */ + /* Skip connections that are bound to other polling devices */ if (dev == can_conn->dev) { + /* Perform the packet TX poll */ + can_poll(dev, can_conn); /* Call back into the driver */ @@ -361,17 +363,22 @@ static inline int devif_poll_icmp(FAR struct net_driver_s *dev, while (!bstop && (conn = icmp_nextconn(conn)) != NULL) { - /* Perform the ICMP poll */ + /* Skip ICMP connections that are bound to other polling devices */ - icmp_poll(dev, conn); + if (dev == conn->dev) + { + /* Perform the ICMP poll */ - /* Perform any necessary conversions on outgoing packets */ + icmp_poll(dev, conn); - devif_packet_conversion(dev, DEVIF_ICMP); + /* Perform any necessary conversions on outgoing packets */ - /* Call back into the driver */ + devif_packet_conversion(dev, DEVIF_ICMP); - bstop = callback(dev); + /* Call back into the driver */ + + bstop = callback(dev); + } } return bstop; @@ -393,27 +400,32 @@ static inline int devif_poll_icmpv6(FAR struct net_driver_s *dev, FAR struct icmpv6_conn_s *conn = NULL; int bstop = 0; - /* Traverse all of the allocated ICMPV6 connections and perform the poll + /* Traverse all of the allocated ICMPv6 connections and perform the poll * action. */ do { - /* Perform the ICMPV6 poll - * Note: conn equal NULL in the first iteration means poll dev's - * callback list since icmpv6_autoconfig and icmpv6_neighbor still - * append it's callback into this list. - */ + /* Skip ICMPv6 connections that are bound to other polling devices */ - icmpv6_poll(dev, conn); + if (conn == NULL || dev == conn->dev) + { + /* Perform the ICMPV6 poll + * Note: conn equal NULL in the first iteration means poll dev's + * callback list since icmpv6_autoconfig and icmpv6_neighbor still + * append it's callback into this list. + */ - /* Perform any necessary conversions on outgoing packets */ + icmpv6_poll(dev, conn); - devif_packet_conversion(dev, DEVIF_ICMP6); + /* Perform any necessary conversions on outgoing packets */ - /* Call back into the driver */ + devif_packet_conversion(dev, DEVIF_ICMP6); - bstop = callback(dev); + /* Call back into the driver */ + + bstop = callback(dev); + } } while (!bstop && (conn = icmpv6_nextconn(conn)) != NULL); @@ -533,17 +545,24 @@ static int devif_poll_udp_connections(FAR struct net_driver_s *dev, while (!bstop && (conn = udp_nextconn(conn))) { - /* Perform the UDP TX poll */ +#ifdef CONFIG_NET_UDP_WRITE_BUFFERS + /* Skip UDP connections that are bound to other polling devices */ - udp_poll(dev, conn); + if (dev == conn->dev) +#endif + { + /* Perform the UDP TX poll */ - /* Perform any necessary conversions on outgoing packets */ + udp_poll(dev, conn); - devif_packet_conversion(dev, DEVIF_UDP); + /* Perform any necessary conversions on outgoing packets */ - /* Call back into the driver */ + devif_packet_conversion(dev, DEVIF_UDP); - bstop = callback(dev); + /* Call back into the driver */ + + bstop = callback(dev); + } } return bstop; @@ -573,17 +592,22 @@ static inline int devif_poll_tcp_connections(FAR struct net_driver_s *dev, while (!bstop && (conn = tcp_nextconn(conn))) { - /* Perform the TCP TX poll */ + /* Skip TCP connections that are bound to other polling devices */ - tcp_poll(dev, conn); + if (dev == conn->dev) + { + /* Perform the TCP TX poll */ - /* Perform any necessary conversions on outgoing packets */ + tcp_poll(dev, conn); - devif_packet_conversion(dev, DEVIF_TCP); + /* Perform any necessary conversions on outgoing packets */ - /* Call back into the driver */ + devif_packet_conversion(dev, DEVIF_TCP); - bstop = callback(dev); + /* Call back into the driver */ + + bstop = callback(dev); + } } return bstop; @@ -619,17 +643,22 @@ static inline int devif_poll_tcp_timer(FAR struct net_driver_s *dev, while (!bstop && (conn = tcp_nextconn(conn))) { - /* Perform the TCP timer poll */ + /* Skip TCP connections that are bound to other polling devices */ - tcp_timer(dev, conn, hsec); + if (dev == conn->dev) + { + /* Perform the TCP timer poll */ - /* Perform any necessary conversions on outgoing packets */ + tcp_timer(dev, conn, hsec); - devif_packet_conversion(dev, DEVIF_TCP); + /* Perform any necessary conversions on outgoing packets */ - /* Call back into the driver */ + devif_packet_conversion(dev, DEVIF_TCP); - bstop = callback(dev); + /* Call back into the driver */ + + bstop = callback(dev); + } } return bstop; diff --git a/net/icmp/icmp_poll.c b/net/icmp/icmp_poll.c index f0f76c6..bd0f761 100644 --- a/net/icmp/icmp_poll.c +++ b/net/icmp/icmp_poll.c @@ -54,11 +54,16 @@ * * Assumptions: * The network is locked. + * dev is not NULL. + * conn is not NULL. + * The connection (conn) is bound to the polling device (dev). * ****************************************************************************/ void icmp_poll(FAR struct net_driver_s *dev, FAR struct icmp_conn_s *conn) { + DEBUGASSERT(dev != NULL && conn != NULL && dev == conn->dev); + /* Setup for the application callback */ dev->d_appdata = &dev->d_buf[NET_LL_HDRLEN(dev) + IPICMP_HDRLEN]; diff --git a/net/icmpv6/icmpv6_poll.c b/net/icmpv6/icmpv6_poll.c index 63d89f6..5124739 100644 --- a/net/icmpv6/icmpv6_poll.c +++ b/net/icmpv6/icmpv6_poll.c @@ -53,12 +53,18 @@ * * Assumptions: * The network is locked. + * dev is not NULL. + * conn may be NULL. + * The connection (conn), if not NULL, is bound to + * the polling device (dev). * ****************************************************************************/ void icmpv6_poll(FAR struct net_driver_s *dev, FAR struct icmpv6_conn_s *conn) { + DEBUGASSERT(dev != NULL && (conn == NULL || dev == conn->dev)); + /* Setup for the application callback */ dev->d_appdata = &dev->d_buf[NET_LL_HDRLEN(dev) + IPICMPv6_HDRLEN]; diff --git a/net/tcp/tcp_devpoll.c b/net/tcp/tcp_devpoll.c index e1b7da2..30fd278 100644 --- a/net/tcp/tcp_devpoll.c +++ b/net/tcp/tcp_devpoll.c @@ -74,7 +74,10 @@ * None * * Assumptions: - * Called with the network locked. + * It is called with the network locked. + * dev is not NULL. + * conn is not NULL. + * The connection (conn) is bound to the polling device (dev). * ****************************************************************************/ @@ -82,6 +85,8 @@ void tcp_poll(FAR struct net_driver_s *dev, FAR struct tcp_conn_s *conn) { uint16_t result; + DEBUGASSERT(dev != NULL && conn != NULL && dev == conn->dev); + /* Discard any currently buffered data */ dev->d_len = 0; @@ -91,43 +96,34 @@ void tcp_poll(FAR struct net_driver_s *dev, FAR struct tcp_conn_s *conn) if ((conn->tcpstateflags & TCP_STATE_MASK) == TCP_ESTABLISHED) { - /* The TCP connection is established and, hence, should be bound - * to a device. Make sure that the polling device is the one that - * we are bound to. + /* Set up for the callback. We can't know in advance if the + * application is going to send a IPv4 or an IPv6 packet, so this + * setup may not actually be used. */ - DEBUGASSERT(conn->dev != NULL); - if (dev == conn->dev) - { - /* Set up for the callback. We can't know in advance if the - * application is going to send a IPv4 or an IPv6 packet, so this - * setup may not actually be used. - */ - #if defined(CONFIG_NET_IPv6) && defined(CONFIG_NET_IPv4) - if (conn->domain == PF_INET) - { - tcp_ipv4_select(dev); - } - else - { - tcp_ipv6_select(dev); - } + if (conn->domain == PF_INET) + { + tcp_ipv4_select(dev); + } + else + { + tcp_ipv6_select(dev); + } #elif defined(CONFIG_NET_IPv4) - tcp_ipv4_select(dev); + tcp_ipv4_select(dev); #else /* if defined(CONFIG_NET_IPv6) */ - tcp_ipv6_select(dev); + tcp_ipv6_select(dev); #endif - /* Perform the callback */ + /* Perform the callback */ - result = tcp_callback(dev, conn, TCP_POLL); + result = tcp_callback(dev, conn, TCP_POLL); - /* Handle the callback response */ + /* Handle the callback response */ - tcp_appsend(dev, conn, result); - } + tcp_appsend(dev, conn, result); } } diff --git a/net/tcp/tcp_timer.c b/net/tcp/tcp_timer.c index 4e8fc13..7ecb987 100644 --- a/net/tcp/tcp_timer.c +++ b/net/tcp/tcp_timer.c @@ -94,6 +94,9 @@ * * Assumptions: * The network is locked. + * dev is not NULL. + * conn is not NULL. + * The connection (conn) is bound to the polling device (dev). * ****************************************************************************/ @@ -103,6 +106,14 @@ void tcp_timer(FAR struct net_driver_s *dev, FAR struct tcp_conn_s *conn, uint16_t result; uint8_t hdrlen; + /* NOTE: It is important to decrease conn->timer at "hsec" pace, + * not faster. Excessive (false) decrements of conn->timer are not allowed + * here. Otherwise, it breaks TCP timings and leads to TCP spurious + * retransmissions and other issues due to premature timeouts. + */ + + DEBUGASSERT(dev != NULL && conn != NULL && dev == conn->dev); + /* Set up for the callback. We can't know in advance if the application * is going to send a IPv4 or an IPv6 packet, so this setup may not * actually be used. Furthermore, the TCP logic is required to call @@ -140,6 +151,13 @@ void tcp_timer(FAR struct net_driver_s *dev, FAR struct tcp_conn_s *conn, dev->d_len = 0; dev->d_sndlen = 0; + if (conn->tcpstateflags == TCP_CLOSED) + { + /* Nothing to be done */ + + return; + } + /* Check if the connection is in a state in which we simply wait * for the connection to time out. If so, we increase the * connection's timer and remove the connection if it times @@ -162,30 +180,13 @@ void tcp_timer(FAR struct net_driver_s *dev, FAR struct tcp_conn_s *conn, /* Set the timer to the maximum value */ conn->timer = TCP_TIME_WAIT_TIMEOUT * HSEC_PER_SEC; + conn->tcpstateflags = TCP_CLOSED; - /* The TCP connection was established and, hence, should be bound - * to a device. Make sure that the polling device is the one that - * we are bound to. - * - * If not, then we will catch the timeout on the next poll from - * the correct device. - */ - - DEBUGASSERT(conn->dev != NULL); - if (dev != conn->dev) - { - ninfo("TCP: TCP_CLOSED pending\n"); - } - else - { - conn->tcpstateflags = TCP_CLOSED; - - /* Notify upper layers about the timeout */ + /* Notify upper layers about the timeout */ - tcp_callback(dev, conn, TCP_TIMEDOUT); + tcp_callback(dev, conn, TCP_TIMEDOUT); - ninfo("TCP state: TCP_CLOSED\n"); - } + ninfo("TCP state: TCP_CLOSED\n"); } else { @@ -217,21 +218,6 @@ void tcp_timer(FAR struct net_driver_s *dev, FAR struct tcp_conn_s *conn, conn->timer = 0; - /* The TCP is connected and, hence, should be bound to a - * device. Make sure that the polling device is the one that - * we are bound to. - * - * If not, then we will catch the timeout on the next poll - * from the correct device. - */ - - DEBUGASSERT(conn->dev != NULL); - if (dev != conn->dev) - { - ninfo("TCP: TCP_CLOSED pending\n"); - goto done; - } - /* Check for a timeout on connection in the TCP_SYN_RCVD state. * On such timeouts, we would normally resend the SYNACK until * the ACK is received, completing the 3-way handshake. But if @@ -370,157 +356,148 @@ void tcp_timer(FAR struct net_driver_s *dev, FAR struct tcp_conn_s *conn, else if ((conn->tcpstateflags & TCP_STATE_MASK) == TCP_ESTABLISHED) { - /* The TCP connection is established and, hence, should be bound - * to a device. Make sure that the polling device is the one that - * we are bound to. - */ +#ifdef CONFIG_NET_TCP_KEEPALIVE + /* Is this an established connected with KeepAlive enabled? */ - DEBUGASSERT(conn->dev != NULL); - if (dev == conn->dev) + if (conn->keepalive) { -#ifdef CONFIG_NET_TCP_KEEPALIVE - /* Is this an established connected with KeepAlive enabled? */ + socktimeo_t timeo; + uint32_t saveseq; - if (conn->keepalive) + /* If this is the first probe, then the keepstart time is + * the time that the last ACK or data was received from the + * remote. + * + * On subsequent retries, keepstart is the time that the + * last probe was sent. + */ + + if (conn->keepretries > 0) { - socktimeo_t timeo; - uint32_t saveseq; - - /* If this is the first probe, then the keepstart time is - * the time that the last ACK or data was received from the - * remote. - * - * On subsequent retries, keepstart is the time that the - * last probe was sent. - */ + timeo = (socktimeo_t)conn->keepintvl; + } + else + { + timeo = (socktimeo_t)conn->keepidle; + } - if (conn->keepretries > 0) - { - timeo = (socktimeo_t)conn->keepintvl; - } - else - { - timeo = (socktimeo_t)conn->keepidle; - } + /* Yes... has the idle period elapsed with no data or ACK + * received from the remote peer? + */ - /* Yes... has the idle period elapsed with no data or ACK - * received from the remote peer? - */ + if (net_timeo(conn->keeptime, timeo)) + { + /* Yes.. Has the retry count expired? */ - if (net_timeo(conn->keeptime, timeo)) + if (conn->keepretries >= conn->keepcnt) { - /* Yes.. Has the retry count expired? */ - - if (conn->keepretries >= conn->keepcnt) - { - /* Yes... stop the network monitor, closing the - * connection and all sockets associated with the - * connection. - */ + /* Yes... stop the network monitor, closing the + * connection and all sockets associated with the + * connection. + */ - tcp_stop_monitor(conn, TCP_ABORT); - } - else - { - unsigned int tcpiplen; + tcp_stop_monitor(conn, TCP_ABORT); + } + else + { + unsigned int tcpiplen; - /* No.. we need to send another probe. - * Get the size of the IP and TCP header. - */ + /* No.. we need to send another probe. + * Get the size of the IP and TCP header. + */ #ifdef CONFIG_NET_IPv4 #ifdef CONFIG_NET_IPv6 - if (conn->domain == PF_INET) + if (conn->domain == PF_INET) #endif - { - tcpiplen = IPv4_HDRLEN + TCP_HDRLEN; - } + { + tcpiplen = IPv4_HDRLEN + TCP_HDRLEN; + } #endif #ifdef CONFIG_NET_IPv6 #ifdef CONFIG_NET_IPv4 - else + else #endif - { - tcpiplen = IPv6_HDRLEN + TCP_HDRLEN; - } + { + tcpiplen = IPv6_HDRLEN + TCP_HDRLEN; + } #endif - /* And send the probe. - * The packet we send must have these properties: - * - * - TCP_ACK flag (only) is set. - * - Sequence number is the sequence number of - * previously ACKed data, i.e., the expected - * sequence number minus one. - * - * tcp_send() will send the TCP sequence number as - * conn->sndseq. Rather than creating a new - * interface, we spoof tcp_end() here: - */ + /* And send the probe. + * The packet we send must have these properties: + * + * - TCP_ACK flag (only) is set. + * - Sequence number is the sequence number of + * previously ACKed data, i.e., the expected + * sequence number minus one. + * + * tcp_send() will send the TCP sequence number as + * conn->sndseq. Rather than creating a new + * interface, we spoof tcp_end() here: + */ - saveseq = tcp_getsequence(conn->sndseq); - tcp_setsequence(conn->sndseq, saveseq - 1); + saveseq = tcp_getsequence(conn->sndseq); + tcp_setsequence(conn->sndseq, saveseq - 1); - tcp_send(dev, conn, TCP_ACK, tcpiplen); + tcp_send(dev, conn, TCP_ACK, tcpiplen); - tcp_setsequence(conn->sndseq, saveseq); + tcp_setsequence(conn->sndseq, saveseq); #ifdef CONFIG_NET_TCP_WRITE_BUFFERS - /* Increment the un-ACKed sequence number */ + /* Increment the un-ACKed sequence number */ - conn->sndseq_max++; + conn->sndseq_max++; #endif - /* Update for the next probe */ - - conn->keeptime = clock_systime_ticks(); - conn->keepretries++; - } + /* Update for the next probe */ - goto done; + conn->keeptime = clock_systime_ticks(); + conn->keepretries++; } + + goto done; } + } #endif #ifdef CONFIG_NET_TCP_DELAYED_ACK - /* Handle delayed acknowledgments. Is there a segment with a - * delayed acknowledgment? - */ + /* Handle delayed acknowledgments. Is there a segment with a + * delayed acknowledgment? + */ - if (conn->rx_unackseg > 0) - { - /* Increment the ACK delay. */ + if (conn->rx_unackseg > 0) + { + /* Increment the ACK delay. */ - conn->rx_acktimer += hsec; + conn->rx_acktimer += hsec; - /* Per RFC 1122: "...an ACK should not be excessively - * delayed; in particular, the delay must be less than - * 0.5 seconds..." - */ + /* Per RFC 1122: "...an ACK should not be excessively + * delayed; in particular, the delay must be less than + * 0.5 seconds..." + */ - if (conn->rx_acktimer >= ACK_DELAY) - { - /* Reset the delayed ACK state and send the ACK - * packet. - */ + if (conn->rx_acktimer >= ACK_DELAY) + { + /* Reset the delayed ACK state and send the ACK + * packet. + */ - conn->rx_unackseg = 0; - conn->rx_acktimer = 0; - tcp_synack(dev, conn, TCP_ACK); - goto done; - } + conn->rx_unackseg = 0; + conn->rx_acktimer = 0; + tcp_synack(dev, conn, TCP_ACK); + goto done; } + } #endif - /* There was no need for a retransmission and there was no - * need to probe the remote peer and there was no need to - * send a delayed ACK. We poll the application for new - * outgoing data. - */ + /* There was no need for a retransmission and there was no + * need to probe the remote peer and there was no need to + * send a delayed ACK. We poll the application for new + * outgoing data. + */ - result = tcp_callback(dev, conn, TCP_POLL); - tcp_appsend(dev, conn, result); - goto done; - } + result = tcp_callback(dev, conn, TCP_POLL); + tcp_appsend(dev, conn, result); + goto done; } } diff --git a/net/udp/udp_devpoll.c b/net/udp/udp_devpoll.c index 92eaf04..7796f8f 100644 --- a/net/udp/udp_devpoll.c +++ b/net/udp/udp_devpoll.c @@ -45,6 +45,7 @@ #include <nuttx/config.h> #if defined(CONFIG_NET) && defined(CONFIG_NET_UDP) +#include <assert.h> #include <debug.h> #include <nuttx/net/netconfig.h> @@ -73,11 +74,21 @@ * * Assumptions: * The network is locked. + * dev is not NULL. + * conn is not NULL. + * The connection (conn) is bound to the polling device (dev) in case of + * enabled CONFIG_NET_UDP_WRITE_BUFFERS option. * ****************************************************************************/ void udp_poll(FAR struct net_driver_s *dev, FAR struct udp_conn_s *conn) { + DEBUGASSERT(dev != NULL && conn != NULL); + +#ifdef CONFIG_NET_UDP_WRITE_BUFFERS + DEBUGASSERT(dev == conn->dev); +#endif + /* Verify that the UDP connection is valid */ if (conn->lport != 0)