Re: [PATCH] cfq: Take whether cfq group is changed into account when choosing service tree

[Vivek Goyal <vgoyal@redhat.com>]

On Fri, Dec 11, 2009 at 01:02:10PM +0800, Gui Jianfeng wrote:
> Currently, with IO Controller introduced, CFQ chooses cfq group
> at the top, and then choose service tree. So we need to take 
> whether cfq group is changed into account to decide whether we 
> should choose service tree start from scratch.
> 

I am not able to understand the need/purpose of this patch. Once we
switched the group during scheduling, why should we reset the order
of workload with-in group. In fact we don't want to do that and we
want to start from where we left so that no workload with-in group is
starved.

When a group is resumed, choose_service_tree() always checks if any RT
queue got backlogged or not. If yes, it does choose that first.

Can you give more details why do you want to do this change?

Thanks
Vivek

> Signed-off-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com>
> ---
>  block/cfq-iosched.c |   27 ++++++++++++++++++---------
>  1 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index f3f6239..16084ca 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -1964,7 +1964,7 @@ static void cfq_setup_merge(struct cfq_queue *cfqq, struct cfq_queue *new_cfqq)
>  
>  static enum wl_type_t cfq_choose_wl(struct cfq_data *cfqd,
>  				struct cfq_group *cfqg, enum wl_prio_t prio,
> -				bool prio_changed)
> +				bool cfqg_changed, bool prio_changed)
>  {
>  	struct cfq_queue *queue;
>  	int i;
> @@ -1972,7 +1972,7 @@ static enum wl_type_t cfq_choose_wl(struct cfq_data *cfqd,
>  	unsigned long lowest_key = 0;
>  	enum wl_type_t cur_best = SYNC_NOIDLE_WORKLOAD;
>  
> -	if (prio_changed) {
> +	if (cfqg_changed || prio_changed) {
>  		/*
>  		 * When priorities switched, we prefer starting
>  		 * from SYNC_NOIDLE (first choice), or just SYNC
> @@ -2001,7 +2001,8 @@ static enum wl_type_t cfq_choose_wl(struct cfq_data *cfqd,
>  	return cur_best;
>  }
>  
> -static void choose_service_tree(struct cfq_data *cfqd, struct cfq_group *cfqg)
> +static void choose_service_tree(struct cfq_data *cfqd, 
> +				struct cfq_group *cfqg, bool cfqg_changed)
>  {
>  	enum wl_prio_t previous_prio = cfqd->serving_prio;
>  	bool prio_changed;
> @@ -2033,21 +2034,22 @@ static void choose_service_tree(struct cfq_data *cfqd, struct cfq_group *cfqg)
>  	 * expiration time
>  	 */
>  	prio_changed = (cfqd->serving_prio != previous_prio);
> -	st = service_tree_for(cfqg, cfqd->serving_prio, cfqd->serving_type,
> -				cfqd);
> +	st = service_tree_for(cfqg, cfqd->serving_prio, cfqd->serving_type, 
> +			      cfqd);
>  	count = st->count;
>  
>  	/*
>  	 * If priority didn't change, check workload expiration,
>  	 * and that we still have other queues ready
>  	 */
> -	if (!prio_changed && count &&
> +	if (!cfqg_changed && !prio_changed && count &&
>  	    !time_after(jiffies, cfqd->workload_expires))
>  		return;
>  
>  	/* otherwise select new workload type */
>  	cfqd->serving_type =
> -		cfq_choose_wl(cfqd, cfqg, cfqd->serving_prio, prio_changed);
> +		cfq_choose_wl(cfqd, cfqg, cfqd->serving_prio, 
> +			      cfqg_changed, prio_changed);
>  	st = service_tree_for(cfqg, cfqd->serving_prio, cfqd->serving_type,
>  				cfqd);
>  	count = st->count;
> @@ -2104,9 +2106,16 @@ static struct cfq_group *cfq_get_next_cfqg(struct cfq_data *cfqd)
>  
>  static void cfq_choose_cfqg(struct cfq_data *cfqd)
>  {
> +	bool cfqg_changed = false;
> +
> +	struct cfq_group *orig_cfqg = cfqd->serving_group;
> +
>  	struct cfq_group *cfqg = cfq_get_next_cfqg(cfqd);
>  
> -	cfqd->serving_group = cfqg;
> +	if (orig_cfqg != cfqg) {
> +		cfqg_changed = 1;
> +		cfqd->serving_group = cfqg;
> +	}
>  
>  	/* Restore the workload type data */
>  	if (cfqg->saved_workload_slice) {
> @@ -2114,7 +2123,7 @@ static void cfq_choose_cfqg(struct cfq_data *cfqd)
>  		cfqd->serving_type = cfqg->saved_workload;
>  		cfqd->serving_prio = cfqg->saved_serving_prio;
>  	}
> -	choose_service_tree(cfqd, cfqg);
> +	choose_service_tree(cfqd, cfqg, cfqg_changed);
>  }
>  
>  /*
> -- 
> 1.5.4.rc3
>