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 2021/10/02 01:27:49 UTC
[GitHub] [incubator-nuttx] a-lunev opened a new pull request #4639: tcp_timer: eliminated false decrements of conn->timer in case of multiple network adapters.
a-lunev opened a new pull request #4639:
URL: https://github.com/apache/incubator-nuttx/pull/4639
## Summary
Fixed an issue with false decrements of conn->timer in case of multiple network adapters.
The false timer decrements sometimes provoked TCP spurious retransmissions due to premature timeouts.
## Impact
TCP protocol
## Testing
--
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 #4639: tcp_timer: eliminated false decrements of conn->timer in case of multiple network adapters.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #4639:
URL: https://github.com/apache/incubator-nuttx/pull/4639
--
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 #4639: tcp_timer: eliminated false decrements of conn->timer in case of multiple network adapters.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #4639:
URL: https://github.com/apache/incubator-nuttx/pull/4639#discussion_r726274522
##########
File path: net/devif/devif_poll.c
##########
@@ -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)
Review comment:
As I've understood, icmpv6_poll is a special case. It can receive NULL conn argument.
There is explanation of it in comments as follows:
```
/* 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.
*/
```
--
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 pull request #4639: tcp_timer: eliminated false decrements of conn->timer in case of multiple network adapters.
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #4639:
URL: https://github.com/apache/incubator-nuttx/pull/4639#issuecomment-939953021
> @anchao I have added checks for the listed protocols as well.
```
------------------------------------------------------------------------------------
Cleaning...
Configuring...
Disabling CONFIG_ARMV7M_TOOLCHAIN_GNU_EABIL
Enabling CONFIG_ARMV7M_TOOLCHAIN_GNU_EABIL
Building NuttX...
can/can_poll.c: In function 'can_poll':
Error: can/can_poll.c:64:3: error: implicit declaration of function 'DEBUGASSERT' [-Werror=implicit-function-declaration]
64 | DEBUGASSERT(dev != NULL && conn != NULL && dev == conn->dev);
| ^~~~~~~~~~~
```
There is a build break that needs to be resolved,
```
diff --git a/net/can/can_poll.c b/net/can/can_poll.c
index cf2b8dc70d..3caed22544 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>
```
--
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 #4639: tcp_timer: eliminated false decrements of conn->timer in case of multiple network adapters.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4639:
URL: https://github.com/apache/incubator-nuttx/pull/4639#discussion_r726265980
##########
File path: net/tcp/tcp_timer.c
##########
@@ -140,6 +140,29 @@ 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;
+ }
+
+ /* The TCP connection is established and, hence, it should be bound
+ * to given device. Make sure that the polling device is the one that
+ * we are bound to. Otherwise, wait for the next poll from the correct
+ * device.
+ * NOTE: It is also 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(conn->dev != NULL);
+ if (dev != conn->dev)
Review comment:
look like we can merge two patch into one?
--
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 #4639: tcp_timer: eliminated false decrements of conn->timer in case of multiple network adapters.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #4639:
URL: https://github.com/apache/incubator-nuttx/pull/4639#issuecomment-939660471
> Let us only change the protocol that includes dev member restriction:
>
> ```
> tcp_conn_s:
> devif_poll_tcp_timer
> devif_poll_tcp_connections
>
> udp_conn_s: (if defniend (CONFIG_NET_UDP_WRITE_BUFFERS))
> devif_poll_udp_connections
>
> icmpv6_conn_s:
> devif_poll_icmpv6
>
> icmp_conn_s:
> devif_poll_icmp
> ```
@anchao I have added checks for the listed protocols 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 pull request #4639: tcp_timer: eliminated false decrements of conn->timer in case of multiple network adapters.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #4639:
URL: https://github.com/apache/incubator-nuttx/pull/4639#issuecomment-939178536
> LGTM,
>
> Seems that other protocol would be better to include similar device checks, how about add more checks in devif_poll_[icmp/udp/tcp/packet/...]_connections ? I just noticed protocol CAN has already include this check:
>
> https://github.com/apache/incubator-nuttx/blob/master/net/devif/devif_poll.c#L253
Concerning the other protocols, dev field of ***_conn_s structures is not supported in udp (unbuffered), bluetooth, ieee802154 and pkt. I'm not sure if there are more obstacles?
--
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 #4639: tcp_timer: eliminated false decrements of conn->timer in case of multiple network adapters.
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #4639:
URL: https://github.com/apache/incubator-nuttx/pull/4639#issuecomment-936222120
ping @yamt ping @anchao
--
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 pull request #4639: tcp_timer: eliminated false decrements of conn->timer in case of multiple network adapters.
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #4639:
URL: https://github.com/apache/incubator-nuttx/pull/4639#issuecomment-938668810
LGTM,
Seems that other protocol would be better to include similar device checks,
how about add more checks in devif_poll_[icmp/udp/tcp/packet/...]_connections ?
I just noticed protocol CAN has already include this check:
https://github.com/apache/incubator-nuttx/blob/master/net/devif/devif_poll.c#L253
--
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 #4639: tcp_timer: eliminated false decrements of conn->timer in case of multiple network adapters.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #4639:
URL: https://github.com/apache/incubator-nuttx/pull/4639#discussion_r726302647
##########
File path: net/tcp/tcp_timer.c
##########
@@ -140,6 +140,29 @@ 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;
+ }
+
+ /* The TCP connection is established and, hence, it should be bound
+ * to given device. Make sure that the polling device is the one that
+ * we are bound to. Otherwise, wait for the next poll from the correct
+ * device.
+ * NOTE: It is also 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(conn->dev != NULL);
+ if (dev != conn->dev)
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4639: tcp_timer: eliminated false decrements of conn->timer in case of multiple network adapters.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4639:
URL: https://github.com/apache/incubator-nuttx/pull/4639#discussion_r726465948
##########
File path: net/devif/devif_poll.c
##########
@@ -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)
Review comment:
Ok.
--
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 pull request #4639: tcp_timer: eliminated false decrements of conn->timer in case of multiple network adapters.
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #4639:
URL: https://github.com/apache/incubator-nuttx/pull/4639#issuecomment-939211775
> Concerning the other protocols, dev field of ***_conn_s structures is not supported in udp (unbuffered), bluetooth, ieee802154 and pkt. I'm not sure if there are more obstacles?
Let us only change the protocol that includes dev member restriction:
```
tcp_conn_s:
devif_poll_tcp_timer
devif_poll_tcp_connections
udp_conn_s: (if defniend (CONFIG_NET_UDP_WRITE_BUFFERS))
devif_poll_udp_connections
icmpv6_conn_s:
devif_poll_icmpv6
icmp_conn_s:
devif_poll_icmp
```
--
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 #4639: tcp_timer: eliminated false decrements of conn->timer in case of multiple network adapters.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #4639:
URL: https://github.com/apache/incubator-nuttx/pull/4639#issuecomment-939966965
> > @anchao I have added checks for the listed protocols as well.
>
> ```
> ------------------------------------------------------------------------------------
> Cleaning...
> Configuring...
> Disabling CONFIG_ARMV7M_TOOLCHAIN_GNU_EABIL
> Enabling CONFIG_ARMV7M_TOOLCHAIN_GNU_EABIL
> Building NuttX...
> can/can_poll.c: In function 'can_poll':
> Error: can/can_poll.c:64:3: error: implicit declaration of function 'DEBUGASSERT' [-Werror=implicit-function-declaration]
> 64 | DEBUGASSERT(dev != NULL && conn != NULL && dev == conn->dev);
> | ^~~~~~~~~~~
> ```
>
> There is a build break that needs to be resolved,
>
> ```
> diff --git a/net/can/can_poll.c b/net/can/can_poll.c
> index cf2b8dc70d..3caed22544 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>
>
> ```
Yes, I have just added it. 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] xiaoxiang781216 commented on a change in pull request #4639: tcp_timer: eliminated false decrements of conn->timer in case of multiple network adapters.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4639:
URL: https://github.com/apache/incubator-nuttx/pull/4639#discussion_r726265388
##########
File path: net/devif/devif_poll.c
##########
@@ -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)
Review comment:
why ICMP has different condition against ICMPv6?
--
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