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/07/05 09:35:44 UTC
[GitHub] [incubator-nuttx] anchao opened a new pull request #4070: net/tcp: add window scale support
anchao opened a new pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070
## Summary
net/tcp: add window scale support
net/tcp: change the tcp optdata to dynamic arrays
net/tcp: remove the invalid break during tcp option loop
Reference here:
https://tools.ietf.org/html/rfc1323
## Impact
High performance network throughput request if the receiving window exceeds 64K
## Testing
HTTP streaming
--
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] yamt commented on a change in pull request #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#discussion_r664215474
##########
File path: net/tcp/Kconfig
##########
@@ -106,6 +106,27 @@ config NET_TCP_FAST_RETRANSMIT_WATERMARK
missing segment, without waiting for a retransmission timer to
expire.
+config NET_TCP_WINDOW_SCALE
+ bool "Enable TCP/IP Window Scale Option"
+ default n
+ ---help---
+ RFC1323:
+ 2. TCP WINDOW SCALE OPTION
+ The window scale extension expands the definition of the TCP
+ window to 32 bits and then uses a scale factor to carry this 32-
+ bit value in the 16-bit Window field of the TCP header (SEG.WND in
+ RFC-793).
+
+if NET_TCP_WINDOW_SCALE
+
+config NET_TCP_WINDOW_SCALE_FACTOR
+ int "TCP/IP Window Scale Factor"
+ default 7
Review comment:
i guess 0 is a reasonable default because i suspect 16-bit window is just enough for the majority of setup.
maybe a more reasonable thing is to calculate the minimum value which can represent our max window. (from IOB and RECVBUF configs)
--
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] yamt commented on a change in pull request #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#discussion_r664235030
##########
File path: net/tcp/Kconfig
##########
@@ -106,6 +106,27 @@ config NET_TCP_FAST_RETRANSMIT_WATERMARK
missing segment, without waiting for a retransmission timer to
expire.
+config NET_TCP_WINDOW_SCALE
+ bool "Enable TCP/IP Window Scale Option"
+ default n
+ ---help---
+ RFC1323:
+ 2. TCP WINDOW SCALE OPTION
+ The window scale extension expands the definition of the TCP
+ window to 32 bits and then uses a scale factor to carry this 32-
+ bit value in the 16-bit Window field of the TCP header (SEG.WND in
+ RFC-793).
+
+if NET_TCP_WINDOW_SCALE
+
+config NET_TCP_WINDOW_SCALE_FACTOR
+ int "TCP/IP Window Scale Factor"
+ default 7
Review comment:
@anchao do you want to do this "find a better scaling value" things in this PR?
--
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 #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#discussion_r665025686
##########
File path: net/tcp/tcp_recvwindow.c
##########
@@ -224,14 +203,49 @@ uint16_t tcp_get_recvwindow(FAR struct net_driver_s *dev,
return tcp_calc_rcvsize(conn, recvwndo);
}
+/****************************************************************************
+ * Name: tcp_get_scalewindow
Review comment:
Ok, rename the tcp_get_scalewindow to tcp_get_scaled_recvwindow
--
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 #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#discussion_r664530004
##########
File path: net/tcp/tcp_send.c
##########
@@ -372,6 +384,13 @@ static void tcp_sendcommon(FAR struct net_driver_s *dev,
/* Update the Receiver Window */
+#ifdef CONFIG_NET_TCP_WINDOW_SCALE
+ if (recvwndo > 0 && conn->rcv_scale > 0)
+ {
+ recvwndo <<= conn->rcv_scale;
+ }
+#endif
+
Review comment:
Yes, I added a API tcp_get_scalewindow() to handle UINT16_MAX check and scale calculation
--
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 #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#discussion_r664998575
##########
File path: net/tcp/tcp_recvwindow.c
##########
@@ -127,18 +115,19 @@ static uint16_t tcp_maxrcvwin(FAR struct tcp_conn_s *conn)
* Calculate the TCP receive window for the specified device.
*
* Input Parameters:
- * dev - The device whose TCP receive window will be updated.
+ * dev - The device whose TCP receive window will be updated.
+ * conn - The TCP connection structure holding connection information.
*
* Returned Value:
* The value of the TCP receive window to use.
*
****************************************************************************/
-uint16_t tcp_get_recvwindow(FAR struct net_driver_s *dev,
+uint32_t tcp_get_recvwindow(FAR struct net_driver_s *dev,
FAR struct tcp_conn_s *conn)
{
- uint16_t tailroom;
- uint16_t recvwndo;
+ unsigned int tailroom;
Review comment:
uint32_t
##########
File path: net/tcp/tcp_recvwindow.c
##########
@@ -251,15 +265,21 @@ bool tcp_should_send_recvwindow(FAR struct tcp_conn_s *conn)
oldwin = 0;
}
- win = tcp_get_recvwindow(dev, conn);
+ win = tcp_get_scalewindow(dev, conn);
+#ifdef CONFIG_NET_TCP_WINDOW_SCALE
+ if (conn->rcv_scale > 0)
+ {
+ win <<= conn->rcv_scale;
Review comment:
directly scale
##########
File path: net/tcp/tcp_send.c
##########
@@ -363,23 +363,23 @@ static void tcp_sendcommon(FAR struct net_driver_s *dev,
/* Update the TCP received window based on I/O buffer availability */
uint32_t rcvseq = tcp_getsequence(conn->rcvseq);
- uint16_t recvwndo = tcp_get_recvwindow(dev, conn);
+ uint32_t recvwndo = tcp_get_scalewindow(dev, conn);
- /* Update the Receiver Window */
+ /* Set the TCP Window */
- conn->rcv_adv = rcvseq + recvwndo;
+ tcp->wnd[0] = recvwndo >> 8;
+ tcp->wnd[1] = recvwndo & 0xff;
#ifdef CONFIG_NET_TCP_WINDOW_SCALE
- if (recvwndo > 0 && conn->rcv_scale > 0)
+ if (conn->rcv_scale > 0)
Review comment:
directly simply shift.
##########
File path: net/tcp/tcp_input.c
##########
@@ -440,7 +449,12 @@ static void tcp_input(FAR struct net_driver_s *dev, uint8_t domain,
/* Update the connection's window size */
- conn->snd_wnd = ((uint16_t)tcp->wnd[0] << 8) + (uint16_t)tcp->wnd[1];
+#ifdef CONFIG_NET_TCP_WINDOW_SCALE
+ conn->snd_wnd = (((uint16_t)tcp->wnd[0] << 8) + (uint16_t)tcp->wnd[1]) <<
Review comment:
uint32_t
##########
File path: net/tcp/tcp_send.c
##########
@@ -365,14 +365,21 @@ static void tcp_sendcommon(FAR struct net_driver_s *dev,
uint32_t rcvseq = tcp_getsequence(conn->rcvseq);
uint16_t recvwndo = tcp_get_recvwindow(dev, conn);
+ /* Update the Receiver Window */
+
+ conn->rcv_adv = rcvseq + recvwndo;
+
+#ifdef CONFIG_NET_TCP_WINDOW_SCALE
+ if (recvwndo > 0 && conn->rcv_scale > 0)
Review comment:
it's more simple and fast to shift directly without check
##########
File path: net/tcp/tcp.h
##########
@@ -198,6 +209,7 @@ struct tcp_conn_s
#else
uint16_t tx_unacked; /* Number bytes sent but not yet ACKed */
#endif
+ uint16_t flags; /* Flags of TCP-specific options */
Review comment:
bool wscale;
and remove TCP_WSCALE
##########
File path: net/tcp/tcp_recvwindow.c
##########
@@ -224,14 +203,49 @@ uint16_t tcp_get_recvwindow(FAR struct net_driver_s *dev,
return tcp_calc_rcvsize(conn, recvwndo);
}
+/****************************************************************************
+ * Name: tcp_get_scalewindow
Review comment:
lets' expose only one function here but not both:
1. tcp_get_recvwindow return the bytes(after UINT16_MAX process)
2. or tcp_get_scaled_recvwindow return the scaled value(after UINT16_MAX process too)
##########
File path: net/tcp/tcp_recvwindow.c
##########
@@ -224,14 +203,49 @@ uint16_t tcp_get_recvwindow(FAR struct net_driver_s *dev,
return tcp_calc_rcvsize(conn, recvwndo);
}
+/****************************************************************************
+ * Name: tcp_get_scalewindow
+ *
+ * Description:
+ * Calculate the TCP receive window scale for the specified device.
+ *
+ * Input Parameters:
+ * dev - The device whose TCP receive window will be updated.
+ * conn - The TCP connection structure holding connection information.
+ *
+ * Returned Value:
+ * The value of the TCP receive window scale to use.
+ *
+ ****************************************************************************/
+
+uint32_t tcp_get_scalewindow(FAR struct net_driver_s *dev,
+ FAR struct tcp_conn_s *conn)
+{
+ uint32_t recvwndo = tcp_get_recvwindow(dev, conn);
+
+#ifdef CONFIG_NET_TCP_WINDOW_SCALE
+ if (recvwndo > 0 && conn->rcv_scale > 0)
+ {
+ recvwndo >>= conn->rcv_scale;
Review comment:
directly shift
--
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 #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#issuecomment-874520270
@yamt
I upload a new commit. When the window scale is enabled, the received buffer is easily over 64K. I modified the related data type to uint32_t to avoid the TCP window overflow. Please review again. 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] yamt commented on a change in pull request #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#discussion_r663806067
##########
File path: net/tcp/tcp_send.c
##########
@@ -365,6 +365,13 @@ static void tcp_sendcommon(FAR struct net_driver_s *dev,
uint32_t rcvseq = tcp_getsequence(conn->rcvseq);
uint16_t recvwndo = tcp_get_recvwindow(dev, conn);
+#ifdef CONFIG_NET_TCP_WINDOW_SCALE
+ if (recvwndo > 0 && conn->rcv_scale > 0)
+ {
+ recvwndo >>= conn->rcv_scale;
+ }
+#endif
Review comment:
rcv_adv update below should use the unscaled value
##########
File path: net/tcp/tcp_send.c
##########
@@ -669,11 +677,24 @@ void tcp_synack(FAR struct net_driver_s *dev, FAR struct tcp_conn_s *conn,
/* We send out the TCP Maximum Segment Size option with our ACK. */
- tcp->optdata[0] = TCP_OPT_MSS;
- tcp->optdata[1] = TCP_OPT_MSS_LEN;
- tcp->optdata[2] = tcp_mss >> 8;
- tcp->optdata[3] = tcp_mss & 0xff;
- tcp->tcpoffset = ((TCP_HDRLEN + TCP_OPT_MSS_LEN) / 4) << 4;
+ tcp->optdata[optlen++] = TCP_OPT_MSS;
+ tcp->optdata[optlen++] = TCP_OPT_MSS_LEN;
+ tcp->optdata[optlen++] = tcp_mss >> 8;
+ tcp->optdata[optlen++] = tcp_mss & 0xff;
+
+#ifdef CONFIG_NET_TCP_WINDOW_SCALE
+ if (tcp->flags == TCP_SYN ||
+ ((tcp->flags == (TCP_ACK | TCP_SYN)) && conn->snd_scale))
Review comment:
0 is a valid value for the window scaling option.
i guess you need a separate bit to record "window scaling option received".
##########
File path: net/tcp/Kconfig
##########
@@ -106,6 +106,27 @@ config NET_TCP_FAST_RETRANSMIT_WATERMARK
missing segment, without waiting for a retransmission timer to
expire.
+config NET_TCP_WINDOW_SCALE
+ bool "Enable TCP/IP Window Scale Option"
+ default n
+ ---help---
+ RFC1323:
+ 2. TCP WINDOW SCALE OPTION
+ The window scale extension expands the definition of the TCP
+ window to 32 bits and then uses a scale factor to carry this 32-
+ bit value in the 16-bit Window field of the TCP header (SEG.WND in
+ RFC-793).
+
+if NET_TCP_WINDOW_SCALE
+
+config NET_TCP_WINDOW_SCALE_FACTOR
+ int "TCP/IP Window Scale Factor"
+ default 7
Review comment:
why 7?
##########
File path: net/tcp/Kconfig
##########
@@ -106,6 +106,27 @@ config NET_TCP_FAST_RETRANSMIT_WATERMARK
missing segment, without waiting for a retransmission timer to
expire.
+config NET_TCP_WINDOW_SCALE
+ bool "Enable TCP/IP Window Scale Option"
+ default n
+ ---help---
+ RFC1323:
+ 2. TCP WINDOW SCALE OPTION
+ The window scale extension expands the definition of the TCP
+ window to 32 bits and then uses a scale factor to carry this 32-
+ bit value in the 16-bit Window field of the TCP header (SEG.WND in
+ RFC-793).
+
+if NET_TCP_WINDOW_SCALE
+
+config NET_TCP_WINDOW_SCALE_FACTOR
+ int "TCP/IP Window Scale Factor"
+ default 7
Review comment:
i guess 0 is a reasonable default because i suspect 16-bit window is just enough for the majority of setup.
maybe a more reasonable thing is to calculate the minimum value which can represent our max window. (from IOB and RECVBUF configs)
##########
File path: net/tcp/Kconfig
##########
@@ -106,6 +106,27 @@ config NET_TCP_FAST_RETRANSMIT_WATERMARK
missing segment, without waiting for a retransmission timer to
expire.
+config NET_TCP_WINDOW_SCALE
+ bool "Enable TCP/IP Window Scale Option"
+ default n
+ ---help---
+ RFC1323:
+ 2. TCP WINDOW SCALE OPTION
+ The window scale extension expands the definition of the TCP
+ window to 32 bits and then uses a scale factor to carry this 32-
+ bit value in the 16-bit Window field of the TCP header (SEG.WND in
+ RFC-793).
+
+if NET_TCP_WINDOW_SCALE
+
+config NET_TCP_WINDOW_SCALE_FACTOR
+ int "TCP/IP Window Scale Factor"
+ default 7
Review comment:
> maybe a more reasonable thing is to calculate the minimum value which can represent our max window. (from IOB and RECVBUF configs)
as recv buffer size can change during the lifetime of the connection, maybe just from IOB configurations.
##########
File path: net/tcp/Kconfig
##########
@@ -106,6 +106,27 @@ config NET_TCP_FAST_RETRANSMIT_WATERMARK
missing segment, without waiting for a retransmission timer to
expire.
+config NET_TCP_WINDOW_SCALE
+ bool "Enable TCP/IP Window Scale Option"
+ default n
+ ---help---
+ RFC1323:
+ 2. TCP WINDOW SCALE OPTION
+ The window scale extension expands the definition of the TCP
+ window to 32 bits and then uses a scale factor to carry this 32-
+ bit value in the 16-bit Window field of the TCP header (SEG.WND in
+ RFC-793).
+
+if NET_TCP_WINDOW_SCALE
+
+config NET_TCP_WINDOW_SCALE_FACTOR
+ int "TCP/IP Window Scale Factor"
+ default 7
Review comment:
@anchao do you want to do this "find a better scaling value" things in this PR?
##########
File path: net/tcp/Kconfig
##########
@@ -106,6 +106,27 @@ config NET_TCP_FAST_RETRANSMIT_WATERMARK
missing segment, without waiting for a retransmission timer to
expire.
+config NET_TCP_WINDOW_SCALE
+ bool "Enable TCP/IP Window Scale Option"
+ default n
+ ---help---
+ RFC1323:
+ 2. TCP WINDOW SCALE OPTION
+ The window scale extension expands the definition of the TCP
+ window to 32 bits and then uses a scale factor to carry this 32-
+ bit value in the 16-bit Window field of the TCP header (SEG.WND in
+ RFC-793).
+
+if NET_TCP_WINDOW_SCALE
+
+config NET_TCP_WINDOW_SCALE_FACTOR
+ int "TCP/IP Window Scale Factor"
+ default 7
Review comment:
ok.
then i will merge this later today unless anyone beats me or raises an objection.
(i don't like to merge a complex patch within 24 hours)
##########
File path: net/tcp/tcp_send.c
##########
@@ -372,6 +384,13 @@ static void tcp_sendcommon(FAR struct net_driver_s *dev,
/* Update the Receiver Window */
+#ifdef CONFIG_NET_TCP_WINDOW_SCALE
+ if (recvwndo > 0 && conn->rcv_scale > 0)
+ {
+ recvwndo <<= conn->rcv_scale;
+ }
+#endif
+
Review comment:
adjusting the window for UINT16_MAX and scaling here can confuse tcp_should_send_recvwindow.
i feel it's simpler to do it in tcp_get_recvwindow.
how do you think?
##########
File path: net/sixlowpan/sixlowpan_tcpsend.c
##########
@@ -258,7 +258,12 @@ static int sixlowpan_tcp_header(FAR struct tcp_conn_s *conn,
/* Update the TCP received window based on I/O buffer availability */
uint32_t rcvseq = tcp_getsequence(conn->rcvseq);
- uint16_t recvwndo = tcp_get_recvwindow(dev, conn);
+ uint32_t recvwndo = tcp_get_recvwindow(dev, conn);
+
+ if (recvwndo > UINT16_MAX)
+ {
+ recvwndo = UINT16_MAX;
+ }
Review comment:
doesn't this copy of tcp need scaling?
--
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 #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#issuecomment-874520270
@yamt
I upload a new commit. When the window scale is enabled, the received buffer is easily over 64K. I modified the related data type to uint32_t to avoid the TCP window overflow. Please review again. 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 #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#discussion_r664114022
##########
File path: net/tcp/Kconfig
##########
@@ -106,6 +106,27 @@ config NET_TCP_FAST_RETRANSMIT_WATERMARK
missing segment, without waiting for a retransmission timer to
expire.
+config NET_TCP_WINDOW_SCALE
+ bool "Enable TCP/IP Window Scale Option"
+ default n
+ ---help---
+ RFC1323:
+ 2. TCP WINDOW SCALE OPTION
+ The window scale extension expands the definition of the TCP
+ window to 32 bits and then uses a scale factor to carry this 32-
+ bit value in the 16-bit Window field of the TCP header (SEG.WND in
+ RFC-793).
+
+if NET_TCP_WINDOW_SCALE
+
+config NET_TCP_WINDOW_SCALE_FACTOR
+ int "TCP/IP Window Scale Factor"
+ default 7
Review comment:
This factor only affects the sliding window size of receive and send, which does not help for performance. The window factor is best to be consistent with the iob buffer size，maybe we can calculate the scale from it.
--
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] yamt commented on a change in pull request #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#discussion_r663960060
##########
File path: net/tcp/Kconfig
##########
@@ -106,6 +106,27 @@ config NET_TCP_FAST_RETRANSMIT_WATERMARK
missing segment, without waiting for a retransmission timer to
expire.
+config NET_TCP_WINDOW_SCALE
+ bool "Enable TCP/IP Window Scale Option"
+ default n
+ ---help---
+ RFC1323:
+ 2. TCP WINDOW SCALE OPTION
+ The window scale extension expands the definition of the TCP
+ window to 32 bits and then uses a scale factor to carry this 32-
+ bit value in the 16-bit Window field of the TCP header (SEG.WND in
+ RFC-793).
+
+if NET_TCP_WINDOW_SCALE
+
+config NET_TCP_WINDOW_SCALE_FACTOR
+ int "TCP/IP Window Scale Factor"
+ default 7
Review comment:
why 7?
--
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 #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#issuecomment-875381445
LGTM, @yamt please take a look.
--
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 #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#discussion_r664403991
##########
File path: net/sixlowpan/sixlowpan_tcpsend.c
##########
@@ -258,7 +258,12 @@ static int sixlowpan_tcp_header(FAR struct tcp_conn_s *conn,
/* Update the TCP received window based on I/O buffer availability */
uint32_t rcvseq = tcp_getsequence(conn->rcvseq);
- uint16_t recvwndo = tcp_get_recvwindow(dev, conn);
+ uint32_t recvwndo = tcp_get_recvwindow(dev, conn);
+
+ if (recvwndo > UINT16_MAX)
+ {
+ recvwndo = UINT16_MAX;
+ }
Review comment:
Yes, the current sixlowpan stack does not support optdata:
https://github.com/apache/incubator-nuttx/blob/b901f22c27bb26630a1289605466b04b96707050/net/sixlowpan/sixlowpan_tcpsend.c#L231
--
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 a change in pull request #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#discussion_r664091493
##########
File path: net/tcp/Kconfig
##########
@@ -106,6 +106,27 @@ config NET_TCP_FAST_RETRANSMIT_WATERMARK
missing segment, without waiting for a retransmission timer to
expire.
+config NET_TCP_WINDOW_SCALE
+ bool "Enable TCP/IP Window Scale Option"
+ default n
+ ---help---
+ RFC1323:
+ 2. TCP WINDOW SCALE OPTION
+ The window scale extension expands the definition of the TCP
+ window to 32 bits and then uses a scale factor to carry this 32-
+ bit value in the 16-bit Window field of the TCP header (SEG.WND in
+ RFC-793).
+
+if NET_TCP_WINDOW_SCALE
+
+config NET_TCP_WINDOW_SCALE_FACTOR
+ int "TCP/IP Window Scale Factor"
+ default 7
Review comment:
> I just noticed that most of the network configurations are CONFIG_IOB_BUFSIZE is greater than 128, so the scale factor is set to 7. is it better to set the default value to 0?
@anchao did you make some performance test? Maybe it is better to test it do define a good value.
--
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 #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#discussion_r665026605
##########
File path: net/tcp/tcp.h
##########
@@ -198,6 +209,7 @@ struct tcp_conn_s
#else
uint16_t tx_unacked; /* Number bytes sent but not yet ACKed */
#endif
+ uint16_t flags; /* Flags of TCP-specific options */
Review comment:
The flags filed will be extended by TCP options and reused in the future, such as Selective-ACK, Time-Stamp, etc.
--
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] yamt commented on a change in pull request #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#discussion_r664228213
##########
File path: net/tcp/Kconfig
##########
@@ -106,6 +106,27 @@ config NET_TCP_FAST_RETRANSMIT_WATERMARK
missing segment, without waiting for a retransmission timer to
expire.
+config NET_TCP_WINDOW_SCALE
+ bool "Enable TCP/IP Window Scale Option"
+ default n
+ ---help---
+ RFC1323:
+ 2. TCP WINDOW SCALE OPTION
+ The window scale extension expands the definition of the TCP
+ window to 32 bits and then uses a scale factor to carry this 32-
+ bit value in the 16-bit Window field of the TCP header (SEG.WND in
+ RFC-793).
+
+if NET_TCP_WINDOW_SCALE
+
+config NET_TCP_WINDOW_SCALE_FACTOR
+ int "TCP/IP Window Scale Factor"
+ default 7
Review comment:
> maybe a more reasonable thing is to calculate the minimum value which can represent our max window. (from IOB and RECVBUF configs)
as recv buffer size can change during the lifetime of the connection, maybe just from IOB configurations.
--
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] yamt commented on a change in pull request #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#discussion_r664254702
##########
File path: net/tcp/Kconfig
##########
@@ -106,6 +106,27 @@ config NET_TCP_FAST_RETRANSMIT_WATERMARK
missing segment, without waiting for a retransmission timer to
expire.
+config NET_TCP_WINDOW_SCALE
+ bool "Enable TCP/IP Window Scale Option"
+ default n
+ ---help---
+ RFC1323:
+ 2. TCP WINDOW SCALE OPTION
+ The window scale extension expands the definition of the TCP
+ window to 32 bits and then uses a scale factor to carry this 32-
+ bit value in the 16-bit Window field of the TCP header (SEG.WND in
+ RFC-793).
+
+if NET_TCP_WINDOW_SCALE
+
+config NET_TCP_WINDOW_SCALE_FACTOR
+ int "TCP/IP Window Scale Factor"
+ default 7
Review comment:
ok.
then i will merge this later today unless anyone beats me or raises an objection.
(i don't like to merge a complex patch within 24 hours)
--
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 #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#discussion_r664251381
##########
File path: net/tcp/Kconfig
##########
@@ -106,6 +106,27 @@ config NET_TCP_FAST_RETRANSMIT_WATERMARK
missing segment, without waiting for a retransmission timer to
expire.
+config NET_TCP_WINDOW_SCALE
+ bool "Enable TCP/IP Window Scale Option"
+ default n
+ ---help---
+ RFC1323:
+ 2. TCP WINDOW SCALE OPTION
+ The window scale extension expands the definition of the TCP
+ window to 32 bits and then uses a scale factor to carry this 32-
+ bit value in the 16-bit Window field of the TCP header (SEG.WND in
+ RFC-793).
+
+if NET_TCP_WINDOW_SCALE
+
+config NET_TCP_WINDOW_SCALE_FACTOR
+ int "TCP/IP Window Scale Factor"
+ default 7
Review comment:
Let us do this optimization in the next PR, I think maybe you have already thought of a better solution
--
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] yamt commented on a change in pull request #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#discussion_r664377240
##########
File path: net/tcp/tcp_send.c
##########
@@ -372,6 +384,13 @@ static void tcp_sendcommon(FAR struct net_driver_s *dev,
/* Update the Receiver Window */
+#ifdef CONFIG_NET_TCP_WINDOW_SCALE
+ if (recvwndo > 0 && conn->rcv_scale > 0)
+ {
+ recvwndo <<= conn->rcv_scale;
+ }
+#endif
+
Review comment:
adjusting the window for UINT16_MAX and scaling here can confuse tcp_should_send_recvwindow.
i feel it's simpler to do it in tcp_get_recvwindow.
how do you think?
##########
File path: net/sixlowpan/sixlowpan_tcpsend.c
##########
@@ -258,7 +258,12 @@ static int sixlowpan_tcp_header(FAR struct tcp_conn_s *conn,
/* Update the TCP received window based on I/O buffer availability */
uint32_t rcvseq = tcp_getsequence(conn->rcvseq);
- uint16_t recvwndo = tcp_get_recvwindow(dev, conn);
+ uint32_t recvwndo = tcp_get_recvwindow(dev, conn);
+
+ if (recvwndo > UINT16_MAX)
+ {
+ recvwndo = UINT16_MAX;
+ }
Review comment:
doesn't this copy of tcp need scaling?
--
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] yamt commented on a change in pull request #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#discussion_r664918293
##########
File path: net/tcp/tcp_recvwindow.c
##########
@@ -224,14 +203,49 @@ uint16_t tcp_get_recvwindow(FAR struct net_driver_s *dev,
return tcp_calc_rcvsize(conn, recvwndo);
}
+/****************************************************************************
+ * Name: tcp_get_scalewindow
Review comment:
i'm not sure if i like this name.
how about tcp_get_unscaled_recvwindow?
--
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 a change in pull request #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#discussion_r664091493
##########
File path: net/tcp/Kconfig
##########
@@ -106,6 +106,27 @@ config NET_TCP_FAST_RETRANSMIT_WATERMARK
missing segment, without waiting for a retransmission timer to
expire.
+config NET_TCP_WINDOW_SCALE
+ bool "Enable TCP/IP Window Scale Option"
+ default n
+ ---help---
+ RFC1323:
+ 2. TCP WINDOW SCALE OPTION
+ The window scale extension expands the definition of the TCP
+ window to 32 bits and then uses a scale factor to carry this 32-
+ bit value in the 16-bit Window field of the TCP header (SEG.WND in
+ RFC-793).
+
+if NET_TCP_WINDOW_SCALE
+
+config NET_TCP_WINDOW_SCALE_FACTOR
+ int "TCP/IP Window Scale Factor"
+ default 7
Review comment:
> I just noticed that most of the network configurations are CONFIG_IOB_BUFSIZE is greater than 128, so the scale factor is set to 7. is it better to set the default value to 0?
@anchao did you make some performance test? Maybe it is better to test it do define a good value.
--
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] yamt commented on a change in pull request #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#discussion_r663806067
##########
File path: net/tcp/tcp_send.c
##########
@@ -365,6 +365,13 @@ static void tcp_sendcommon(FAR struct net_driver_s *dev,
uint32_t rcvseq = tcp_getsequence(conn->rcvseq);
uint16_t recvwndo = tcp_get_recvwindow(dev, conn);
+#ifdef CONFIG_NET_TCP_WINDOW_SCALE
+ if (recvwndo > 0 && conn->rcv_scale > 0)
+ {
+ recvwndo >>= conn->rcv_scale;
+ }
+#endif
Review comment:
rcv_adv update below should use the unscaled value
##########
File path: net/tcp/tcp_send.c
##########
@@ -669,11 +677,24 @@ void tcp_synack(FAR struct net_driver_s *dev, FAR struct tcp_conn_s *conn,
/* We send out the TCP Maximum Segment Size option with our ACK. */
- tcp->optdata[0] = TCP_OPT_MSS;
- tcp->optdata[1] = TCP_OPT_MSS_LEN;
- tcp->optdata[2] = tcp_mss >> 8;
- tcp->optdata[3] = tcp_mss & 0xff;
- tcp->tcpoffset = ((TCP_HDRLEN + TCP_OPT_MSS_LEN) / 4) << 4;
+ tcp->optdata[optlen++] = TCP_OPT_MSS;
+ tcp->optdata[optlen++] = TCP_OPT_MSS_LEN;
+ tcp->optdata[optlen++] = tcp_mss >> 8;
+ tcp->optdata[optlen++] = tcp_mss & 0xff;
+
+#ifdef CONFIG_NET_TCP_WINDOW_SCALE
+ if (tcp->flags == TCP_SYN ||
+ ((tcp->flags == (TCP_ACK | TCP_SYN)) && conn->snd_scale))
Review comment:
0 is a valid value for the window scaling option.
i guess you need a separate bit to record "window scaling option received".
--
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 #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#discussion_r663992243
##########
File path: net/tcp/Kconfig
##########
@@ -106,6 +106,27 @@ config NET_TCP_FAST_RETRANSMIT_WATERMARK
missing segment, without waiting for a retransmission timer to
expire.
+config NET_TCP_WINDOW_SCALE
+ bool "Enable TCP/IP Window Scale Option"
+ default n
+ ---help---
+ RFC1323:
+ 2. TCP WINDOW SCALE OPTION
+ The window scale extension expands the definition of the TCP
+ window to 32 bits and then uses a scale factor to carry this 32-
+ bit value in the 16-bit Window field of the TCP header (SEG.WND in
+ RFC-793).
+
+if NET_TCP_WINDOW_SCALE
+
+config NET_TCP_WINDOW_SCALE_FACTOR
+ int "TCP/IP Window Scale Factor"
+ default 7
Review comment:
I just noticed that most of the network configurations are CONFIG_IOB_BUFSIZE is greater than 128, so the scale factor is set to 7. is it better to set the default value to 0?
--
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 #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#discussion_r663992243
##########
File path: net/tcp/Kconfig
##########
@@ -106,6 +106,27 @@ config NET_TCP_FAST_RETRANSMIT_WATERMARK
missing segment, without waiting for a retransmission timer to
expire.
+config NET_TCP_WINDOW_SCALE
+ bool "Enable TCP/IP Window Scale Option"
+ default n
+ ---help---
+ RFC1323:
+ 2. TCP WINDOW SCALE OPTION
+ The window scale extension expands the definition of the TCP
+ window to 32 bits and then uses a scale factor to carry this 32-
+ bit value in the 16-bit Window field of the TCP header (SEG.WND in
+ RFC-793).
+
+if NET_TCP_WINDOW_SCALE
+
+config NET_TCP_WINDOW_SCALE_FACTOR
+ int "TCP/IP Window Scale Factor"
+ default 7
Review comment:
I just noticed that most of the network configurations are CONFIG_IOB_BUFSIZE is greater than 128, so the scale factor is set to 7. is it better to set the default value to 0?
##########
File path: net/tcp/Kconfig
##########
@@ -106,6 +106,27 @@ config NET_TCP_FAST_RETRANSMIT_WATERMARK
missing segment, without waiting for a retransmission timer to
expire.
+config NET_TCP_WINDOW_SCALE
+ bool "Enable TCP/IP Window Scale Option"
+ default n
+ ---help---
+ RFC1323:
+ 2. TCP WINDOW SCALE OPTION
+ The window scale extension expands the definition of the TCP
+ window to 32 bits and then uses a scale factor to carry this 32-
+ bit value in the 16-bit Window field of the TCP header (SEG.WND in
+ RFC-793).
+
+if NET_TCP_WINDOW_SCALE
+
+config NET_TCP_WINDOW_SCALE_FACTOR
+ int "TCP/IP Window Scale Factor"
+ default 7
Review comment:
This factor only affects the sliding window size of receive and send, which does not help for performance. The window factor is best to be consistent with the iob buffer size，maybe we can calculate the scale from it.
##########
File path: net/tcp/Kconfig
##########
@@ -106,6 +106,27 @@ config NET_TCP_FAST_RETRANSMIT_WATERMARK
missing segment, without waiting for a retransmission timer to
expire.
+config NET_TCP_WINDOW_SCALE
+ bool "Enable TCP/IP Window Scale Option"
+ default n
+ ---help---
+ RFC1323:
+ 2. TCP WINDOW SCALE OPTION
+ The window scale extension expands the definition of the TCP
+ window to 32 bits and then uses a scale factor to carry this 32-
+ bit value in the 16-bit Window field of the TCP header (SEG.WND in
+ RFC-793).
+
+if NET_TCP_WINDOW_SCALE
+
+config NET_TCP_WINDOW_SCALE_FACTOR
+ int "TCP/IP Window Scale Factor"
+ default 7
Review comment:
Let us do this optimization in the next PR, I think maybe you have already thought of a better solution
##########
File path: net/sixlowpan/sixlowpan_tcpsend.c
##########
@@ -258,7 +258,12 @@ static int sixlowpan_tcp_header(FAR struct tcp_conn_s *conn,
/* Update the TCP received window based on I/O buffer availability */
uint32_t rcvseq = tcp_getsequence(conn->rcvseq);
- uint16_t recvwndo = tcp_get_recvwindow(dev, conn);
+ uint32_t recvwndo = tcp_get_recvwindow(dev, conn);
+
+ if (recvwndo > UINT16_MAX)
+ {
+ recvwndo = UINT16_MAX;
+ }
Review comment:
Yes, the current sixlowpan stack does not support optdata:
https://github.com/apache/incubator-nuttx/blob/b901f22c27bb26630a1289605466b04b96707050/net/sixlowpan/sixlowpan_tcpsend.c#L231
--
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 #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070
--
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 #4070: net/tcp: add window scale support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4070:
URL: https://github.com/apache/incubator-nuttx/pull/4070#discussion_r665028707
##########
File path: net/tcp/tcp.h
##########
@@ -198,6 +209,7 @@ struct tcp_conn_s
#else
uint16_t tx_unacked; /* Number bytes sent but not yet ACKed */
#endif
+ uint16_t flags; /* Flags of TCP-specific options */
Review comment:
If so, let's merge keepalive here too
--
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