2011-03-22 22:14:26

by Justin TerAvest

Subject: [PATCH] cfq-iosched: Don't clear queue stats when preempt.

Previous commit "cfq-iosched: Don't set active queue in preempt" wrongly
cleared stats for preempting queues when it shouldn't have, because when
we choose a queue to preempt, it still isn't necessarily scheduled next.

Thanks to Vivek Goyal for figuring this out and understanding how the
preemption code works.

Signed-off-by: Justin TerAvest <[email protected]>
---
block/cfq-iosched.c | 38 +++++++++++++++-----------------------
1 files changed, 15 insertions(+), 23 deletions(-)

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 69208d7..efa1f30 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -1620,33 +1620,27 @@ static inline void cfq_del_timer(struct cfq_data *cfqd, struct cfq_queue *cfqq)
cfq_blkiocg_update_idle_time_stats(&cfqq->cfqg->blkg);
}

-static void cfq_clear_queue_stats(struct cfq_data *cfqd,
- struct cfq_queue *cfqq)
-{
- cfq_blkiocg_update_avg_queue_size_stats(&cfqq->cfqg->blkg);
- cfqq->slice_start = 0;
- cfqq->dispatch_start = jiffies;
- cfqq->allocated_slice = 0;
- cfqq->slice_end = 0;
- cfqq->slice_dispatch = 0;
- cfqq->nr_sectors = 0;
-
- cfq_clear_cfqq_wait_request(cfqq);
- cfq_clear_cfqq_must_dispatch(cfqq);
- cfq_clear_cfqq_must_alloc_slice(cfqq);
- cfq_clear_cfqq_fifo_expire(cfqq);
- cfq_mark_cfqq_slice_new(cfqq);
-
- cfq_del_timer(cfqd, cfqq);
-}
-
static void __cfq_set_active_queue(struct cfq_data *cfqd,
struct cfq_queue *cfqq)
{
if (cfqq) {
cfq_log_cfqq(cfqd, cfqq, "set_active wl_prio:%d wl_type:%d",
cfqd->serving_prio, cfqd->serving_type);
- cfq_clear_queue_stats(cfqd, cfqq);
+ cfq_blkiocg_update_avg_queue_size_stats(&cfqq->cfqg->blkg);
+ cfqq->slice_start = 0;
+ cfqq->dispatch_start = jiffies;
+ cfqq->allocated_slice = 0;
+ cfqq->slice_end = 0;
+ cfqq->slice_dispatch = 0;
+ cfqq->nr_sectors = 0;
+
+ cfq_clear_cfqq_wait_request(cfqq);
+ cfq_clear_cfqq_must_dispatch(cfqq);
+ cfq_clear_cfqq_must_alloc_slice(cfqq);
+ cfq_clear_cfqq_fifo_expire(cfqq);
+ cfq_mark_cfqq_slice_new(cfqq);
+
+ cfq_del_timer(cfqd, cfqq);
}

cfqd->active_queue = cfqq;
@@ -3338,8 +3332,6 @@ static void cfq_preempt_queue(struct cfq_data *cfqd, struct cfq_queue *cfqq)
BUG_ON(!cfq_cfqq_on_rr(cfqq));

cfq_service_tree_add(cfqd, cfqq, 1);
-
- cfq_clear_queue_stats(cfqd, cfqq);
}

/*
--
1.7.3.1

2011-03-22 22:19:46

by Vivek Goyal

Subject: Re: [PATCH] cfq-iosched: Don't clear queue stats when preempt.

On Tue, Mar 22, 2011 at 03:14:12PM -0700, Justin TerAvest wrote:
> Previous commit "cfq-iosched: Don't set active queue in preempt" wrongly
> cleared stats for preempting queues when it shouldn't have, because when
> we choose a queue to preempt, it still isn't necessarily scheduled next.
>
> Thanks to Vivek Goyal for figuring this out and understanding how the
> preemption code works.
>
> Signed-off-by: Justin TerAvest <[email protected]>
> ---
> block/cfq-iosched.c | 38 +++++++++++++++-----------------------
> 1 files changed, 15 insertions(+), 23 deletions(-)
>
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index 69208d7..efa1f30 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -1620,33 +1620,27 @@ static inline void cfq_del_timer(struct cfq_data *cfqd, struct cfq_queue *cfqq)
> cfq_blkiocg_update_idle_time_stats(&cfqq->cfqg->blkg);
> }
>
> -static void cfq_clear_queue_stats(struct cfq_data *cfqd,
> - struct cfq_queue *cfqq)
> -{
> - cfq_blkiocg_update_avg_queue_size_stats(&cfqq->cfqg->blkg);
> - cfqq->slice_start = 0;
> - cfqq->dispatch_start = jiffies;
> - cfqq->allocated_slice = 0;
> - cfqq->slice_end = 0;
> - cfqq->slice_dispatch = 0;
> - cfqq->nr_sectors = 0;
> -
> - cfq_clear_cfqq_wait_request(cfqq);
> - cfq_clear_cfqq_must_dispatch(cfqq);
> - cfq_clear_cfqq_must_alloc_slice(cfqq);
> - cfq_clear_cfqq_fifo_expire(cfqq);
> - cfq_mark_cfqq_slice_new(cfqq);
> -
> - cfq_del_timer(cfqd, cfqq);
> -}
> -
> static void __cfq_set_active_queue(struct cfq_data *cfqd,
> struct cfq_queue *cfqq)
> {
> if (cfqq) {
> cfq_log_cfqq(cfqd, cfqq, "set_active wl_prio:%d wl_type:%d",
> cfqd->serving_prio, cfqd->serving_type);
> - cfq_clear_queue_stats(cfqd, cfqq);
> + cfq_blkiocg_update_avg_queue_size_stats(&cfqq->cfqg->blkg);
> + cfqq->slice_start = 0;
> + cfqq->dispatch_start = jiffies;
> + cfqq->allocated_slice = 0;
> + cfqq->slice_end = 0;
> + cfqq->slice_dispatch = 0;
> + cfqq->nr_sectors = 0;
> +
> + cfq_clear_cfqq_wait_request(cfqq);
> + cfq_clear_cfqq_must_dispatch(cfqq);
> + cfq_clear_cfqq_must_alloc_slice(cfqq);
> + cfq_clear_cfqq_fifo_expire(cfqq);
> + cfq_mark_cfqq_slice_new(cfqq);
> +
> + cfq_del_timer(cfqd, cfqq);
> }
>
> cfqd->active_queue = cfqq;
> @@ -3338,8 +3332,6 @@ static void cfq_preempt_queue(struct cfq_data *cfqd, struct cfq_queue *cfqq)
> BUG_ON(!cfq_cfqq_on_rr(cfqq));
>
> cfq_service_tree_add(cfqd, cfqq, 1);
> -
> - cfq_clear_queue_stats(cfqd, cfqq);
> }

Can you restore the old following two lines.

cfqq->slice_end = 0;
cfq_mark_cfqq_slice_new(cfqq);

It is just to preserve the old behavior. Thought off the top of my head I
am not sure why these are there. So until somebody has studied it and
figured it out whether these are required or not, let these be there.

Thanks
Vivek