* [PATCH]cfq-iosched: give busy sync queue no dispatch limit
@ 2011-03-04 8:01 Shaohua Li
2011-03-04 9:14 ` Gui Jianfeng
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Shaohua Li @ 2011-03-04 8:01 UTC (permalink / raw)
To: Jens Axboe; +Cc: lkml, Vivek Goyal, Jeff Moyer, Corrado Zoccolo, Gui Jianfeng
If there are a sync and an async queue and the sync queue's think time is small,
we can ignore the sync queue's dispatch quantum. Because the sync queue will
always preempt the async queue, we don't need to care about async's latency.
This can fix a performance regression of aiostress test, which is introduced by
commit f8ae6e3eb825. The issue should exist even without the commit, but the
commit amplifies the impact.
The initial post does the same optimization for RT queue too, but since I have
no real workload for it, Vivek suggests to drop it.
Signed-off-by: Shaohua Li <shaohua.li@intel.com>
---
block/cfq-iosched.c | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
Index: linux/block/cfq-iosched.c
===================================================================
--- linux.orig/block/cfq-iosched.c 2011-03-04 09:50:22.000000000 +0800
+++ linux/block/cfq-iosched.c 2011-03-04 10:22:16.000000000 +0800
@@ -238,6 +238,7 @@ struct cfq_data {
struct rb_root prio_trees[CFQ_PRIO_LISTS];
unsigned int busy_queues;
+ unsigned int busy_sync_queues;
int rq_in_driver;
int rq_in_flight[2];
@@ -1372,6 +1373,8 @@ static void cfq_add_cfqq_rr(struct cfq_d
BUG_ON(cfq_cfqq_on_rr(cfqq));
cfq_mark_cfqq_on_rr(cfqq);
cfqd->busy_queues++;
+ if (cfq_cfqq_sync(cfqq))
+ cfqd->busy_sync_queues++;
cfq_resort_rr_list(cfqd, cfqq);
}
@@ -1398,6 +1401,8 @@ static void cfq_del_cfqq_rr(struct cfq_d
cfq_group_service_tree_del(cfqd, cfqq->cfqg);
BUG_ON(!cfqd->busy_queues);
cfqd->busy_queues--;
+ if (cfq_cfqq_sync(cfqq))
+ cfqd->busy_sync_queues--;
}
/*
@@ -2405,6 +2410,7 @@ static bool cfq_may_dispatch(struct cfq_
* Does this cfqq already have too much IO in flight?
*/
if (cfqq->dispatched >= max_dispatch) {
+ bool promote_sync = false;
/*
* idle queue must always only have a single IO in flight
*/
@@ -2412,15 +2418,31 @@ static bool cfq_may_dispatch(struct cfq_
return false;
/*
+ * If there is only one sync queue, and its think time is
+ * small, we can ignore async queue here and give the sync
+ * queue no dispatch limit. The reason is a sync queue can
+ * preempt async queue, limiting the sync queue doesn't make
+ * sense. This is useful for aiostress test.
+ */
+ if (cfq_cfqq_sync(cfqq) && cfqd->busy_sync_queues == 1) {
+ struct cfq_io_context *cic = RQ_CIC(cfqq->next_rq);
+
+ if (sample_valid(cic->ttime_samples) &&
+ cic->ttime_mean < cfqd->cfq_slice_idle)
+ promote_sync = true;
+ }
+
+ /*
* We have other queues, don't allow more IO from this one
*/
- if (cfqd->busy_queues > 1 && cfq_slice_used_soon(cfqd, cfqq))
+ if (cfqd->busy_queues > 1 && cfq_slice_used_soon(cfqd, cfqq) &&
+ !promote_sync)
return false;
/*
* Sole queue user, no limit
*/
- if (cfqd->busy_queues == 1)
+ if (cfqd->busy_queues == 1 || promote_sync)
max_dispatch = -1;
else
/*
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH]cfq-iosched: give busy sync queue no dispatch limit
2011-03-04 8:01 [PATCH]cfq-iosched: give busy sync queue no dispatch limit Shaohua Li
@ 2011-03-04 9:14 ` Gui Jianfeng
2011-03-04 16:40 ` Vivek Goyal
2011-03-07 8:26 ` Jens Axboe
2 siblings, 0 replies; 6+ messages in thread
From: Gui Jianfeng @ 2011-03-04 9:14 UTC (permalink / raw)
To: Shaohua Li; +Cc: Jens Axboe, lkml, Vivek Goyal, Jeff Moyer, Corrado Zoccolo
Shaohua Li wrote:
> If there are a sync and an async queue and the sync queue's think time is small,
> we can ignore the sync queue's dispatch quantum. Because the sync queue will
> always preempt the async queue, we don't need to care about async's latency.
> This can fix a performance regression of aiostress test, which is introduced by
> commit f8ae6e3eb825. The issue should exist even without the commit, but the
> commit amplifies the impact.
>
> The initial post does the same optimization for RT queue too, but since I have
> no real workload for it, Vivek suggests to drop it.
>
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
This fix looks good to me.
Reviewed-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com>
Thanks,
Gui
> ---
> block/cfq-iosched.c | 26 ++++++++++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> Index: linux/block/cfq-iosched.c
> ===================================================================
> --- linux.orig/block/cfq-iosched.c 2011-03-04 09:50:22.000000000 +0800
> +++ linux/block/cfq-iosched.c 2011-03-04 10:22:16.000000000 +0800
> @@ -238,6 +238,7 @@ struct cfq_data {
> struct rb_root prio_trees[CFQ_PRIO_LISTS];
>
> unsigned int busy_queues;
> + unsigned int busy_sync_queues;
>
> int rq_in_driver;
> int rq_in_flight[2];
> @@ -1372,6 +1373,8 @@ static void cfq_add_cfqq_rr(struct cfq_d
> BUG_ON(cfq_cfqq_on_rr(cfqq));
> cfq_mark_cfqq_on_rr(cfqq);
> cfqd->busy_queues++;
> + if (cfq_cfqq_sync(cfqq))
> + cfqd->busy_sync_queues++;
>
> cfq_resort_rr_list(cfqd, cfqq);
> }
> @@ -1398,6 +1401,8 @@ static void cfq_del_cfqq_rr(struct cfq_d
> cfq_group_service_tree_del(cfqd, cfqq->cfqg);
> BUG_ON(!cfqd->busy_queues);
> cfqd->busy_queues--;
> + if (cfq_cfqq_sync(cfqq))
> + cfqd->busy_sync_queues--;
> }
>
> /*
> @@ -2405,6 +2410,7 @@ static bool cfq_may_dispatch(struct cfq_
> * Does this cfqq already have too much IO in flight?
> */
> if (cfqq->dispatched >= max_dispatch) {
> + bool promote_sync = false;
> /*
> * idle queue must always only have a single IO in flight
> */
> @@ -2412,15 +2418,31 @@ static bool cfq_may_dispatch(struct cfq_
> return false;
>
> /*
> + * If there is only one sync queue, and its think time is
> + * small, we can ignore async queue here and give the sync
> + * queue no dispatch limit. The reason is a sync queue can
> + * preempt async queue, limiting the sync queue doesn't make
> + * sense. This is useful for aiostress test.
> + */
> + if (cfq_cfqq_sync(cfqq) && cfqd->busy_sync_queues == 1) {
> + struct cfq_io_context *cic = RQ_CIC(cfqq->next_rq);
> +
> + if (sample_valid(cic->ttime_samples) &&
> + cic->ttime_mean < cfqd->cfq_slice_idle)
> + promote_sync = true;
> + }
> +
> + /*
> * We have other queues, don't allow more IO from this one
> */
> - if (cfqd->busy_queues > 1 && cfq_slice_used_soon(cfqd, cfqq))
> + if (cfqd->busy_queues > 1 && cfq_slice_used_soon(cfqd, cfqq) &&
> + !promote_sync)
> return false;
>
> /*
> * Sole queue user, no limit
> */
> - if (cfqd->busy_queues == 1)
> + if (cfqd->busy_queues == 1 || promote_sync)
> max_dispatch = -1;
> else
> /*
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH]cfq-iosched: give busy sync queue no dispatch limit
2011-03-04 8:01 [PATCH]cfq-iosched: give busy sync queue no dispatch limit Shaohua Li
2011-03-04 9:14 ` Gui Jianfeng
@ 2011-03-04 16:40 ` Vivek Goyal
2011-03-07 1:23 ` Shaohua Li
2011-03-07 8:26 ` Jens Axboe
2 siblings, 1 reply; 6+ messages in thread
From: Vivek Goyal @ 2011-03-04 16:40 UTC (permalink / raw)
To: Shaohua Li; +Cc: Jens Axboe, lkml, Jeff Moyer, Corrado Zoccolo, Gui Jianfeng
On Fri, Mar 04, 2011 at 04:01:29PM +0800, Shaohua Li wrote:
[..]
> @@ -2412,15 +2418,31 @@ static bool cfq_may_dispatch(struct cfq_
> return false;
>
> /*
> + * If there is only one sync queue, and its think time is
> + * small, we can ignore async queue here and give the sync
> + * queue no dispatch limit. The reason is a sync queue can
> + * preempt async queue, limiting the sync queue doesn't make
> + * sense. This is useful for aiostress test.
> + */
> + if (cfq_cfqq_sync(cfqq) && cfqd->busy_sync_queues == 1) {
> + struct cfq_io_context *cic = RQ_CIC(cfqq->next_rq);
> +
> + if (sample_valid(cic->ttime_samples) &&
> + cic->ttime_mean < cfqd->cfq_slice_idle)
> + promote_sync = true;
> + }
What's the relation of think time here? Or why should we check for think
time being small. To me it does not make a difference in this case.
We have a request in existing queue and we figure out that this is the
only sync queue in the system to we let it dispatch more than quantum.
Thinktime should not even matter.
Thanks
Vivek
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH]cfq-iosched: give busy sync queue no dispatch limit
2011-03-04 16:40 ` Vivek Goyal
@ 2011-03-07 1:23 ` Shaohua Li
2011-03-07 14:35 ` Vivek Goyal
0 siblings, 1 reply; 6+ messages in thread
From: Shaohua Li @ 2011-03-07 1:23 UTC (permalink / raw)
To: Vivek Goyal; +Cc: Jens Axboe, lkml, Jeff Moyer, Corrado Zoccolo, Gui Jianfeng
On Sat, 2011-03-05 at 00:40 +0800, Vivek Goyal wrote:
> On Fri, Mar 04, 2011 at 04:01:29PM +0800, Shaohua Li wrote:
>
> [..]
> > @@ -2412,15 +2418,31 @@ static bool cfq_may_dispatch(struct cfq_
> > return false;
> >
> > /*
> > + * If there is only one sync queue, and its think time is
> > + * small, we can ignore async queue here and give the sync
> > + * queue no dispatch limit. The reason is a sync queue can
> > + * preempt async queue, limiting the sync queue doesn't make
> > + * sense. This is useful for aiostress test.
> > + */
> > + if (cfq_cfqq_sync(cfqq) && cfqd->busy_sync_queues == 1) {
> > + struct cfq_io_context *cic = RQ_CIC(cfqq->next_rq);
> > +
> > + if (sample_valid(cic->ttime_samples) &&
> > + cic->ttime_mean < cfqd->cfq_slice_idle)
> > + promote_sync = true;
> > + }
>
> What's the relation of think time here? Or why should we check for think
> time being small. To me it does not make a difference in this case.
>
> We have a request in existing queue and we figure out that this is the
> only sync queue in the system to we let it dispatch more than quantum.
> Thinktime should not even matter.
The reason in my mind is if think time is small, sync queue will keep
preempting async queue, so limit is meaningless. if think time is big,
there is less preempt. I'm afraid to go too far way in the less preempt
case and starve async too much.
Thanks,
Shaohua
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH]cfq-iosched: give busy sync queue no dispatch limit
2011-03-07 1:23 ` Shaohua Li
@ 2011-03-07 14:35 ` Vivek Goyal
0 siblings, 0 replies; 6+ messages in thread
From: Vivek Goyal @ 2011-03-07 14:35 UTC (permalink / raw)
To: Shaohua Li; +Cc: Jens Axboe, lkml, Jeff Moyer, Corrado Zoccolo, Gui Jianfeng
On Mon, Mar 07, 2011 at 09:23:37AM +0800, Shaohua Li wrote:
> On Sat, 2011-03-05 at 00:40 +0800, Vivek Goyal wrote:
> > On Fri, Mar 04, 2011 at 04:01:29PM +0800, Shaohua Li wrote:
> >
> > [..]
> > > @@ -2412,15 +2418,31 @@ static bool cfq_may_dispatch(struct cfq_
> > > return false;
> > >
> > > /*
> > > + * If there is only one sync queue, and its think time is
> > > + * small, we can ignore async queue here and give the sync
> > > + * queue no dispatch limit. The reason is a sync queue can
> > > + * preempt async queue, limiting the sync queue doesn't make
> > > + * sense. This is useful for aiostress test.
> > > + */
> > > + if (cfq_cfqq_sync(cfqq) && cfqd->busy_sync_queues == 1) {
> > > + struct cfq_io_context *cic = RQ_CIC(cfqq->next_rq);
> > > +
> > > + if (sample_valid(cic->ttime_samples) &&
> > > + cic->ttime_mean < cfqd->cfq_slice_idle)
> > > + promote_sync = true;
> > > + }
> >
> > What's the relation of think time here? Or why should we check for think
> > time being small. To me it does not make a difference in this case.
> >
> > We have a request in existing queue and we figure out that this is the
> > only sync queue in the system to we let it dispatch more than quantum.
> > Thinktime should not even matter.
> The reason in my mind is if think time is small, sync queue will keep
> preempting async queue, so limit is meaningless. if think time is big,
> there is less preempt. I'm afraid to go too far way in the less preempt
> case and starve async too much.
So only case left out is that a cfqq is driving deep queue depths but
think time is high?
sync queue has already preempted async and not we will be idling on it
and not allow async to dispatch.
If think time is high, anyway you will automatically reduce the preemption
of async queue.
IMHO, it does not make much sense to also check for think time and make
it complicated. We should get rid of it.
Thanks
Vivek
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH]cfq-iosched: give busy sync queue no dispatch limit
2011-03-04 8:01 [PATCH]cfq-iosched: give busy sync queue no dispatch limit Shaohua Li
2011-03-04 9:14 ` Gui Jianfeng
2011-03-04 16:40 ` Vivek Goyal
@ 2011-03-07 8:26 ` Jens Axboe
2 siblings, 0 replies; 6+ messages in thread
From: Jens Axboe @ 2011-03-07 8:26 UTC (permalink / raw)
To: Shaohua Li; +Cc: lkml, Vivek Goyal, Jeff Moyer, Corrado Zoccolo, Gui Jianfeng
On 2011-03-04 09:01, Shaohua Li wrote:
> If there are a sync and an async queue and the sync queue's think time is small,
> we can ignore the sync queue's dispatch quantum. Because the sync queue will
> always preempt the async queue, we don't need to care about async's latency.
> This can fix a performance regression of aiostress test, which is introduced by
> commit f8ae6e3eb825. The issue should exist even without the commit, but the
> commit amplifies the impact.
>
> The initial post does the same optimization for RT queue too, but since I have
> no real workload for it, Vivek suggests to drop it.
Thanks, applied.
--
Jens Axboe
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-03-07 14:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-04 8:01 [PATCH]cfq-iosched: give busy sync queue no dispatch limit Shaohua Li
2011-03-04 9:14 ` Gui Jianfeng
2011-03-04 16:40 ` Vivek Goyal
2011-03-07 1:23 ` Shaohua Li
2011-03-07 14:35 ` Vivek Goyal
2011-03-07 8:26 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox