Re: [PATCH 4/4] cfq-iosched: fix corner cases in idling logic

From: Corrado Zoccolo
Date: Wed Dec 02 2009 - 09:14:35 EST


Hi Jeff,
On Wed, Dec 2, 2009 at 2:42 PM, Jeff Moyer <jmoyer@xxxxxxxxxx> wrote:
> Corrado Zoccolo <czoccolo@xxxxxxxxx> writes:
>
>> Idling logic was disabled in some corner cases, leading to unfair share
>> for noidle queues.
>> * the idle timer was not armed if there were other requests in the
>> Â driver. unfortunately, those requests could come from other workloads,
>> Â or queues for which we don't enable idling. So we will check only
>> Â pending requests from the active queue
>> * rq_noidle check on no-idle queue could disable the end of tree idle if
>> Â the last completed request was rq_noidle. Now, we will disable that
>> Â idle only if all the queues served in the no-idle tree had rq_noidle
>> Â requests.
>>
>> Reported-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
>> Signed-off-by: Corrado Zoccolo <czoccolo@xxxxxxxxx>
>
>> @@ -2606,17 +2608,27 @@ static void cfq_completed_request(struct request_queue *q, struct request *rq)
>> Â Â Â Â Â Â Â Â Â Â Â cfq_clear_cfqq_slice_new(cfqq);
>> Â Â Â Â Â Â Â }
>> Â Â Â Â Â Â Â /*
>> - Â Â Â Â Â Â Â* If there are no requests waiting in this queue, and
>> - Â Â Â Â Â Â Â* there are other queues ready to issue requests, AND
>> - Â Â Â Â Â Â Â* those other queues are issuing requests within our
>> - Â Â Â Â Â Â Â* mean seek distance, give them a chance to run instead
>> - Â Â Â Â Â Â Â* of idling.
>> + Â Â Â Â Â Â Â* Idling is not enabled on:
>> + Â Â Â Â Â Â Â* - expired queues
>> + Â Â Â Â Â Â Â* - idle-priority queues
>> + Â Â Â Â Â Â Â* - async queues
>> + Â Â Â Â Â Â Â* - queues with still some requests queued
>> + Â Â Â Â Â Â Â* - when there is a close cooperator
>> Â Â Â Â Â Â Â Â*/
>
> I'm not sure this logic is correct. ÂIs this for the 2.6.33 branch?
Yes.
>ÂIf so, the coop flag now means that multiple processes share the same
> cfqq. ÂAre you sure this is the right thing to do for close cooperators?
I'm not sure. I didn't change the logic for close cooperators:
- else if (cfqq_empty && !cfq_close_cooperator(cfqd, cfqq) &&
- sync && !rq_noidle(rq))
- cfq_arm_slice_timer(cfqd);
+ else if (sync && cfqq_empty &&
+ !cfq_close_cooperator(cfqd, cfqq)) {
+ cfqd->noidle_tree_requires_idle |= !rq_noidle(rq);

I changed the rq_noidle part, and rewrote the comment to be aligned
with the code.
So I don't mind if you improve (or just remove) the close cooperator part.
Probably, you should do a test where close cooperating processes are competing
with a sequential reader, to see the effect of idling or not on them.

Thanks
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/