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: <Z70btzRaN83FbTJp@fedora>
Date: Tue, 25 Feb 2025 09:24:07 +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 Mon, Feb 24, 2025 at 08:03:32PM +0800, Yu Kuai wrote:
> Hi,
> 
> 在 2025/02/24 16:56, Ming Lei 写道:
> > On Mon, Feb 24, 2025 at 03:03:18PM +0800, Yu Kuai wrote:
> > > Hi, Ming!
> > > 
> > > 在 2025/02/24 11:28, Ming Lei 写道:
> > > > throtl_trim_slice() returns immediately if throtl_slice_used()
> > > > is true.
> > > > 
> > > > And throtl_slice_used() checks jiffies in [start, end] via time_in_range(),
> > > > so if `start <= jiffies <= end', it still returns false.
> > > 
> > > Yes, I misread the code, by thinking throtl_slice_used() will return
> > > true if the slice is still used. :(
> > > 
> > > 
> > > > > BTW, throtl_trim_slice() looks like problematic:
> > > > > 
> > > > > -       if (bytes_trim <= 0 && io_trim <= 0)
> > > > > +       if (bytes_trim <= 0 || io_trim <= 0 ||
> > > > > +           tg->bytes_disp[rw] < bytes_trim || tg->io_disp[rw] < io_trim)
> > > > >                   return;
> > > > That is exactly what my patch is doing, just taking deviation and
> > > > timeout into account, also U64_MAX limit has to be excluded.
> > > Yes, perhaps you can add some comments in the last two conditions of
> > > your patch.
> > 
> > Yes, we need to add comment on the check, how about the following words?
> > 
> > ```
> > 
> > If actually rate doesn't match with expected rate, do not trim slice
> > otherwise the present rate control info is lost, we don't have chance
> > to compensate it in the following period of this slice any more.
> 
> So, I just give your patch a test, and result is 1.3s while 1s is
> expected. While debuging, a new idea come up in mind. :)
> 
> How about keep at least one slice out of consideration from
> throtl_trim_slice()? With following patch, the result is between
> 1.01-1.03s in my VM.

That is easy to get the same result with the approach I suggested,
another big benefit: it is adaptive, and blk-throttle may get
simplified.

> 
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 8d149aff9fd0..5207c85098a5 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -604,9 +604,12 @@ static inline void throtl_trim_slice(struct throtl_grp
> *tg, bool rw)
> 
>         time_elapsed = rounddown(jiffies - tg->slice_start[rw],
>                                  tg->td->throtl_slice);
> -       if (!time_elapsed)
> +       /* don't trim slice until at least 2 slice is used */
> +       if (time_elapsed < tg->td->throtl_slice * 2)
>                 return;

If you just want to fix throtl/001, the above patch might
work(sometimes, it might not, and timer may expire by 2 jiffies), but it
is easy to fail other tests, such as, reduce the bps limit a bit, and
increase BS a bit to make the IO cross exactly two slices.

Also the big question is that how you can make sure that rate is always
good when the window is >= 2 slice?


Thanks,
Ming