| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4e5e476b0912111001h3c0b9798u2a2b25c9fcc39504@mail.gmail.com>
Date: Fri, 11 Dec 2009 19:01:14 +0100
From: Corrado Zoccolo <czoccolo@...il.com>
To: Vivek Goyal <vgoyal@...hat.com>
Cc: Gui Jianfeng <guijianfeng@...fujitsu.com>,
Jens Axboe <jens.axboe@...cle.com>,
linux kernel mailing list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] cfq: Take whether cfq group is changed into account when
choosing service tree
Hi guys,
On Fri, Dec 11, 2009 at 4:07 PM, Vivek Goyal <vgoyal@...hat.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@...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@...r.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@...il.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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists