a-lunev commented on a change in pull request #4659:
URL: https://github.com/apache/incubator-nuttx/pull/4659#discussion_r727627443
##########
File path: net/tcp/tcp_send_unbuffered.c
##########
@@ -243,18 +243,54 @@ static uint16_t tcpsend_eventhandler(FAR struct
net_driver_s *dev,
else if ((flags & TCP_REXMIT) != 0)
{
+#if defined(CONFIG_NET_TCP_SPLIT)
/* Yes.. in this case, reset the number of bytes that have been sent
* to the number of bytes that have been ACKed.
*/
pstate->snd_sent = pstate->snd_acked;
Review comment:
Yes, it does. I wrote about this in the PR summary. It contradicts to
RFC 6298 (5.4) and other TCP related RFCs. Possibly it was done intentionally
as a workaround (as I understand, CONFIG_NET_TCP_SPLIT algorithm is some sort
of a trick as it is described in the source code comments).
If CONFIG_NET_TCP_SPLIT was planned to be removed, the removing would
simplify this part of the code.
##########
File path: net/tcp/tcp_send_unbuffered.c
##########
@@ -243,18 +243,54 @@ static uint16_t tcpsend_eventhandler(FAR struct
net_driver_s *dev,
else if ((flags & TCP_REXMIT) != 0)
{
+#if defined(CONFIG_NET_TCP_SPLIT)
/* Yes.. in this case, reset the number of bytes that have been sent
* to the number of bytes that have been ACKed.
*/
pstate->snd_sent = pstate->snd_acked;
Review comment:
I understand that part of code as follows:
Here is an example:
```
pstate->snd_sent = 0, pstate->snd_acked = 0: SND.seq = 100 (it's ISN), len =
10 -> receiver successfully gets the segment by ACKing with RCV.ack = 110.
pstate->snd_sent = 10, pstate->snd_acked = 10: SND.seq = 110, len = 10 ->
the segment is lost somewhere in the network
pstate->snd_sent = 20, pstate->snd_acked = 10: SND.seq = 120, len = 10 ->
receiver gets the segment, however still ACKing with RCV.ack = 110.
pstate->snd_sent = 30, pstate->snd_acked = 10: SND.seq = 130, len = 10 ->
receiver gets the segment, however still ACKing with RCV.ack = 110.
pstate->snd_sent = 40, pstate->snd_acked = 10: SND.seq = 140, len = 10 ->
receiver gets the segment, however still ACKing with RCV.ack = 110.
```
Due to timeout NuttX TCP sender sets pstate->snd_sent variable back to 10 as
follows:
```
pstate->snd_sent = 10, pstate->snd_acked = 10: SND.seq = 110, len = 10 ->
receiver successfully gets the segment by ACKing with RCV.ack = 120.
pstate->snd_sent = 20, pstate->snd_acked = 20: SND.seq = 120, len = 10 ->
receiver successfully gets the segment by ACKing with RCV.ack = 130.
pstate->snd_sent = 30, pstate->snd_acked = 30: SND.seq = 130, len = 10 ->
receiver successfully gets the segment by ACKing with RCV.ack = 140.
pstate->snd_sent = 40, pstate->snd_acked = 40: SND.seq = 140, len = 10 ->
receiver successfully gets the segment by ACKing with RCV.ack = 150.
```
Thus in this example it re-sends all 4 segments instead of only one segment
with SND.seq = 110.
##########
File path: net/tcp/tcp_send_unbuffered.c
##########
@@ -243,18 +243,54 @@ static uint16_t tcpsend_eventhandler(FAR struct
net_driver_s *dev,
else if ((flags & TCP_REXMIT) != 0)
{
+#if defined(CONFIG_NET_TCP_SPLIT)
/* Yes.. in this case, reset the number of bytes that have been sent
* to the number of bytes that have been ACKed.
*/
pstate->snd_sent = pstate->snd_acked;
Review comment:
I understand that part of code as follows:
Here is an example:
```
pstate->snd_sent = 0, pstate->snd_acked = 0: SND.seq = 100 (it's ISN), len =
10 -> receiver successfully gets the segment by ACKing with RCV.ack = 110.
pstate->snd_sent = 10, pstate->snd_acked = 10: SND.seq = 110, len = 10 ->
the segment is lost somewhere in the network
pstate->snd_sent = 20, pstate->snd_acked = 10: SND.seq = 120, len = 10 ->
receiver gets the segment, however still ACKing with RCV.ack = 110.
pstate->snd_sent = 30, pstate->snd_acked = 10: SND.seq = 130, len = 10 ->
receiver gets the segment, however still ACKing with RCV.ack = 110.
pstate->snd_sent = 40, pstate->snd_acked = 10: SND.seq = 140, len = 10 ->
receiver gets the segment, however still ACKing with RCV.ack = 110.
```
Due to timeout NuttX TCP sender sets pstate->snd_sent variable back to 10
(and SND.seq is set back to 110) as follows:
```
pstate->snd_sent = 10, pstate->snd_acked = 10: SND.seq = 110, len = 10 ->
receiver successfully gets the segment by ACKing with RCV.ack = 120.
pstate->snd_sent = 20, pstate->snd_acked = 20: SND.seq = 120, len = 10 ->
receiver successfully gets the segment by ACKing with RCV.ack = 130.
pstate->snd_sent = 30, pstate->snd_acked = 30: SND.seq = 130, len = 10 ->
receiver successfully gets the segment by ACKing with RCV.ack = 140.
pstate->snd_sent = 40, pstate->snd_acked = 40: SND.seq = 140, len = 10 ->
receiver successfully gets the segment by ACKing with RCV.ack = 150.
```
Thus in this example it re-sends all 4 segments instead of only one segment
with SND.seq = 110.
##########
File path: net/tcp/tcp_send_unbuffered.c
##########
@@ -243,18 +243,54 @@ static uint16_t tcpsend_eventhandler(FAR struct
net_driver_s *dev,
else if ((flags & TCP_REXMIT) != 0)
{
+#if defined(CONFIG_NET_TCP_SPLIT)
/* Yes.. in this case, reset the number of bytes that have been sent
* to the number of bytes that have been ACKed.
*/
pstate->snd_sent = pstate->snd_acked;
Review comment:
Just now I have updated the description:
from "... to the earliest one"
to "... to the earliest not acknowledged one".
##########
File path: net/tcp/tcp.h
##########
@@ -172,6 +172,9 @@ struct tcp_conn_s
uint8_t rcvseq[4]; /* The sequence number that we expect to
* receive next */
uint8_t sndseq[4]; /* The sequence number that was last sent by us */
+#if !defined(CONFIG_NET_TCP_SPLIT) && !defined(CONFIG_NET_TCP_WRITE_BUFFERS)
+ uint32_t rexmit_seq; /* The sequence number to be retrasmitted */
Review comment:
Previously I read your description in #4293 ("in some cases, we send tcp
segments with sequence numbers back and forth.
especially when sending an empty segment. (eg. pure ack, window updates)").
Thus I have added conn->rexmit_seq field that is used only for retrasmitting
a single TCP segment w/o a need to modify conn->sndseq each time and recovering
it back.
Thus I consider using conn->rexmit_seq as more reliable and stable way
rather than using conn->sndseq for retransmissions.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
