Re: [PATCH net-next v2 1/2] tcp: ensure to use the most recently sent skb when filling the rate sample

[Paolo Abeni <pabeni@redhat.com>]

On Sun, 2022-04-17 at 14:51 -0400, Neal Cardwell wrote:
> On Sat, Apr 16, 2022 at 5:20 AM Pengcheng Yang <yangpc@wangsu.com> 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.
> > 
> > Signed-off-by: Pengcheng Yang <yangpc@wangsu.com>
> > Cc: Neal Cardwell <ncardwell@google.com>
> > ---
> >  include/net/tcp.h   |  6 ++++++
> >  net/ipv4/tcp_rate.c | 11 ++++++++---
> >  2 files changed, 14 insertions(+), 3 deletions(-)
> > 
> > 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);
> > --
> 
> Thanks for the patch! The change looks good to me, and it passes our
> team's packetdrill tests.
> 
> One suggestion: currently this patch seems to be targeted to the
> net-next branch. However, since it's a bug fix my sense is that it
> would be best to target this to the net branch, so that it gets
> backported to stable releases.
> 
> One complication is that the follow-on patch in this series ("tcp: use
> tcp_skb_sent_after() instead in RACK") is a pure re-factor/cleanup,
> which is more appropriate for net-next. So the plan I was trying to
> describe in the previous thread was that this series could be
> implemented as:
> 
> (1) first, submit "tcp: ensure to use the most recently sent skb when
> filling the rate sample" to the net branch
> (2) wait for the fix in the net branch to be merged into the net-next branch
> (3) second, submit "tcp: use tcp_skb_sent_after() instead in RACK" to
> the net-next branch
> 
> What do folks think?

+1 for the above.

@Pengcheng: please additionally provide a suitable 'fixes' tag for
patch 1/2.

Thanks!

Paolo