On Fri, Feb 21, 2025 at 02:29:30PM +0800, Yu Kuai wrote: > Hi, > > 在 2025/02/21 12:18, Ming Lei 写道: > > > - jiffy_wait = div64_u64(extra_bytes * HZ, bps_limit); > > > - > > > - if (!jiffy_wait) > > > - jiffy_wait = 1; > > > + jiffy_wait = div64_u64_rem(extra_bytes * HZ, bps_limit, > > > &carryover_bytes); > > > + tg->carryover_bytes[rw] -= div64_u64(carryover_bytes, HZ); > > Can you explain a bit why `carryover_bytes/HZ` is subtracted instead of > > carryover_bytes? > > For example, if bps_limit is 1000, extra_bytes is 9, then: > > jiffy_wait = (9 * 100) / 1000 = 0; > carryover_bytes = (9 * 100) % 1000 = 900; > > Hence we need to divide it by HZ: > tg->carryover_bytes = 0 - 900/100 = -9; > > -9 can be considered debt, and for the next IO, the bytes_allowed will > consider the carryover_bytes. Got it, it is just because the dividend is 'extra_bytes * HZ', so the remainder need to be divided by HZ. > > > > Also tg_within_bps_limit() may return 0 now, which isn't expected. > > I think it's expected, this IO will now be dispatched directly instead > of wait for one more jiffies, and debt will be paid if there are > following IOs. OK. > > And if the tg idle for a long time before dispatching the next IO, > tg_trim_slice() should handle this case and avoid long slice end. This > is not quite handy, perhaps it's better to add a helper like > tg_in_debt() before throtl_start_new_slice() to hande this case. > > BTW, we must update the comment about carryover_bytes/ios, it's alredy > used as debt. Indeed, the approach is similar with the handling for bio_issue_as_root_blkg(). Tested-by: Ming Lei <ming.lei@xxxxxxxxxx> Thanks, Ming
