| Message ID | 20200627040535.858564-2-ysseung@google.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | tcp: improve delivered counts in SCM_TSTAMP_ACK | expand |
On Sat, Jun 27, 2020 at 12:06 AM Yousuk Seung <ysseung@google.com> wrote: > > Currently tp->delivered is updated with sacked packets but not > cumulatively acked when SCP_TSTAMP_ACK is timestamped. This patch moves > a tcp_ack_tstamp() call in tcp_clean_rtx_queue() to later in the loop so > that when a skb is fully acked OPT_STATS of SCM_TSTAMP_ACK will include > the current skb in the delivered count. When not fully acked > tcp_ack_tstamp() is a no-op and there is no change in behavior. > > Signed-off-by: Yousuk Seung <ysseung@google.com> > Signed-off-by: Yuchung Cheng <ycheng@google.com> > Acked-by: Eric Dumazet <edumazet@google.com> > Acked-by: Neal Cardwell <ncardwell@google.com> > Acked-by: Soheil Hassas Yeganeh <soheil@google.com> > --- > net/ipv4/tcp_input.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index f3a0eb139b76..2a683e785cca 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -3078,8 +3078,6 @@ static int tcp_clean_rtx_queue(struct sock *sk, u32 prior_fack, > u8 sacked = scb->sacked; > u32 acked_pcount; > > - tcp_ack_tstamp(sk, skb, prior_snd_una); > - > /* Determine how many packets and what bytes were acked, tso and else */ > if (after(scb->end_seq, tp->snd_una)) { > if (tcp_skb_pcount(skb) == 1 || > @@ -3143,6 +3141,8 @@ static int tcp_clean_rtx_queue(struct sock *sk, u32 prior_fack, > if (!fully_acked) > break; > > + tcp_ack_tstamp(sk, skb, prior_snd_una); > + > next = skb_rb_next(skb); > if (unlikely(skb == tp->retransmit_skb_hint)) > tp->retransmit_skb_hint = NULL; This moves tcp_ack_tstamp beyond these two breaks: tcp_ack_tstamp(sk, skb, prior_snd_una); /* Determine how many packets and what bytes were acked, tso and else */ if (after(scb->end_seq, tp->snd_una)) { if (tcp_skb_pcount(skb) == 1 || !after(tp->snd_una, scb->seq)) break; acked_pcount = tcp_tso_acked(sk, skb); if (!acked_pcount) break; fully_acked = false; } else { acked_pcount = tcp_skb_pcount(skb); } but tcp_ack_tstamp does not necessarily act on the end_seq in the packet, it acts on shinfo->tskey: if (!before(shinfo->tskey, prior_snd_una) && before(shinfo->tskey, tcp_sk(sk)->snd_una)) { tcp_skb_tsorted_save(skb) { __skb_tstamp_tx(skb, NULL, sk, SCM_TSTAMP_ACK); } tcp_skb_tsorted_restore(skb); } on the next call to tcp_clean_rtx_queue tcp_sk(sk)->snd_una will be beyond tskey, so can this result in missing notifications?
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index f3a0eb139b76..2a683e785cca 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -3078,8 +3078,6 @@ static int tcp_clean_rtx_queue(struct sock *sk, u32 prior_fack, u8 sacked = scb->sacked; u32 acked_pcount; - tcp_ack_tstamp(sk, skb, prior_snd_una); - /* Determine how many packets and what bytes were acked, tso and else */ if (after(scb->end_seq, tp->snd_una)) { if (tcp_skb_pcount(skb) == 1 || @@ -3143,6 +3141,8 @@ static int tcp_clean_rtx_queue(struct sock *sk, u32 prior_fack, if (!fully_acked) break; + tcp_ack_tstamp(sk, skb, prior_snd_una); + next = skb_rb_next(skb); if (unlikely(skb == tp->retransmit_skb_hint)) tp->retransmit_skb_hint = NULL;