[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] ratelimit: restrict the delay time to a non-negative value
From: |
Wang Liang |
Subject: |
Re: [PATCH] ratelimit: restrict the delay time to a non-negative value |
Date: |
Wed, 21 Sep 2022 14:26:22 +0800 |
On Wed, 2022-09-21 at 06:53 +0200, Markus Armbruster wrote:
> Wang Liang <wangliangzz@126.com> writes:
>
> > On Tue, 2022-09-20 at 13:18 +0000, Alberto Garcia wrote:
> > > On Tue 20 Sep 2022 08:33:50 PM +08, wangliangzz@126.com wrote:
> > > > From: Wang Liang <wangliangzz@inspur.com>
> > > >
> > > > The delay time should never be a negative value.
> > > >
> > > > - return limit->slice_end_time - now;
> > > > + return MAX(limit->slice_end_time - now, 0);
> > >
> > > How can this be negative? slice_end_time is guaranteed to be
> > > larger
> > > than
> > > now:
> > >
> > > if (limit->slice_end_time < now) {
> > > /* Previous, possibly extended, time slice finished;
> > > reset
> > > the
> > > * accounting. */
> > > limit->slice_start_time = now;
> > > limit->slice_end_time = now + limit->slice_ns;
> > > limit->dispatched = 0;
> > > }
> > >
> > This is just a guarantee.
>
> Smells like an invariant to me.
>
> > If slice_end_time is assigned later by
> > limit->slice_end_time = limit->slice_start_time +
> > (uint64_t)(delay_slices * limit->slice_ns);
> > There may be precision issues at that time.
>
> What are the issues exactly? What misbehavior are you observing?
>
> Your commit message should show how delay time can become negative,
> and
> why that's bad.
It was observed in a production environment based on qemu v2.12.1.
The block-stream job delayed a very long time and do not get any
progress since ratelimit_calculate_delay returns a negative value.
Sorry, I don't have an environment to reproduce it in the mainline
version now.