Messages in this thread Patch in this message | | | Date | Fri, 11 Dec 2009 19:01:14 +0100 | Subject | Re: [PATCH] cfq: Take whether cfq group is changed into account when choosing service tree | From | Corrado Zoccolo <> |
| |
Hi guys, On Fri, Dec 11, 2009 at 4:07 PM, Vivek Goyal <vgoyal@redhat.com> wrote: > 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.
I understand it, and in fact I was thinking about this. The idea is the same as with priorities. If we have not serviced a group for a while, we want to start with the no-idle workload to reduce its latency.
Unfortunately, a group may have a too small share, that could cause some workloads to be starved, as Vivek correctly points out, and we should avoid this. It should be easily reproducible testing a "many groups with mixed workloads" scenario with group_isolation=1.
Moreover, even if the approach is groups on top, when group isolation is not set (i.e. the default), in the non-root groups you will only have the sync-idle queues, so it is much more similar (logically) to a workload on top than it appears from the code.
I think the net result of this patch is, when group isolation is not set, to select no-idle workload first only when entering the root group, thus a slight penalization of the async workload. Gui, if you observed improvements with this patch, probably you can obtain them without the starvation drawback by making it conditional to !group_isolation.
BTW, since you always use cfqg_changed and prio_changed OR-ed together, you can have just one argument, called e.g. boost_no_idle, and you pass (!group_isolation && cfqg_changed) || prio_changed.
> 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.
Agreed.
Thanks, Corrado
> > 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 >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ >
-- __________________________________________________________________________
dott. Corrado Zoccolo mailto:czoccolo@gmail.com PhD - Department of Computer Science - University of Pisa, Italy -------------------------------------------------------------------------- The self-confidence of a warrior is not the self-confidence of the average man. The average man seeks certainty in the eyes of the onlooker and calls that self-confidence. The warrior seeks impeccability in his own eyes and calls that humbleness. Tales of Power - C. Castaneda -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |