Skip to content

Commit

Permalink
net-tcp_bbr: v2: remove unnecessary rs.delivered_ce logic upon loss
Browse files Browse the repository at this point in the history
There is no reason to compute rs.delivered_ce upon loss.

In fact, we specifically do not want to compute rs.delivered_ce upon loss.

Two issues:

(1) This would be the wrong thing to do, in behavior terms.  With
    RACK's dynamic reordering window, losses can be marked long after
    the sequence hole appears in the ACK/SACK stream. We want to to
    catch the ECN mark rate rising too high as quickly as possible,
    which means we want to check for high ECN mark rates at ACK time
    (as BBRv2 currently does) and not loss marking time.

(2) This is dead code. The ECN mark rate cannot be detected as too
    high because the check needs rs->delivered to be > 0 as well:

       if (rs->delivered_ce > 0 && rs->delivered > 0 &&

    Since we are not setting rs->delivered upon loss, this check
    cannot succeed, so setting delivered_ce is pointless.

This dead and wrong line was discovered by Randall Stewart at Netflix
as he was reading the BBRv2 code.

Change-Id: I37f83f418a259ec31d8f82de986db071b364b76a
  • Loading branch information
nealcardwell authored and xanmod committed Apr 27, 2021
1 parent 14abdf5 commit 5cc6335
Showing 1 changed file with 0 additions and 1 deletion.
1 change: 0 additions & 1 deletion net/ipv4/tcp_bbr2.c
Expand Up @@ -2508,7 +2508,6 @@ static void bbr2_skb_marked_lost(struct sock *sk, const struct sk_buff *skb)
memset(&rs, 0, sizeof(rs));
rs.tx_in_flight = scb->tx.in_flight;
rs.lost = tp->lost - scb->tx.lost;
rs.delivered_ce = tp->delivered_ce - scb->tx.delivered_ce;
rs.is_app_limited = scb->tx.is_app_limited;
if (bbr2_is_inflight_too_high(sk, &rs)) {
rs.tx_in_flight = bbr2_inflight_hi_from_lost_skb(sk, &rs, skb);
Expand Down

0 comments on commit 5cc6335

Please sign in to comment.