* [PATCH] blk-iocost: fix very large vtime when iocg activate
@ 2022-05-16 8:40 Chengming Zhou
2022-05-16 18:46 ` Tejun Heo
0 siblings, 1 reply; 6+ messages in thread
From: Chengming Zhou @ 2022-05-16 8:40 UTC (permalink / raw)
To: tj, axboe
Cc: linux-block, linux-kernel, duanxiongchun, songmuchun, Chengming Zhou
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] blk-iocost: fix very large vtime when iocg activate
2022-05-16 8:40 [PATCH] blk-iocost: fix very large vtime when iocg activate Chengming Zhou
@ 2022-05-16 18:46 ` Tejun Heo
2022-05-17 0:57 ` [Phishing Risk] [External] " Chengming Zhou
0 siblings, 1 reply; 6+ messages in thread
From: Tejun Heo @ 2022-05-16 18:46 UTC (permalink / raw)
To: Chengming Zhou
Cc: axboe, linux-block, linux-kernel, duanxiongchun, songmuchun
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Phishing Risk] [External] Re: [PATCH] blk-iocost: fix very large vtime when iocg activate
2022-05-16 18:46 ` Tejun Heo
@ 2022-05-17 0:57 ` Chengming Zhou
2022-05-17 1:03 ` Tejun Heo
0 siblings, 1 reply; 6+ messages in thread
From: Chengming Zhou @ 2022-05-17 0:57 UTC (permalink / raw)
To: Tejun Heo; +Cc: axboe, linux-block, linux-kernel, duanxiongchun, songmuchun
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.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Phishing Risk] [External] Re: [PATCH] blk-iocost: fix very large vtime when iocg activate
2022-05-17 0:57 ` [Phishing Risk] [External] " Chengming Zhou
@ 2022-05-17 1:03 ` Tejun Heo
2022-05-17 1:23 ` [Phishing Risk] " Chengming Zhou
2022-05-17 1:39 ` Chengming Zhou
0 siblings, 2 replies; 6+ messages in thread
From: Tejun Heo @ 2022-05-17 1:03 UTC (permalink / raw)
To: Chengming Zhou
Cc: axboe, linux-block, linux-kernel, duanxiongchun, songmuchun
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Phishing Risk] Re: [Phishing Risk] [External] Re: [PATCH] blk-iocost: fix very large vtime when iocg activate
2022-05-17 1:03 ` Tejun Heo
@ 2022-05-17 1:23 ` Chengming Zhou
2022-05-17 1:39 ` Chengming Zhou
1 sibling, 0 replies; 6+ messages in thread
From: Chengming Zhou @ 2022-05-17 1:23 UTC (permalink / raw)
To: Tejun Heo; +Cc: axboe, linux-block, linux-kernel, duanxiongchun, songmuchun
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.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Phishing Risk] Re: [Phishing Risk] [External] Re: [PATCH] blk-iocost: fix very large vtime when iocg activate
2022-05-17 1:03 ` Tejun Heo
2022-05-17 1:23 ` [Phishing Risk] " Chengming Zhou
@ 2022-05-17 1:39 ` Chengming Zhou
1 sibling, 0 replies; 6+ messages in thread
From: Chengming Zhou @ 2022-05-17 1:39 UTC (permalink / raw)
To: Tejun Heo; +Cc: axboe, linux-block, linux-kernel, duanxiongchun, songmuchun
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.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-05-17 1:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-16 8:40 [PATCH] blk-iocost: fix very large vtime when iocg activate Chengming Zhou
2022-05-16 18:46 ` Tejun Heo
2022-05-17 0:57 ` [Phishing Risk] [External] " Chengming Zhou
2022-05-17 1:03 ` Tejun Heo
2022-05-17 1:23 ` [Phishing Risk] " Chengming Zhou
2022-05-17 1:39 ` Chengming Zhou
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).