[PATCH] iocost: treat as root level when parents are activated

[PATCH] iocost: treat as root level when parents are activated

[Jiufei Xue]

Internal nodes that issued IOs are treated as root when leaves are
activated. However, leaf nodes can still be activated when internal
nodes are active, leaving the sum of hweights exceeds HWEIGHT_WHOLE.

I think we should also treat the leaf nodes as root while leaf-only
constraint broken.

Signed-off-by: Jiufei Xue <jiufei.xue@linux.alibaba.com>
---
 block/blk-iocost.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index a7ed434..d3ca6e8 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -1057,8 +1057,8 @@ static bool iocg_activate(struct ioc_gq *iocg, struct ioc_now *now)
 	atomic64_set(&iocg->active_period, cur_period);
 
 	/* already activated or breaking leaf-only constraint? */
-	for (i = iocg->level; i > 0; i--)
-		if (!list_empty(&iocg->active_list))
+	for (i = iocg->level - 1; i > 0; i--)
+		if (!list_empty(&iocg->ancestors[i]->active_list))
 			goto fail_unlock;
 	if (iocg->child_active_sum)
 		goto fail_unlock;
-- 
1.8.3.1

Re: [PATCH] iocost: treat as root level when parents are activated

[Tejun Heo]

Hello, Jiufei.

On Mon, Nov 11, 2019 at 03:37:18PM +0800, Jiufei Xue wrote:
> Internal nodes that issued IOs are treated as root when leaves are
> activated. However, leaf nodes can still be activated when internal
> nodes are active, leaving the sum of hweights exceeds HWEIGHT_WHOLE.
> 
> I think we should also treat the leaf nodes as root while leaf-only
> constraint broken.

Hmm... I'm not sure this description makes sense.

> @@ -1057,8 +1057,8 @@ static bool iocg_activate(struct ioc_gq *iocg, struct ioc_now *now)
>  	atomic64_set(&iocg->active_period, cur_period);
>  
>  	/* already activated or breaking leaf-only constraint? */
> -	for (i = iocg->level; i > 0; i--)
> -		if (!list_empty(&iocg->active_list))
> +	for (i = iocg->level - 1; i > 0; i--)
> +		if (!list_empty(&iocg->ancestors[i]->active_list))

But there's an obvious bug there as it's checking the same active_list
over and over again.  Shouldn't it be sth like the following instead?

	if (!list_empty(&iocg->active_list))
		goto succeed_unlock;
	for (i = iocg->level - 1; i > 0; i--)
		if (!list_empty(&iocg->ancestors[i]->active_list))
			goto fail_unlock;

Thanks.

-- 
tejun

Re: [PATCH] iocost: treat as root level when parents are activated

[Jiufei Xue]

Hi Tejun,

On 2019/11/12 上午12:25, Tejun Heo wrote:
> Hello, Jiufei.
> 
> On Mon, Nov 11, 2019 at 03:37:18PM +0800, Jiufei Xue wrote:
>> Internal nodes that issued IOs are treated as root when leaves are
>> activated. However, leaf nodes can still be activated when internal
>> nodes are active, leaving the sum of hweights exceeds HWEIGHT_WHOLE.
>>
>> I think we should also treat the leaf nodes as root while leaf-only
>> constraint broken.
> 
> Hmm... I'm not sure this description makes sense.
> 
Should I change the description to something like this?
"we should treat the leaf nodes as root while the parent are already activated".


>> @@ -1057,8 +1057,8 @@ static bool iocg_activate(struct ioc_gq *iocg, struct ioc_now *now)
>>  	atomic64_set(&iocg->active_period, cur_period);
>>  
>>  	/* already activated or breaking leaf-only constraint? */
>> -	for (i = iocg->level; i > 0; i--)
>> -		if (!list_empty(&iocg->active_list))
>> +	for (i = iocg->level - 1; i > 0; i--)
>> +		if (!list_empty(&iocg->ancestors[i]->active_list))
> 
> But there's an obvious bug there as it's checking the same active_list
> over and over again.  Shouldn't it be sth like the following instead?
> 
> 	if (!list_empty(&iocg->active_list))
> 		goto succeed_unlock;

iocg has already checked before, do you mean we should check it again
after ioc->lock?

> 	for (i = iocg->level - 1; i > 0; i--)
> 		if (!list_empty(&iocg->ancestors[i]->active_list))
> 			goto fail_unlock;
> 
> Thanks.
>

Thanks,
Jiufei

Re: [PATCH] iocost: treat as root level when parents are activated

[Tejun Heo]

Hello,

On Tue, Nov 12, 2019 at 09:38:57AM +0800, Jiufei Xue wrote:
> > Hmm... I'm not sure this description makes sense.
> > 
> Should I change the description to something like this?
> "we should treat the leaf nodes as root while the parent are already activated".

Hmm... this is addressing an obvious bug.  The intention of the code
was checking whether all the ancestors and self have already been
activated but it just failed to do so, so I think the patch
description should reflect that.

> > But there's an obvious bug there as it's checking the same active_list
> > over and over again.  Shouldn't it be sth like the following instead?
> > 
> > 	if (!list_empty(&iocg->active_list))
> > 		goto succeed_unlock;
> 
> iocg has already checked before, do you mean we should check it again
> after ioc->lock?

Yes, that part of the code is correct.  It needs to check it again as
someone could have changed it since the previous lockless
opportunistic checking.

Thanks.

-- 
tejun