From 119cfaa88f307f854b65adcc87ecde93d9211663 Mon Sep 17 00:00:00 2001 From: Neal Cardwell Date: Thu, 21 Nov 2019 15:28:01 -0500 Subject: [PATCH] net-tcp_bbr: v2: remove unnecessary rs.delivered_ce logic upon loss 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 --- net/ipv4/tcp_bbr2.c | 1 - 1 file changed, 1 deletion(-) diff --git a/net/ipv4/tcp_bbr2.c b/net/ipv4/tcp_bbr2.c index a6959b70e51d1..e00b47850dcef 100644 --- a/net/ipv4/tcp_bbr2.c +++ b/net/ipv4/tcp_bbr2.c @@ -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);