Date: Tue, 8 Mar 2016 10:29:32 +0900 From: Yongmin Cho <yongmincho82@gmail.com> To: hiren panchasara <hiren@strugglingcoder.info> Cc: transport@freebsd.org Subject: Re: In TCP recovery state, problem of computing the pipe(amount of data in flight). Message-ID: <20160308012930.GA24199@yongmincho-All-Series> In-Reply-To: <20160229175453.GA82027@strugglingcoder.info> References: <20160225112625.GA5680@yongmincho-All-Series> <20160227031604.GP31665@strugglingcoder.info> <20160229084045.GA21079@yongmincho-All-Series> <20160229175453.GA82027@strugglingcoder.info>
next in thread | previous in thread | raw e-mail | index | archive | help
--oyUTqETQ0mS9luUI
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
On Mon, Feb 29, 2016 at 09:54:53AM -0800, hiren panchasara wrote:
> On 02/29/16 at 05:40P, Yongmin Cho wrote:
> > Thanks very much for the quick reply.
> >
> > I'm sorry, I didn't make patch file.
> > First, I wanted to discuss my opinion is right.
> >
> > Let me give you example, please check this.
> >
> > <- ACK (cumack = 1, sack[3-4], sack[6-7], sack[9-10])
> > * here segment/byte 2, 5, and 8 are missing.
> > <- ACK (cumack = 1, sack[12-13], sack[15-16], sack[18-19])
> > * here segment/bytes 11, 14 and 17 are also reported missing.
> > <- ACK (cumack = 1, sack[18-19], sack[21-24], sack[27, 28])
> > * here segment/bytes 20 and 25, 26 are missing. (triggered fast
> > retransmission)
> > <- ACK.....(cumack = 1, sack[21-24], sack[27, 28], sack[32, 33])
> > <- ACK.....(cumack = 1, sack[34-35], sack[37, 40], sack[42, 44])
> > <- ACK.....(cumack = 1, sack[34-35], sack[37, 40], sack[42, 45])
> > <- ACKs.....(many duplication acks, and new sacked blocks)
> >
> > In the fast recovery phase, the pipe is caculated like below, If the
> > net.inet.tcp.rfc6675_pipe is turned on.
> > pipe = snd_max - snd_una + sack_bytes_rexmit(1 MSS size) -
> > sacked_bytes(10 = 34-35, 37-40, 42-45 tcp_sack.c:390)
> > One segment is sended(sack_bytes_rexmit), when triggered fast
> > retransmission.
> > Because the snd_cwnd was set 1 mss size. (tcp_input.c:2609)
> > In the fast recovery phase, The sender can send data,
> > If this condition is right(awnd < tp->snd_ssthresh tcp_input.c:2568).
> > When in the network, It still has many in flight packets,
> > snd_max and snd_una will not be changed, and sack_bytes_rexmit is one MSS
> > size, and sacked_bytes is caculated by last ACK that has three SACK
> > blocks(34-35, 37-40, 42-45).
> > So, sometimes(In my test environment) the awnd(pipe) value can't go
> > down less than the snd_ssthresh, while receiving each ACKs
> > in fast recovery phase.
> > You know, If the awnd value can't go down less than the snd_ssthresh,
> > The sender can't send data that is included snd_holes.
> >
> > So, I think, the sacked_bytes should be caculated by all of sacked
> > blocks that is greater than snd_una, like below.
> > pipe = snd_max - snd_una + sack_bytes_rexmit - sacked_bytes(3-4, 6-7,
> > 9-10, 12-13, 15-16, 18-19, 21-24, 27-28, ...).
> >
> > But on current implementation, the sacked_bytes is caculated by three(or four)
> > sacked blocks that is in last ACK, like below.
> > pipe = snd_max - snd_una + sack_bytes_rexmit - sacked_bytes(34-35,
> > 37-40, 42-45 -> sacked blocks of last ACK).
> >
> > My opinion may not be right. Just I want to check implementation of
> > computing pipe.
>
> Your opinion seems correct to me. If you can create a patch based on
> this, that'd be great. As I cannot spend time on this until next week.
>
> Cheers,
> Hiren
I've created a patch based on this issue.
You know, The sack_blocks array in tcp_sack_doack function(tcp_sack.c)
have only 3 or 4 sacked blocks.
But We need to know all the data that has been sacked.
The snd_holes queue(in tcp_cb structure) is updated every time we get
an ACK with sack blocks.
So, the snd_holes queue know all of hole information.
And, We can get the sacked_bytes from the snd_holes.
So, I calculated the sacked_bytes every time when updating the
snd_holes.
I've tested this patch in my test environment, And I think, this
implementation is working well.
Please check this patch. any feedback will be welcome.
Thanks.
--oyUTqETQ0mS9luUI
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="tcp_sack.diff"
Index: sys/netinet/tcp_sack.c
===================================================================
--- sys/netinet/tcp_sack.c (revision 296480)
+++ sys/netinet/tcp_sack.c (working copy)
@@ -355,12 +355,13 @@ tcp_sack_doack(struct tcpcb *tp, struct tcpopt *to
{
struct sackhole *cur, *temp;
struct sackblk sack, sack_blocks[TCP_MAX_SACK + 1], *sblkp;
- int i, j, num_sack_blks, sack_changed;
+ int i, j, num_sack_blks, sack_changed, sacked_bytes;
INP_WLOCK_ASSERT(tp->t_inpcb);
num_sack_blks = 0;
sack_changed = 0;
+ sacked_bytes = 0;
/*
* If SND.UNA will be advanced by SEG.ACK, and if SACK holes exist,
* treat [SND.UNA, SEG.ACK) as if it is a SACK block.
@@ -374,7 +375,6 @@ tcp_sack_doack(struct tcpcb *tp, struct tcpopt *to
* received new blocks from the other side.
*/
if (to->to_flags & TOF_SACK) {
- tp->sackhint.sacked_bytes = 0; /* reset */
for (i = 0; i < to->to_nsacks; i++) {
bcopy((to->to_sacks + i * TCPOLEN_SACK),
&sack, sizeof(sack));
@@ -387,8 +387,6 @@ tcp_sack_doack(struct tcpcb *tp, struct tcpopt *to
SEQ_GT(sack.end, tp->snd_una) &&
SEQ_LEQ(sack.end, tp->snd_max)) {
sack_blocks[num_sack_blks++] = sack;
- tp->sackhint.sacked_bytes +=
- (sack.end-sack.start);
}
}
}
@@ -446,6 +444,7 @@ tcp_sack_doack(struct tcpcb *tp, struct tcpopt *to
temp = tcp_sackhole_insert(tp, tp->snd_fack,sblkp->start,NULL);
if (temp != NULL) {
tp->snd_fack = sblkp->end;
+ sacked_bytes += (sblkp->end - sblkp->start);
/* Go to the previous sack block. */
sblkp--;
sack_changed = 1;
@@ -462,11 +461,14 @@ tcp_sack_doack(struct tcpcb *tp, struct tcpopt *to
SEQ_LT(tp->snd_fack, sblkp->start))
sblkp--;
if (sblkp >= sack_blocks &&
- SEQ_LT(tp->snd_fack, sblkp->end))
+ SEQ_LT(tp->snd_fack, sblkp->end)) {
+ sacked_bytes += (sblkp->end - tp->snd_fack);
tp->snd_fack = sblkp->end;
+ }
}
} else if (SEQ_LT(tp->snd_fack, sblkp->end)) {
/* fack is advanced. */
+ sacked_bytes += (sblkp->end - tp->snd_fack);
tp->snd_fack = sblkp->end;
sack_changed = 1;
}
@@ -503,6 +505,10 @@ tcp_sack_doack(struct tcpcb *tp, struct tcpopt *to
/* Data acks at least the beginning of hole. */
if (SEQ_GEQ(sblkp->end, cur->end)) {
/* Acks entire hole, so delete hole. */
+ if (SEQ_GT(cur->start, th_ack))
+ sacked_bytes += (cur->end - cur->start);
+ else
+ sacked_bytes -= (sblkp->end - cur->end);
temp = cur;
cur = TAILQ_PREV(cur, sackhole_head, scblink);
tcp_sackhole_remove(tp, temp);
@@ -514,6 +520,8 @@ tcp_sack_doack(struct tcpcb *tp, struct tcpopt *to
continue;
} else {
/* Move start of hole forward. */
+ if (SEQ_GT(sblkp->start, th_ack))
+ sacked_bytes += (sblkp->end - cur->start);
cur->start = sblkp->end;
cur->rxmit = SEQ_MAX(cur->rxmit, cur->start);
}
@@ -521,6 +529,7 @@ tcp_sack_doack(struct tcpcb *tp, struct tcpopt *to
/* Data acks at least the end of hole. */
if (SEQ_GEQ(sblkp->end, cur->end)) {
/* Move end of hole backward. */
+ sacked_bytes += (cur->end - sblkp->start);
cur->end = sblkp->start;
cur->rxmit = SEQ_MIN(cur->rxmit, cur->end);
} else {
@@ -528,6 +537,7 @@ tcp_sack_doack(struct tcpcb *tp, struct tcpopt *to
* ACKs some data in middle of a hole; need
* to split current hole
*/
+ sacked_bytes += (sblkp->end - sblkp->start);
temp = tcp_sackhole_insert(tp, sblkp->end,
cur->end, cur);
if (temp != NULL) {
@@ -554,6 +564,7 @@ tcp_sack_doack(struct tcpcb *tp, struct tcpopt *to
else
sblkp--;
}
+ tp->sackhint.sacked_bytes += sacked_bytes;
return (sack_changed);
}
--oyUTqETQ0mS9luUI--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160308012930.GA24199>