linux-kernel - Re: [PATCH 2/2] blk-throttle: fix off-by-one jiffies wait

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z719gj8GOl0itRwV@fedora>
Date: Tue, 25 Feb 2025 16:21:22 +0800
From: Ming Lei <ming.lei@...hat.com>
To: Yu Kuai <yukuai1@...weicloud.com>
Cc: tj@...nel.org, josef@...icpanda.com, axboe@...nel.dk,
	cgroups@...r.kernel.org, linux-block@...r.kernel.org,
	linux-kernel@...r.kernel.org, yi.zhang@...wei.com,
	yangerkun@...wei.com, "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH 2/2] blk-throttle: fix off-by-one jiffies wait_time

On Tue, Feb 25, 2025 at 11:12:24AM +0800, Yu Kuai wrote:
> Hi, Ming!
> 
> 在 2025/02/25 10:28, Ming Lei 写道:
> > Can you explain in details why it signals that the rate is expected now?
> > 
> > If rate isn't expected, it will cause trouble to trim, even just the
> > previous part.
> 
> Ok, for example, assume bps_limit is 1000bytes, 1 jiffes is 10ms, and
> slice is 20ms(2 jiffies).
> 

We all know how it works, but I didn't understand the behind idea why it
is correct. Now I figured it out: 

1) increase default slice window to 2 * td->throttle_slice

2) slice window is set as [jiffies - td->throttle_slice, jiffies + td->throttle_slice]

3) initialize td->bytes_disp[]/td->io_dis[] as actual dispatched bytes/ios
done [jiffies - td->throttle_slice, 0]

This approach looks smart, and it should work well for any deviation which is <= 1
throttle_slice.

Probably it is enough for fixing the issue in throtl/001, even though 2 jiffies
timer drift still may be observed, see the below log collected in my VM(HZ_100)
by just running one time of blktests './check throtl':

@timer_expire_delay:
[1, 2)               387 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
[2, 3)                11 |@                                                   |

bpftrace -e 'kfunc:throtl_pending_timer_fn { @timer_expire_delay = lhist(jiffies - args->t->expires, 0, 16, 1);}'


Also I'd suggest to remove ->carryover_bytes/ios since blk-throttle algorithm is
supposed to be adaptive, and the approach I suggested may cover this area,
what do you think of this cleanup? I have one local patchset, which can
pass all blktest throtl tests with removing ->carryover_bytes/ios.



Thanks,
Ming