| Message ID | 1650422081-22153-1-git-send-email-yangpc@wangsu.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b253a0680ceadc5d7b4acca7aa2d870326cad8ad |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3] tcp: ensure to use the most recently sent skb when filling the rate sample | expand |
On Wed, 20 Apr 2022 10:34:41 +0800 Pengcheng Yang wrote: > 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: b9f64820fb22 ("tcp: track data delivery rate for a TCP connection") > Signed-off-by: Pengcheng Yang <yangpc@wangsu.com> > Cc: Neal Cardwell <ncardwell@google.com> > Cc: Paolo Abeni <pabeni@redhat.com> Somehow this patch got marked as archived in patchwork. Reviving it now. Eric, Neal, ack?
On Fri, Apr 22, 2022 at 4:37 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Wed, 20 Apr 2022 10:34:41 +0800 Pengcheng Yang wrote: > > 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: b9f64820fb22 ("tcp: track data delivery rate for a TCP connection") > > Signed-off-by: Pengcheng Yang <yangpc@wangsu.com> > > Cc: Neal Cardwell <ncardwell@google.com> > > Cc: Paolo Abeni <pabeni@redhat.com> > > Somehow this patch got marked as archived in patchwork. Reviving it now. > > Eric, Neal, ack? Acked-by: Neal Cardwell <ncardwell@google.com> Tested-by: Neal Cardwell <ncardwell@google.com> Looks good to me. Thanks for the patch! neal
On Fri, Apr 22, 2022 at 1:37 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Wed, 20 Apr 2022 10:34:41 +0800 Pengcheng Yang wrote: > > 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: b9f64820fb22 ("tcp: track data delivery rate for a TCP connection") > > Signed-off-by: Pengcheng Yang <yangpc@wangsu.com> > > Cc: Neal Cardwell <ncardwell@google.com> > > Cc: Paolo Abeni <pabeni@redhat.com> > > Somehow this patch got marked as archived in patchwork. Reviving it now. > > Eric, Neal, ack? Oops, right, thanks ! Reviewed-by: Eric Dumazet <edumazet@google.com>
Hello: This patch was applied to netdev/net.git (master) by Jakub Kicinski <kuba@kernel.org>: On Wed, 20 Apr 2022 10:34:41 +0800 you wrote: > 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. > > [...] Here is the summary with links: - [net,v3] tcp: ensure to use the most recently sent skb when filling the rate sample https://git.kernel.org/netdev/net/c/b253a0680cea You are awesome, thank you!
diff --git a/include/net/tcp.h b/include/net/tcp.h index 6d50a66..fcd69fc 100644 --- a/include/net/tcp.h +++ b/include/net/tcp.h @@ -1042,6 +1042,7 @@ struct rate_sample { int losses; /* number of packets marked lost upon ACK */ u32 acked_sacked; /* number of packets newly (S)ACKed upon ACK */ u32 prior_in_flight; /* in flight before this ACK */ + u32 last_end_seq; /* end_seq of most recently ACKed packet */ bool is_app_limited; /* is sample from packet with bubble in pipe? */ bool is_retrans; /* is sample from retransmission? */ bool is_ack_delayed; /* is this (likely) a delayed ACK? */ @@ -1158,6 +1159,11 @@ void tcp_rate_gen(struct sock *sk, u32 delivered, u32 lost, bool is_sack_reneg, struct rate_sample *rs); void tcp_rate_check_app_limited(struct sock *sk); +static inline bool tcp_skb_sent_after(u64 t1, u64 t2, u32 seq1, u32 seq2) +{ + return t1 > t2 || (t1 == t2 && after(seq1, seq2)); +} + /* These functions determine how the current flow behaves in respect of SACK * handling. SACK is negotiated with the peer, and therefore it can vary * between different flows. diff --git a/net/ipv4/tcp_rate.c b/net/ipv4/tcp_rate.c index 617b818..a8f6d9d 100644 --- a/net/ipv4/tcp_rate.c +++ b/net/ipv4/tcp_rate.c @@ -74,27 +74,32 @@ void tcp_rate_skb_sent(struct sock *sk, struct sk_buff *skb) * * If an ACK (s)acks multiple skbs (e.g., stretched-acks), this function is * called multiple times. We favor the information from the most recently - * sent skb, i.e., the skb with the highest prior_delivered count. + * sent skb, i.e., the skb with the most recently sent time and the highest + * sequence. */ void tcp_rate_skb_delivered(struct sock *sk, struct sk_buff *skb, struct rate_sample *rs) { struct tcp_sock *tp = tcp_sk(sk); struct tcp_skb_cb *scb = TCP_SKB_CB(skb); + u64 tx_tstamp; if (!scb->tx.delivered_mstamp) return; + tx_tstamp = tcp_skb_timestamp_us(skb); if (!rs->prior_delivered || - after(scb->tx.delivered, rs->prior_delivered)) { + tcp_skb_sent_after(tx_tstamp, tp->first_tx_mstamp, + scb->end_seq, rs->last_end_seq)) { rs->prior_delivered_ce = scb->tx.delivered_ce; rs->prior_delivered = scb->tx.delivered; rs->prior_mstamp = scb->tx.delivered_mstamp; rs->is_app_limited = scb->tx.is_app_limited; rs->is_retrans = scb->sacked & TCPCB_RETRANS; + rs->last_end_seq = scb->end_seq; /* Record send time of most recently ACKed packet: */ - tp->first_tx_mstamp = tcp_skb_timestamp_us(skb); + tp->first_tx_mstamp = tx_tstamp; /* Find the duration of the "send phase" of this window: */ rs->interval_us = tcp_stamp_us_delta(tp->first_tx_mstamp, scb->tx.first_tx_mstamp);
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: b9f64820fb22 ("tcp: track data delivery rate for a TCP connection") Signed-off-by: Pengcheng Yang <yangpc@wangsu.com> Cc: Neal Cardwell <ncardwell@google.com> Cc: Paolo Abeni <pabeni@redhat.com> --- include/net/tcp.h | 6 ++++++ net/ipv4/tcp_rate.c | 11 ++++++++--- 2 files changed, 14 insertions(+), 3 deletions(-)