linux-kernel - Re: [RFC V2 PATCH 3/5] cfq-iosched: reimplement priorities using different service trees

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4e5e476b0910210937y71d2018en2b8b4a23c4bb1aa1@mail.gmail.com>
Date:	Wed, 21 Oct 2009 18:37:41 +0200
From:	Corrado Zoccolo <czoccolo@...il.com>
To:	Jeff Moyer <jmoyer@...hat.com>
Cc:	Linux-Kernel <linux-kernel@...r.kernel.org>,
	Jens Axboe <jens.axboe@...cle.com>
Subject: Re: [RFC V2 PATCH 3/5] cfq-iosched: reimplement priorities using 
	different service trees

On Wed, Oct 21, 2009 at 5:54 PM, Jeff Moyer <jmoyer@...hat.com> wrote:
> Corrado Zoccolo <czoccolo@...il.com> writes:
>
>> We use different service trees for different priority classes.
>> This allows a simplification in the service tree insertion code, that no
>> longer has to consider priority while walking the tree.
>
> This is kind of funny, considering things used to be divied up into
> lists by class and priority.
>
>> + * Index in the service_trees.
>> + * IDLE is handled separately, so it has negative index
>> + */
>> +enum wl_prio_t {
>> +     IDLE_WORKLOAD = -1,
>> +     BE_WORKLOAD = 0,
>> +     RT_WORKLOAD = 1
>> +};
>
> What's wrong with IOPRIO_CLASS_(RT|BE|IDLE)?  Why invent another enum?
Because I want to index inside my internal structures, and I have no
control over the former ones.
>
>> +
>> +/*
>>   * Per block device queue structure
>>   */
>>  struct cfq_data {
> [...]
>> +     struct cfq_rb_root service_trees[2];
>> +     struct cfq_rb_root service_tree_idle;
>
> Why separate out the idle service tree from the others?
>
In a subsequent patch, I will transform the first [2] in a [2][3],
while the idle tree will remain unchanged.

>> +static struct cfq_rb_root *service_tree_for(enum wl_prio_t prio,
>> +                                         struct cfq_data *cfqd)
>> +{
>> +     if (prio == IDLE_WORKLOAD)
>> +             return &cfqd->service_tree_idle;
>> +
>> +     return &cfqd->service_trees[prio];
>> +}
>
> This should just turn into cfqd->service_trees[IOPRIO_CLASS_*] in the
> callers.
I need the special treatment for idle in next patches, so I had chosen
the different approach.

>
> [...]
>
>>  /*
>> @@ -1106,6 +1134,10 @@ static struct cfq_queue *cfq_close_cooperator(struct cfq_data *cfqd,
>>       if (cfq_cfqq_coop(cfqq))
>>               return NULL;
>>
>> +     /* we don't want to mix processes with different characteristics */
>> +     if (cfqq->service_tree != cur_cfqq->service_tree)
>> +             return NULL;
>> +
>
> Hmm, that looks like a current bug in the close cooperator code.  It
> shouldn't allow cooperation between differring scheduling classes.

Maybe. Anyway, this check for me has more impact, when the service
trees are subsequently splitted in SYNC, SYNC_NOIDLE, and ASYNC. I
don't want close cooperation between them as well.

Corrado

>
> Cheers,
> Jeff
>



-- 
__________________________________________________________________________

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/