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 2020/12/07 06:54:02 UTC
[GitHub] [incubator-nuttx] anchao opened a new pull request #2489: [Performance]net/tcp: send the ACK in time after obtain ahead buffer from iobs
anchao opened a new pull request #2489:
URL: https://github.com/apache/incubator-nuttx/pull/2489
## Summary
1. [Performance]net/tcp: send the ACK in time after obtain ahead buffer from iobs
Request the TCP ACK to estimate the receive window after handle
any data already buffered in a read-ahead buffer.
2. net/tcp: add a member to record the current receiving window
3. net/tcp: rename the winszie to snd_wnd to make the semantics more accurate
## Impact
TCP Performance
## 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #2489: [Performance]net/tcp: send the ACK in time after obtain ahead buffer from iobs
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2489:
URL: https://github.com/apache/incubator-nuttx/pull/2489#discussion_r537499841
##########
File path: net/tcp/tcp_recvfrom.c
##########
@@ -636,14 +683,18 @@ ssize_t psock_tcp_recvfrom(FAR struct socket *psock, FAR void *buf,
size_t len, int flags, FAR struct sockaddr *from,
FAR socklen_t *fromlen)
{
+ FAR struct tcp_conn_s *conn;
struct tcp_recvfrom_s state;
int ret;
Review comment:
Align consistently or remove alignment.
##########
File path: net/tcp/tcp_recvfrom.c
##########
@@ -510,6 +510,53 @@ static uint16_t tcp_eventhandler(FAR struct net_driver_s *dev,
return flags;
}
+/****************************************************************************
+ * Name: tcp_ackhandler
+ *
+ * Description:
+ * This function is called with the network locked to send the ACK in
+ * response by the lower, device interfacing layer.
+ *
+ * Input Parameters:
+ * dev The structure of the network driver that generated the event.
+ * pvconn The connection structure associated with the socket
+ * flags Set of events describing why the callback was invoked
+ *
+ * Returned Value:
+ * ACK should be send in the response.
+ *
+ * Assumptions:
+ * The network is locked.
+ *
+ ****************************************************************************/
+
+static uint16_t tcp_ackhandler(FAR struct net_driver_s *dev,
+ FAR void *pvconn, FAR void *pvpriv,
+ uint16_t flags)
Review comment:
Align formal parameters
##########
File path: net/tcp/tcp_recvfrom.c
##########
@@ -384,7 +384,7 @@ static inline void tcp_sender(FAR struct net_driver_s *dev,
*
****************************************************************************/
-static uint16_t tcp_eventhandler(FAR struct net_driver_s *dev,
+static uint16_t tcp_recvhandler(FAR struct net_driver_s *dev,
FAR void *pvconn, FAR void *pvpriv,
Review comment:
Align formal parameters.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao edited a comment on pull request #2489: [Performance]net/tcp: send the ACK in time after obtain ahead buffer from iobs
Posted by GitBox <gi...@apache.org>.
anchao edited a comment on pull request #2489:
URL: https://github.com/apache/incubator-nuttx/pull/2489#issuecomment-742164923
> @anchao
>
> Let me confirm this PR.
>
> Does this PR relate to TCP ZeroWindow report and TCP Window update
> after IOB is available on NuttX?
Hi @masayuki2009 san,
Thanks for your review!
Yes, in the previous commit, , I removed the slide of the default acceptance window to avoid excessive tcp retransmissions if the iob is unavailable,
https://github.com/apache/incubator-nuttx/blob/375211f5a176ce5a3cdaa36ed8a5cbddddf1fedf/net/tcp/tcp_recvwindow.c#L159-L167
Unfortunately, if too much tcp data is received without waiting in recv() (e.g HTTP GET),
then the stack will unable to reply ACK normally in the subsequent reception,
The window will remain at 0 and the transfer cannot be continued ...
This PR solves a similar issue, If the recv window slide in the previous ACK is 0,
send an ACK to update the current window slide after recv().
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #2489: [Performance]net/tcp: send the ACK in time after obtain ahead buffer from iobs
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #2489:
URL: https://github.com/apache/incubator-nuttx/pull/2489#issuecomment-742129113
@anchao
Let me confirm this PR.
Does this PR relate to TCP Zero window report and TCP Window update
after IOB is available on NuttX?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #2489: [Performance]net/tcp: send the ACK in time after obtain ahead buffer from iobs
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #2489:
URL: https://github.com/apache/incubator-nuttx/pull/2489#discussion_r537529831
##########
File path: net/tcp/tcp_recvfrom.c
##########
@@ -636,14 +683,18 @@ ssize_t psock_tcp_recvfrom(FAR struct socket *psock, FAR void *buf,
size_t len, int flags, FAR struct sockaddr *from,
FAR socklen_t *fromlen)
{
+ FAR struct tcp_conn_s *conn;
struct tcp_recvfrom_s state;
int ret;
Review comment:
Done
##########
File path: net/tcp/tcp_recvfrom.c
##########
@@ -510,6 +510,53 @@ static uint16_t tcp_eventhandler(FAR struct net_driver_s *dev,
return flags;
}
+/****************************************************************************
+ * Name: tcp_ackhandler
+ *
+ * Description:
+ * This function is called with the network locked to send the ACK in
+ * response by the lower, device interfacing layer.
+ *
+ * Input Parameters:
+ * dev The structure of the network driver that generated the event.
+ * pvconn The connection structure associated with the socket
+ * flags Set of events describing why the callback was invoked
+ *
+ * Returned Value:
+ * ACK should be send in the response.
+ *
+ * Assumptions:
+ * The network is locked.
+ *
+ ****************************************************************************/
+
+static uint16_t tcp_ackhandler(FAR struct net_driver_s *dev,
+ FAR void *pvconn, FAR void *pvpriv,
+ uint16_t flags)
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #2489: [Performance]net/tcp: send the ACK in time after obtain ahead buffer from iobs
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #2489:
URL: https://github.com/apache/incubator-nuttx/pull/2489#issuecomment-742211843
@anchao
>Thanks for your confirmation, these changes cannot solve the TCP retransmission perfectly,
>which can only reduce it as much as possible.
Thanks for the updates.
I will merge 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 merged pull request #2489: [Performance]net/tcp: send the ACK in time after obtain ahead buffer from iobs
Posted by GitBox <gi...@apache.org>.
masayuki2009 merged pull request #2489:
URL: https://github.com/apache/incubator-nuttx/pull/2489
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 edited a comment on pull request #2489: [Performance]net/tcp: send the ACK in time after obtain ahead buffer from iobs
Posted by GitBox <gi...@apache.org>.
masayuki2009 edited a comment on pull request #2489:
URL: https://github.com/apache/incubator-nuttx/pull/2489#issuecomment-742129113
@anchao
Let me confirm this PR.
Does this PR relate to TCP ZeroWindow report and TCP Window update
after IOB is available on NuttX?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #2489: [Performance]net/tcp: send the ACK in time after obtain ahead buffer from iobs
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #2489:
URL: https://github.com/apache/incubator-nuttx/pull/2489#issuecomment-742191998
Hi @anchao,
>Yes, in the previous commit, I removed the slide of the default acceptance window to avoid excessive tcp retransmissions if the iob is unavailable,
I confirmed that this PR reduces TCP retransmission while playback an HTTP audio stream with spresense:rndis.
However, TCP retransmission still happens. I suspect it depends on the network environment.
How about your test results?
Does this PR resolve TCP retransmission issues with your environment?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #2489: [Performance]net/tcp: send the ACK in time after obtain ahead buffer from iobs
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #2489:
URL: https://github.com/apache/incubator-nuttx/pull/2489#discussion_r537529677
##########
File path: net/tcp/tcp_recvfrom.c
##########
@@ -384,7 +384,7 @@ static inline void tcp_sender(FAR struct net_driver_s *dev,
*
****************************************************************************/
-static uint16_t tcp_eventhandler(FAR struct net_driver_s *dev,
+static uint16_t tcp_recvhandler(FAR struct net_driver_s *dev,
FAR void *pvconn, FAR void *pvpriv,
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on pull request #2489: [Performance]net/tcp: send the ACK in time after obtain ahead buffer from iobs
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #2489:
URL: https://github.com/apache/incubator-nuttx/pull/2489#issuecomment-742164923
> @anchao
>
> Let me confirm this PR.
>
> Does this PR relate to TCP ZeroWindow report and TCP Window update
> after IOB is available on NuttX?
Thanks for your review!
Yes, in the previous commit, , I removed the slide of the default acceptance window to avoid excessive tcp retransmissions if the iob is unavailable,
https://github.com/apache/incubator-nuttx/blob/375211f5a176ce5a3cdaa36ed8a5cbddddf1fedf/net/tcp/tcp_recvwindow.c#L159-L167
Unfortunately, if too much tcp data is received without waiting in recv() (e.g HTTP GET),
then the stack will unable to reply ACK normally in the subsequent reception,
The window will remain at 0 and the transfer cannot be continued ...
This PR solves a similar issue, If the recv window slide in the previous ACK is 0,
send an ACK to update the current window slide after recv().
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on pull request #2489: [Performance]net/tcp: send the ACK in time after obtain ahead buffer from iobs
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #2489:
URL: https://github.com/apache/incubator-nuttx/pull/2489#issuecomment-742194853
Hi @masayuki2009 san,
Thanks for your confirmation, these changes cannot solve the TCP retransmission perfectly,
which can only reduce it as much as possible.
We found that some issues are still being optimized and it may take some times.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org