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

[Jiufei Xue <jiufei.xue@linux.alibaba.com>]

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