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

[Corrado Zoccolo]

On Wed, Oct 21, 2009 at 5:54 PM, Jeff Moyer <jmoyer@xxxxxxxxxx> wrote:
> Corrado Zoccolo <czoccolo@xxxxxxxxx> 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@xxxxxxxxx
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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/