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).
Signed-off-by: Corrado Zoccolo <[email protected]>
---
block/cfq-iosched.c | 41 +++++++++++++++++++++++++++++++++++++----
1 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 069a610..04d3b66 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -27,6 +27,8 @@ static const int cfq_slice_sync = HZ / 10;
static int cfq_slice_async = HZ / 25;
static const int cfq_slice_async_rq = 2;
static int cfq_slice_idle = HZ / 125;
+static const int cfq_target_latency = HZ * 3/10; /* 300 ms */
+static const int cfq_hist_divisor = 4;
/*
* offset from end of service tree
@@ -134,6 +136,8 @@ struct cfq_data {
struct rb_root prio_trees[CFQ_PRIO_LISTS];
unsigned int busy_queues;
+ unsigned int busy_rt_queues;
+ unsigned int busy_queues_avg[2];
int rq_in_driver[2];
int sync_flight;
@@ -301,10 +305,37 @@ cfq_prio_to_slice(struct cfq_data *cfqd, struct cfq_queue *cfqq)
return cfq_prio_slice(cfqd, cfq_cfqq_sync(cfqq), cfqq->ioprio);
}
+static inline unsigned
+cfq_get_avg_queues(struct cfq_data *cfqd, bool rt) {
+ unsigned min_q, max_q;
+ unsigned mult = cfq_hist_divisor - 1;
+ unsigned round = cfq_hist_divisor / 2;
+ unsigned busy = rt ? cfqd->busy_rt_queues :
+ (cfqd->busy_queues - cfqd->busy_rt_queues);
+ min_q = min(cfqd->busy_queues_avg[rt], busy);
+ max_q = max(cfqd->busy_queues_avg[rt], busy);
+ cfqd->busy_queues_avg[rt] = (mult * max_q + min_q + round) /
+ cfq_hist_divisor;
+ return cfqd->busy_queues_avg[rt];
+}
+
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 = cfqd->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),
+ min(slice, low_slice));
+ }
+ }
+ cfqq->slice_end = jiffies + slice;
cfq_log_cfqq(cfqd, cfqq, "set_slice=%lu", cfqq->slice_end - jiffies);
}
@@ -655,7 +686,8 @@ static void cfq_add_cfqq_rr(struct cfq_data *cfqd, struct cfq_queue *cfqq)
BUG_ON(cfq_cfqq_on_rr(cfqq));
cfq_mark_cfqq_on_rr(cfqq);
cfqd->busy_queues++;
-
+ if (cfq_class_rt(cfqq))
+ cfqd->busy_rt_queues++;
cfq_resort_rr_list(cfqd, cfqq);
}
@@ -678,6 +710,8 @@ static void cfq_del_cfqq_rr(struct cfq_data *cfqd, struct cfq_queue *cfqq)
BUG_ON(!cfqd->busy_queues);
cfqd->busy_queues--;
+ if (cfq_class_rt(cfqq))
+ cfqd->busy_rt_queues--;
}
/*
@@ -2152,10 +2186,9 @@ static void cfq_insert_request(struct request_queue *q, struct request *rq)
cfq_log_cfqq(cfqd, cfqq, "insert_request");
cfq_init_prio_data(cfqq, RQ_CIC(rq)->ioc);
- cfq_add_rq_rb(rq);
-
rq_set_fifo_time(rq, jiffies + cfqd->cfq_fifo_expire[rq_is_sync(rq)]);
list_add_tail(&rq->queuelist, &cfqq->fifo);
+ cfq_add_rq_rb(rq);
cfq_rq_enqueued(cfqd, cfqq, rq);
}
--
1.6.2.5
On Mon, Oct 19 2009, Corrado Zoccolo wrote:
> 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).
Generally, this looks good. Just one minor style nit:
> +static inline unsigned
> +cfq_get_avg_queues(struct cfq_data *cfqd, bool rt) {
> + unsigned min_q, max_q;
> + unsigned mult = cfq_hist_divisor - 1;
> + unsigned round = cfq_hist_divisor / 2;
> + unsigned busy = rt ? cfqd->busy_rt_queues :
> + (cfqd->busy_queues - cfqd->busy_rt_queues);
> + min_q = min(cfqd->busy_queues_avg[rt], busy);
> + max_q = max(cfqd->busy_queues_avg[rt], busy);
> + cfqd->busy_queues_avg[rt] = (mult * max_q + min_q + round) /
> + cfq_hist_divisor;
> + return cfqd->busy_queues_avg[rt];
> +}
A lot of your code suffers from the specific problem of being largely
unreadable. To me, as the maintainer of that code, that is a maintenance
issue. I already asked you to get rid of the ?: constructs for earlier
patches, this series even takes it to the extreme of doing nested ?:
clauses. Don't do it! It's unreadable.
> @@ -2152,10 +2186,9 @@ static void cfq_insert_request(struct request_queue *q, struct request *rq)
> cfq_log_cfqq(cfqd, cfqq, "insert_request");
> cfq_init_prio_data(cfqq, RQ_CIC(rq)->ioc);
>
> - cfq_add_rq_rb(rq);
> -
> rq_set_fifo_time(rq, jiffies + cfqd->cfq_fifo_expire[rq_is_sync(rq)]);
> list_add_tail(&rq->queuelist, &cfqq->fifo);
> + cfq_add_rq_rb(rq);
>
> cfq_rq_enqueued(cfqd, cfqq, rq);
If the fifo vs service tree ordering is now important, you should
comment on why.
--
Jens Axboe
On Tue, Oct 20, 2009 at 2:54 AM, Jens Axboe <[email protected]> wrote:
> On Mon, Oct 19 2009, Corrado Zoccolo wrote:
>> 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).
>
> Generally, this looks good. Just one minor style nit:
>
>> +static inline unsigned
>> +cfq_get_avg_queues(struct cfq_data *cfqd, bool rt) {
>> + unsigned min_q, max_q;
>> + unsigned mult = cfq_hist_divisor - 1;
>> + unsigned round = cfq_hist_divisor / 2;
>> + unsigned busy = rt ? cfqd->busy_rt_queues :
>> + (cfqd->busy_queues - cfqd->busy_rt_queues);
>> + min_q = min(cfqd->busy_queues_avg[rt], busy);
>> + max_q = max(cfqd->busy_queues_avg[rt], busy);
>> + cfqd->busy_queues_avg[rt] = (mult * max_q + min_q + round) /
>> + cfq_hist_divisor;
>> + return cfqd->busy_queues_avg[rt];
>> +}
>
> A lot of your code suffers from the specific problem of being largely
> unreadable. To me, as the maintainer of that code, that is a maintenance
> issue. I already asked you to get rid of the ?: constructs for earlier
> patches, this series even takes it to the extreme of doing nested ?:
> clauses. Don't do it! It's unreadable.
Ok. I'll resubmit a revised version of the patches that address this
stile issue, as well as your concern with too large functions and
lacking comments.
I didn't realize that you hated ?: so much :).
To me, it seems a good way to achieve a different readability goal,
i.e. define the value of a variable in a single place, instead of
scattering it around on multiple lines.
>
>> @@ -2152,10 +2186,9 @@ static void cfq_insert_request(struct request_queue *q, struct request *rq)
>> cfq_log_cfqq(cfqd, cfqq, "insert_request");
>> cfq_init_prio_data(cfqq, RQ_CIC(rq)->ioc);
>>
>> - cfq_add_rq_rb(rq);
>> -
>> rq_set_fifo_time(rq, jiffies + cfqd->cfq_fifo_expire[rq_is_sync(rq)]);
>> list_add_tail(&rq->queuelist, &cfqq->fifo);
>> + cfq_add_rq_rb(rq);
>>
>> cfq_rq_enqueued(cfqd, cfqq, rq);
>
> If the fifo vs service tree ordering is now important, you should
> comment on why.
It's not important for the patches per se, but I found odd (and it
caused me some headache while debugging) that in cfq_add_rq_rb the
fifo was still empty.
In the new form, the rq will be complete when added, while in the
previous, it still had some empty fields.
Corrado
>
> --
> Jens Axboe
>
>
--
__________________________________________________________________________
dott. Corrado Zoccolo mailto:[email protected]
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
On Tue, Oct 20 2009, Corrado Zoccolo wrote:
> On Tue, Oct 20, 2009 at 2:54 AM, Jens Axboe <[email protected]> wrote:
> > On Mon, Oct 19 2009, Corrado Zoccolo wrote:
> >> 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).
> >
> > Generally, this looks good. Just one minor style nit:
> >
> >> +static inline unsigned
> >> +cfq_get_avg_queues(struct cfq_data *cfqd, bool rt) {
> >> + ? ? unsigned min_q, max_q;
> >> + ? ? unsigned mult ?= cfq_hist_divisor - 1;
> >> + ? ? unsigned round = cfq_hist_divisor / 2;
> >> + ? ? unsigned busy ?= rt ? cfqd->busy_rt_queues :
> >> + ? ? ? ? ? ? (cfqd->busy_queues - cfqd->busy_rt_queues);
> >> + ? ? min_q = min(cfqd->busy_queues_avg[rt], busy);
> >> + ? ? max_q = max(cfqd->busy_queues_avg[rt], busy);
> >> + ? ? cfqd->busy_queues_avg[rt] = (mult * max_q + min_q + round) /
> >> + ? ? ? ? ? ? cfq_hist_divisor;
> >> + ? ? return cfqd->busy_queues_avg[rt];
> >> +}
> >
> > A lot of your code suffers from the specific problem of being largely
> > unreadable. To me, as the maintainer of that code, that is a maintenance
> > issue. I already asked you to get rid of the ?: constructs for earlier
> > patches, this series even takes it to the extreme of doing nested ?:
> > clauses. Don't do it! It's unreadable.
>
> Ok. I'll resubmit a revised version of the patches that address this
> stile issue, as well as your concern with too large functions and
> lacking comments.
> I didn't realize that you hated ?: so much :).
I do, personally it doesn't read anywhere near as naturally as a simple
'if'. And when you start doing x = a ? b : c ? d : e; I almost reach for
the nearest expletive :-)
And adding a local scope with {} and having 3-4 broken lines of
multiplications, divisions (etc) inside max()/min() calls doesn't add to
the readability in any positive way...
> To me, it seems a good way to achieve a different readability goal,
> i.e. define the value of a variable in a single place, instead of
> scattering it around on multiple lines.
I prefer putting it elsewhere instead. So instead of doing:
foo_type bar = x(y) == BAZ ? a : b;
you have
get_foo_type(y)
{
if (x(y) == BAZ)
return a;
return b;
}
foo_type bar = get_foo_type(y);
which is a lot more readable to me. Especially since you have to do the
get_foo_type() operation in a lot of places.
> >> @@ -2152,10 +2186,9 @@ static void cfq_insert_request(struct request_queue *q, struct request *rq)
> >> ? ? ? cfq_log_cfqq(cfqd, cfqq, "insert_request");
> >> ? ? ? cfq_init_prio_data(cfqq, RQ_CIC(rq)->ioc);
> >>
> >> - ? ? cfq_add_rq_rb(rq);
> >> -
> >> ? ? ? rq_set_fifo_time(rq, jiffies + cfqd->cfq_fifo_expire[rq_is_sync(rq)]);
> >> ? ? ? list_add_tail(&rq->queuelist, &cfqq->fifo);
> >> + ? ? cfq_add_rq_rb(rq);
> >>
> >> ? ? ? cfq_rq_enqueued(cfqd, cfqq, rq);
> >
> > If the fifo vs service tree ordering is now important, you should
> > comment on why.
> It's not important for the patches per se, but I found odd (and it
> caused me some headache while debugging) that in cfq_add_rq_rb the
> fifo was still empty.
> In the new form, the rq will be complete when added, while in the
> previous, it still had some empty fields.
Then keep it like it is, or do it as a separate patch. When you include
it in a functionally changing patch like this, I'm assuming there must
be a reason for that. And when it seems like there isn't, you wonder
what is up.
--
Jens Axboe
On Tue, Oct 20, 2009 at 3:17 PM, Jens Axboe <[email protected]> wrote:
> I do, personally it doesn't read anywhere near as naturally as a simple
> 'if'. And when you start doing x = a ? b : c ? d : e; I almost reach for
> the nearest expletive :-)
In this case, formatting it on multiple lines could help:
a ? b :
c ? d :
e;
>
> And adding a local scope with {} and having 3-4 broken lines of
> multiplications, divisions (etc) inside max()/min() calls doesn't add to
> the readability in any positive way...
Ok, but min and max hide a lot of complexity for the corner cases.
Espressing those with plain ifs would be really a tough task.
>
>> To me, it seems a good way to achieve a different readability goal,
>> i.e. define the value of a variable in a single place, instead of
>> scattering it around on multiple lines.
>
> I prefer putting it elsewhere instead. So instead of doing:
>
> foo_type bar = x(y) == BAZ ? a : b;
>
> you have
>
> get_foo_type(y)
> {
> if (x(y) == BAZ)
> return a;
>
> return b;
> }
>
> foo_type bar = get_foo_type(y);
>
> which is a lot more readable to me. Especially since you have to do the
> get_foo_type() operation in a lot of places.
Sounds good. I'll do in this way in next version.
>
>> It's not important for the patches per se, but I found odd (and it
>> caused me some headache while debugging) that in cfq_add_rq_rb the
>> fifo was still empty.
>> In the new form, the rq will be complete when added, while in the
>> previous, it still had some empty fields.
>
> Then keep it like it is, or do it as a separate patch. When you include
> it in a functionally changing patch like this, I'm assuming there must
> be a reason for that. And when it seems like there isn't, you wonder
> what is up.
I'm moving it in the preparation patch, that is not changing
functionality. There,. it makes more sense.
I'm going to send the revised series soon.
Corrado
>
> --
> Jens Axboe
>
>
--
__________________________________________________________________________
dott. Corrado Zoccolo mailto:[email protected]
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