Skip to content

Commit b253a06

Browse files
ws-yangpckuba-moo
authored andcommitted
tcp: ensure to use the most recently sent skb when filling the rate sample
If an ACK (s)acks multiple skbs, we favor the information from the most recently sent skb by choosing the skb with the highest prior_delivered count. But in the interval between receiving ACKs, we send multiple skbs with the same prior_delivered, because the tp->delivered only changes when we receive an ACK. We used RACK's solution, copying tcp_rack_sent_after() as tcp_skb_sent_after() helper to determine "which packet was sent last?". Later, we will use tcp_skb_sent_after() instead in RACK. Fixes: b9f6482 ("tcp: track data delivery rate for a TCP connection") Signed-off-by: Pengcheng Yang <[email protected]> Cc: Paolo Abeni <[email protected]> Acked-by: Neal Cardwell <[email protected]> Tested-by: Neal Cardwell <[email protected]> Reviewed-by: Eric Dumazet <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Jakub Kicinski <[email protected]>
1 parent b107a63 commit b253a06

File tree

2 files changed

+14
-3
lines changed

2 files changed

+14
-3
lines changed

include/net/tcp.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1042,6 +1042,7 @@ struct rate_sample {
10421042
int losses; /* number of packets marked lost upon ACK */
10431043
u32 acked_sacked; /* number of packets newly (S)ACKed upon ACK */
10441044
u32 prior_in_flight; /* in flight before this ACK */
1045+
u32 last_end_seq; /* end_seq of most recently ACKed packet */
10451046
bool is_app_limited; /* is sample from packet with bubble in pipe? */
10461047
bool is_retrans; /* is sample from retransmission? */
10471048
bool is_ack_delayed; /* is this (likely) a delayed ACK? */
@@ -1164,6 +1165,11 @@ void tcp_rate_gen(struct sock *sk, u32 delivered, u32 lost,
11641165
bool is_sack_reneg, struct rate_sample *rs);
11651166
void tcp_rate_check_app_limited(struct sock *sk);
11661167

1168+
static inline bool tcp_skb_sent_after(u64 t1, u64 t2, u32 seq1, u32 seq2)
1169+
{
1170+
return t1 > t2 || (t1 == t2 && after(seq1, seq2));
1171+
}
1172+
11671173
/* These functions determine how the current flow behaves in respect of SACK
11681174
* handling. SACK is negotiated with the peer, and therefore it can vary
11691175
* between different flows.

net/ipv4/tcp_rate.c

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,27 +74,32 @@ void tcp_rate_skb_sent(struct sock *sk, struct sk_buff *skb)
7474
*
7575
* If an ACK (s)acks multiple skbs (e.g., stretched-acks), this function is
7676
* called multiple times. We favor the information from the most recently
77-
* sent skb, i.e., the skb with the highest prior_delivered count.
77+
* sent skb, i.e., the skb with the most recently sent time and the highest
78+
* sequence.
7879
*/
7980
void tcp_rate_skb_delivered(struct sock *sk, struct sk_buff *skb,
8081
struct rate_sample *rs)
8182
{
8283
struct tcp_sock *tp = tcp_sk(sk);
8384
struct tcp_skb_cb *scb = TCP_SKB_CB(skb);
85+
u64 tx_tstamp;
8486

8587
if (!scb->tx.delivered_mstamp)
8688
return;
8789

90+
tx_tstamp = tcp_skb_timestamp_us(skb);
8891
if (!rs->prior_delivered ||
89-
after(scb->tx.delivered, rs->prior_delivered)) {
92+
tcp_skb_sent_after(tx_tstamp, tp->first_tx_mstamp,
93+
scb->end_seq, rs->last_end_seq)) {
9094
rs->prior_delivered_ce = scb->tx.delivered_ce;
9195
rs->prior_delivered = scb->tx.delivered;
9296
rs->prior_mstamp = scb->tx.delivered_mstamp;
9397
rs->is_app_limited = scb->tx.is_app_limited;
9498
rs->is_retrans = scb->sacked & TCPCB_RETRANS;
99+
rs->last_end_seq = scb->end_seq;
95100

96101
/* Record send time of most recently ACKed packet: */
97-
tp->first_tx_mstamp = tcp_skb_timestamp_us(skb);
102+
tp->first_tx_mstamp = tx_tstamp;
98103
/* Find the duration of the "send phase" of this window: */
99104
rs->interval_us = tcp_stamp_us_delta(tp->first_tx_mstamp,
100105
scb->tx.first_tx_mstamp);

0 commit comments

Comments
 (0)