bbr: account for bytes buffered at lower layers #1999
Conversation
| fn bbr_bytes_in_net(r: &Congestion, in_flight: usize, now: Instant) -> usize { | ||
| let edt = r.pacer.next_time().max(now); | ||
| let interval = edt.saturating_duration_since(now); | ||
| let interval_delivered = interval.as_secs_f64() * r.bbr_state.btlbw as f64; |
There was a problem hiding this comment.
I cant tell but is r.bbr_state.btlbw equivalent to bbr_bw. If its different should we call out the difference in a comment?
There was a problem hiding this comment.
There was a problem hiding this comment.
From what I can tell btlbw is the closest thing to bw in BBR1 state.
The RFC only seems to mention BBR.BtlBw.
| fn bbr_bytes_in_net(r: &Congestion, in_flight: usize, now: Instant) -> usize { | ||
| let edt = r.pacer.next_time().max(now); | ||
| let interval = edt.saturating_duration_since(now); | ||
| let interval_delivered = interval.as_secs_f64() * r.bbr_state.btlbw as f64; |
There was a problem hiding this comment.
| let mut in_flight_at_edt = in_flight; | ||
|
|
||
| // pacing_gain > 1.0 | ||
| if (r.bbr_state.pacing_gain - 1.0).abs() > f64::EPSILON { |
There was a problem hiding this comment.
Is this .abs() call correct?
If pacing gain were 0.8, we'ld add the send_quantum. This doesn't seem right.
This check is effectively pacing_gain != 1.0, not pacing_gain > 1.0
I see use of .abs() in other places, we may want to audit those too.
There was a problem hiding this comment.
See existing impl of pacing_gain == 1.0
There was a problem hiding this comment.
Yeah, I misunderstood the abs() comparison, and indeed we already do a > 1.0 check a few lines below that one
Somewhat suspiciously the bbr_drain() test started failing again after that change, and to fix it I had to increase the number of sent packets even more (to 7, compared to the original 5). The test itself doesn't take pacing rate into account which I suspect is what causes the mismatch (since the new code expects pacing to be done)... I also realized it had a seemingly stray comment ("Stop at right before filled_pipe=true") that doesn't seem to make sense since it's actually asserting filled_pipe=true afterwards so I removed it...
There was a problem hiding this comment.
Comparing against > epsilon or > 1.0 + epsilon is more correct, but it I don't know how much error we can end up accumulating here. Is gain of 1.0 something that can only happen when we assign 1.0, or can it also happen after (pacing_gain * N) followed by * (1/N) or pacing_gain + N followed by - N?
For example, due to pacing offload some packets might be buffered in the fq qdisc queue and not yet sent on the network. This is more or less meant to be equivalent to Linux' `bbr_packets_in_net_at_edt()` function.
For example, due to pacing offload some packets might be buffered in the fq qdisc queue and not yet sent on the network.
This is more or less meant to be equivalent to Linux'
bbr_packets_in_net_at_edt()function.This is an alternative to #1636 and instead of walking the list of sent packets which is quite expensive, we just sort of estimate the amount of bytes by looking at BW and pacing rate.