When the first iocg activate after blk_iocost_init(), now->vnow
maybe smaller than ioc->margins.target, cause very large vtarget
since it's u64.

	vtarget = now->vnow - ioc->margins.target;
	atomic64_add(vtarget - vtime, &iocg->vtime);

Then the iocg's vtime will be very large too, larger than now->vnow.

Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
---
 block/blk-iocost.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 5019dff524a4..a2ee12175c49 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -1248,7 +1248,7 @@ static bool iocg_activate(struct ioc_gq *iocg, struct ioc_now *now)
 {
 	struct ioc *ioc = iocg->ioc;
 	u64 last_period, cur_period;
-	u64 vtime, vtarget;
+	u64 vtime, vtarget = 0;
 	int i;
 
 	/*
@@ -1290,7 +1290,8 @@ static bool iocg_activate(struct ioc_gq *iocg, struct ioc_now *now)
 	 * Always start with the target budget. On deactivation, we throw away
 	 * anything above it.
 	 */
-	vtarget = now->vnow - ioc->margins.target;
+	if (now->vnow > ioc->margins.target)
+		vtarget = now->vnow - ioc->margins.target;
 	vtime = atomic64_read(&iocg->vtime);
 
 	atomic64_add(vtarget - vtime, &iocg->vtime);
-- 
2.36.1

On Mon, May 16, 2022 at 04:40:45PM +0800, Chengming Zhou wrote:
> When the first iocg activate after blk_iocost_init(), now->vnow
> maybe smaller than ioc->margins.target, cause very large vtarget
> since it's u64.
> 
> 	vtarget = now->vnow - ioc->margins.target;
> 	atomic64_add(vtarget - vtime, &iocg->vtime);
> 
> Then the iocg's vtime will be very large too, larger than now->vnow.

It's a wrapping counter. Please take a look at how time_before64() and
friends work.

Nacked-by: Tejun Heo <tj@kernel.org>

Again, please spend more effort understanding the code before sending these
subtle patches.

Thanks.

-- 
tejun

On 2022/5/17 02:46, Tejun Heo wrote:
> On Mon, May 16, 2022 at 04:40:45PM +0800, Chengming Zhou wrote:
>> When the first iocg activate after blk_iocost_init(), now->vnow
>> maybe smaller than ioc->margins.target, cause very large vtarget
>> since it's u64.
>>
>> 	vtarget = now->vnow - ioc->margins.target;
>> 	atomic64_add(vtarget - vtime, &iocg->vtime);
>>
>> Then the iocg's vtime will be very large too, larger than now->vnow.
> 
> It's a wrapping counter. Please take a look at how time_before64() and
> friends work.

Hi Tejun, below is from the trace of test on original code:

iocost_iocg_activate: [vda:/user.slice] now=38343468:2171657838 vrate=137438 \
period=0->0 vtime=18446744007162209454 weight=6553600/6553600 hweight=65536/65536

The vtime value is very large, much larger than vnow. Maybe the commit message
is a little misleading?

And I take a look at how time_before64() work:

#define time_after64(a,b)	\
	(typecheck(__u64, a) &&	\
	 typecheck(__u64, b) && \
	 ((__s64)((b) - (a)) < 0))
#define time_before64(a,b)	time_after64(b,a)

I still don't get why my changes are wrong. :-)

> 
> Nacked-by: Tejun Heo <tj@kernel.org>
> 
> Again, please spend more effort understanding the code before sending these
> subtle patches.

Ok, will do. This problem is found from the trace of test, then verified fixed
using the trace of the same test with this patch.

Thanks.

> 
> Thanks.
>

On Tue, May 17, 2022 at 08:57:55AM +0800, Chengming Zhou wrote:
> #define time_after64(a,b)	\
> 	(typecheck(__u64, a) &&	\
> 	 typecheck(__u64, b) && \
> 	 ((__s64)((b) - (a)) < 0))
> #define time_before64(a,b)	time_after64(b,a)
> 
> I still don't get why my changes are wrong. :-)

It's a wrapping timestamp where a lower value doesn't necessarily mean
earlier. The before/after relationship is defined only in relation to each
other. Imagine a cirle representing the whole value range and picking two
spots in the circle, if one is in the clockwise half from the other, the
former is said to be earlier than the latter and vice-versa. vtime runs way
faster than nanosecs and wraps regularly, so we can't use absolute values to
compare before/after.

Thanks.

-- 
tejun

On 2022/5/17 09:03, Tejun Heo wrote:
> On Tue, May 17, 2022 at 08:57:55AM +0800, Chengming Zhou wrote:
>> #define time_after64(a,b)	\
>> 	(typecheck(__u64, a) &&	\
>> 	 typecheck(__u64, b) && \
>> 	 ((__s64)((b) - (a)) < 0))
>> #define time_before64(a,b)	time_after64(b,a)
>>
>> I still don't get why my changes are wrong. :-)
> 
> It's a wrapping timestamp where a lower value doesn't necessarily mean
> earlier. The before/after relationship is defined only in relation to each
> other. Imagine a cirle representing the whole value range and picking two
> spots in the circle, if one is in the clockwise half from the other, the
> former is said to be earlier than the latter and vice-versa. vtime runs way
> faster than nanosecs and wraps regularly, so we can't use absolute values to
> compare before/after.

Please ignore my previous reply, you are right. I should fix the tracing
analysis tools to test again.

Thanks.

> 
> Thanks.
>

On 2022/5/17 09:03, Tejun Heo wrote:
> On Tue, May 17, 2022 at 08:57:55AM +0800, Chengming Zhou wrote:
>> #define time_after64(a,b)	\
>> 	(typecheck(__u64, a) &&	\
>> 	 typecheck(__u64, b) && \
>> 	 ((__s64)((b) - (a)) < 0))
>> #define time_before64(a,b)	time_after64(b,a)
>>
>> I still don't get why my changes are wrong. :-)
> 
> It's a wrapping timestamp where a lower value doesn't necessarily mean
> earlier. The before/after relationship is defined only in relation to each
> other. Imagine a cirle representing the whole value range and picking two
> spots in the circle, if one is in the clockwise half from the other, the
> former is said to be earlier than the latter and vice-versa. vtime runs way
> faster than nanosecs and wraps regularly, so we can't use absolute values to
> compare before/after.

Yes, thanks for the explanation. But the problem is not comparing two timestamp,
since ioc->margins.target is not a timestamp. This patch just fix a corner case
when now->vnow < ioc->margins.target:

u64 vtarget;

vtarget = now->vnow - ioc->margins.target; --> vtarget should be a timestamp earlier than vnow.

But when now->vnow < ioc->margins.target, vtarget would be a timestamp after vnow.

Thanks.

> 
> Thanks.
>