Re: [RFC V2 PATCH 1/5] cfq-iosched: adapt slice to number of processes doing I/O

[Corrado Zoccolo <czoccolo@gmail.com>]

Hi Jeff,
On Wed, Oct 21, 2009 at 5:57 PM, Jeff Moyer <jmoyer@redhat.com> wrote:
> Hi, Corrado!
>
> Sorry if folks receive this twice, but my mailer and I had an argument
> about this message.  ;-)
>
> Corrado Zoccolo <czoccolo@gmail.com> writes:
>
>> When the number of processes performing I/O concurrently increases,
>> a fixed time slice per process will cause large latencies.
>>
>> This patch, if low_latency mode is enabled,  will scale the time slice
>> assigned to each process according to a 300ms target latency.
>>
>> In order to keep fairness among processes:
>> * The number of active processes is computed using a special form of
>> running average, that quickly follows sudden increases (to keep latency low),
>> and decrease slowly (to have fairness in spite of rapid decreases of this
>> value).
>>
>> To safeguard sequential bandwidth, we impose a minimum time slice
>> (computed using 2*cfq_slice_idle as base, adjusted according to priority
>> and async-ness).
>
> I like the idea as well, but I have a question and some nits to pick.
>
>>  static inline void
>>  cfq_set_prio_slice(struct cfq_data *cfqd, struct cfq_queue *cfqq)
>>  {
>> -     cfqq->slice_end = cfq_prio_to_slice(cfqd, cfqq) + jiffies;
>> +     unsigned slice = cfq_prio_to_slice(cfqd, cfqq);
>> +     if (cfqd->cfq_latency) {
>> +             unsigned iq = cfq_get_avg_queues(cfqd, cfq_class_rt(cfqq));
>> +             unsigned process_thr = cfq_target_latency / cfqd->cfq_slice[1];
>> +             if (iq > process_thr) {
>> +                     unsigned low_slice = 2 * slice * cfqd->cfq_slice_idle
>> +                             / cfqd->cfq_slice[1];
>> +                     slice = max(slice * cfq_target_latency /
>> +                             (cfqd->cfq_slice[1] * iq),
>
> Couldn't you have just divided the slice by iq?  And why iq?  Why not
> nr_qs or avg_qlen or something?  It's a minor nit; I can live with it.

iq stands for interested queues, because we are restricting the count
just to the same priority class, not all queues in the system.

>
>> +                             min(slice, low_slice));
>> +             }
>> +     }
>> +     cfqq->slice_end = jiffies + slice;
>>       cfq_log_cfqq(cfqd, cfqq, "set_slice=%lu", cfqq->slice_end - jiffies);
>
> Wow.  That function is *dense*.  I tried to write it in a more
> readable fashion, but please chime in if I misinterpreted anything.
>
> static inline void
> cfq_set_prio_slice(struct cfq_data *cfqd, struct cfq_queue *cfqq)
> {
>        unsigned slice = cfq_prio_to_slice(cfqd, cfqq);
>
>        if (cfqd->cfq_latency) {
>                unsigned iq = cfq_get_avg_queues(cfqd, cfq_class_rt(cfqq));
>                unsigned slice_sync = cfqd->cfq_slice[1];
>                unsigned process_thr = cfq_target_latency / slice_sync;
>
>                if (iq > process_thr) {
>                        /*
>                         * Minimum slice is computed using 2*slice_idle as
>                         * a base, and then scaling it by priority and
>                         * async-ness.
>                         */
>                        unsigned total_sync = slice_sync * iq;
>                        unsigned slice_fraction = cfq_target_latency / total_sync;
>                        unsigned min_slice = (2 * cfqd->cfq_slice_idle) *
>                                (slice / slice_sync);
>                        min_slice = min(slice, min_slice);
>                        slice *= slice_fraction;
>                        slice = max(slice, min_slice);
>                }
>        }
>        cfqq->slice_end = jiffies + slice;
>        cfq_log_cfqq(cfqd, cfqq, "set_slice=%lu", cfqq->slice_end - jiffies);
> }
>
I don't think this is equivalent. You seem to compute some divisions
too early, losing in precision.
slice * cfq_target_latency / (cfqd->cfq_slice[1] * iq)
is not generally equivalent to:
slice * (cfq_target_latency / (cfqd->cfq_slice[1] * iq))
that is what you are computing.
There is an other such case in your simplification.

Corrado

>
> Cheers,
> Jeff
>