Re: [PATCH 5/6 v4] cfq-iosched: CFQ group hierarchical schedulingand use_hierarchy interface

[Gui Jianfeng]

Justin TerAvest wrote:
> On Wed, Feb 16, 2011 at 5:21 PM, Gui Jianfeng
> <guijianfeng@xxxxxxxxxxxxxx> wrote:
>> Justin TerAvest wrote:
>>> After a quick read,
>>>
>>> It's sad that we have to have so many use_hierarchy checks; it seems
>>> like we're asking for bugs, especially in the future when one codepath
>>> gets updated but not the other.
>>>
>>> CodingStyle says we should only have one declaration per line.
>>>
>>> I feel like there is an implicit assumption that groups and tasks
>>> should not be children of the same parent; that is, a group should
>>> contain only groups, or only tasks, but I don't see this enforced;
>>> there's just and assumption that BE:SYNC is "good enough" for that
>>> comparison. This smells like something that will be tweaked/tuned for
>>> fairness later. :( Why don't we just prevent this from happening?
>> Hi Justin,
>>
>> Thanks for reviewing.
>>
>> Previously, I posted very first version that makes a group containing only
>> groups or only tasks. But I think it's more flexible to treat groups and
>> tasks at the same level. I think Vivek and Jens have the same opinion.
>> We had discussed in this thread http://lkml.org/lkml/2010/8/30/30
> 
> Hi Gui,
> Thanks for pointing me at the earlier discussion, the decisions make a
> lot more sense now.
> 
>>> The clean_up label in chain_alloc() is strange; I don't think the goto
>>> is necessary at all. I found that method generally hard to understand.
>>> It's doing a lot.
>> I don't understand why clean_up isn't needed.
>> When we fail to allocate a cfq group at some level, we have to clean up
>> all groups in the chain that we have already allocated.
> 
> Cleaning up is necessary, but the label is only used from one place.
> Why add the goto and the label when the code below "clean_up" can just
> be moved inside the condition
> +                       if (!cfqg)

It's common in kernel to put error processing at the end of a function. ;)

Thanks,
Gui

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/