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

[Gui Jianfeng <guijianfeng@cn.fujitsu.com>]

Vivek Goyal wrote:
> On Thu, Feb 10, 2011 at 03:47:45PM +0800, Gui Jianfeng wrote:
>> CFQ group hierarchical scheduling and use_hierarchy interface.
>>
> 
> Hi Gui,
> 
> I have done a quick high level review. Some minor comments inline.
> 
> [..]
>>  struct cfq_data {
>>  	struct request_queue *queue;
>> -	/* Root service tree for cfq_groups */
>> -	struct cfq_rb_root grp_service_tree;
>>  	struct cfq_group root_group;
>>  
>> +	/* cfq group schedule in flat or hierarchy manner. */
>> +	bool use_hierarchy;
>> +
> 
> This seems to be redundant now? Nobody is using it?
> 
>>  	/*
>>  	 * The priority currently being served
>>  	 */
>> @@ -246,6 +251,9 @@ struct cfq_data {
>>  	unsigned long workload_expires;
>>  	struct cfq_group *serving_group;
>>  
>> +	/* Service tree for cfq group flat scheduling mode. */
>> +	struct cfq_rb_root grp_service_tree;
> 
> Above comment is misleading. This service tree is now used both for
> flat as well as hierarhical mode.
> 
> [..]
>>  static void
>>  cfq_group_service_tree_add(struct cfq_data *cfqd, struct cfq_group *cfqg)
>>  {
>> -	struct cfq_rb_root *st = &cfqd->grp_service_tree;
>>  	struct cfq_entity *cfqe = &cfqg->cfqe;
>> -	struct cfq_entity *__cfqe;
>>  	struct rb_node *n;
>> +	struct cfq_entity *entity;
>> +	struct cfq_rb_root *st;
>> +	struct cfq_group *__cfqg;
>>  
>>  	cfqg->nr_cfqq++;
>> +
>>  	if (!RB_EMPTY_NODE(&cfqe->rb_node))
>>  		return;
>>  
>>  	/*
>> -	 * Currently put the group at the end. Later implement something
>> -	 * so that groups get lesser vtime based on their weights, so that
>> -	 * if group does not loose all if it was not continously backlogged.
>> +	 * Enqueue this group and its ancestors onto their service tree.
>>  	 */
>> -	n = rb_last(&st->rb);
>> -	if (n) {
>> -		__cfqe = rb_entry_entity(n);
>> -		cfqe->vdisktime = __cfqe->vdisktime + CFQ_IDLE_DELAY;
>> -	} else
>> -		cfqe->vdisktime = st->min_vdisktime;
>> +	while (cfqe) {
>> +		if (!RB_EMPTY_NODE(&cfqe->rb_node))
>> +			return;
>>  
>> -	cfq_entity_service_tree_add(st, cfqe);
>> +		/*
>> +		 * Currently put the group at the end. Later implement
>> +		 * something so that groups get lesser vtime based on
>> +		 * their weights, so that if group does not loose all
>> +		 * if it was not continously backlogged.
>> +		 */
> 
> Can we use vdisktime boost logic for groups also? I think it can be a separate
> patch in the series (the last one). Keeping it as a separate patch will
> also help you to coordinate with chad's patch.
> 
>> +		st = cfqe->service_tree;
> 
> Group entity set their service tree when they get allocated and retain
> this pointer even when they get deleted from serivce tree. Queue entities
> seem to have it NULL when they get deleted from service tree and it
> gets set again when queue is getting inserted. It would be nice if we
> can fix this discrepancy and keep it consistent. I think clearing up
> cfqe->service_tree is a better idea and then calculate it again for
> group also.

Vivek,

Currently, cfq queue might change workload type and io class, so we need to recalculate
its service_tree. But for cfq groups, IMHO we don't need to add this complexity for the
time being.
I think we can add this change as soon as different io classes or workload types are
introduced. How do you think?

Thanks,
Gui