Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | Subject | Re: [patch 2/3]cfq-iosched: schedule dispatch for noidle queue | From | Shaohua Li <> | Date | Tue, 09 Nov 2010 10:28:36 +0800 |
| |
On Tue, 2010-11-09 at 10:15 +0800, Vivek Goyal wrote: > On Tue, Nov 09, 2010 at 09:31:07AM +0800, Shaohua Li wrote: > > On Mon, 2010-11-08 at 22:28 +0800, Vivek Goyal wrote: > > > On Mon, Nov 08, 2010 at 10:07:18AM +0800, Shaohua Li wrote: > > > > A queue is idle at cfq_dispatch_requests(), but it gets noidle later. Unless > > > > other task explictly does unplug or all requests are drained, we will not > > > > deliever requests to the disk even cfq_arm_slice_timer doesn't make the > > > > queue idle. For example, cfq_should_idle() returns true because of > > > > service_tree->count == 1, and then other queues are added. Note, I didn't > > > > see obvious performance impacts so far with the patch, but just thought > > > > this could be a problem. > > > > > > > > Signed-off-by: Shaohua Li <shaohua.li@intel.com> > > > > > > > > --- > > > > block/cfq-iosched.c | 23 ++++++++++++++++++++++- > > > > 1 file changed, 22 insertions(+), 1 deletion(-) > > > > > > > > Index: linux/block/cfq-iosched.c > > > > =================================================================== > > > > --- linux.orig/block/cfq-iosched.c 2010-11-08 08:41:20.000000000 +0800 > > > > +++ linux/block/cfq-iosched.c 2010-11-08 08:43:51.000000000 +0800 > > > > @@ -3265,6 +3265,10 @@ cfq_should_preempt(struct cfq_data *cfqd > > > > if (cfq_class_rt(new_cfqq) && !cfq_class_rt(cfqq)) > > > > return true; > > > > > > > > + /* An idle queue should not be idle now for some reason */ > > > > + if (RB_EMPTY_ROOT(&cfqq->sort_list) && !cfq_should_idle(cfqd, cfqq)) > > > > + return true; > > > > + > > > > if (!cfqd->active_cic || !cfq_cfqq_wait_request(cfqq)) > > > > return false; > > > > > > > > @@ -3508,8 +3512,25 @@ static void cfq_completed_request(struct > > > > } > > > > } > > > > > > > > - if (!cfqd->rq_in_driver) > > > > + if (!cfqd->rq_in_driver) { > > > > cfq_schedule_dispatch(cfqd); > > > > + return; > > > > + } > > > > + /* > > > > + * A queue is idle at cfq_dispatch_requests(), but it gets noidle > > > > + * later. We schedule a dispatch if the queue has no requests, > > > > + * otherwise the disk is actually in idle till all requests > > > > + * are finished even cfq_arm_slice_timer doesn't make the queue idle > > > > + * */ > > > > > > Why do we have to wait for all requests to finish in device? Will driver > > > most likely not ask for next request when 1-2 requests have completed > > > and at that time we should expire the queue if queue is no more marked > > > as "noidle"? > > The issue is a queue is idle just because it's the last queue of the > > service tree. Then a new queue is added and the idled queue should not > > idle now. we should preempt the idled queue soon. does this make sense > > to you? > > If that's the case then you should just modify should_preempt() so that > addition of a new queue could preempt an empty queue which has now become > noidle. > > You have also modified cfq_completed_request() function, which will wake > up the worker thread and then try to dispatch a request. IMHO, in practice > driver asks for new request almost immediately and you don't gain much > by this additional wakeup. > > So my point being, that we increased the code complexity for no visible > performance improvement also increased thread wakeups resulting in more > cpu consumption. Ah, you are right, we only need modify should_preempt. Updated the patch as below.
> If there was a visible performance gain in your testing then it would have > made sense but you said that you did not notice any improvements. Then > why to increase the complexity. I only test some workloads and can't do all tests, but this is an obvious bug I thought.
Thanks, Shaohua
Subject: cfq-iosched: preempt an idle queue if it should not be idle any more
A queue is idle at cfq_dispatch_requests(), but it gets noidle later. Unless other task explictly does unplug or all requests are drained, we will not deliever requests to the disk even cfq_arm_slice_timer doesn't make the queue idle. For example, cfq_should_idle() returns true because of service_tree->count == 1, and then other queues are added. Note, I didn't see obvious performance impacts so far with the patch, but just thought this could be a problem.
Signed-off-by: Shaohua Li <shaohua.li@intel.com>
--- block/cfq-iosched.c | 4 ++++ 1 file changed, 4 insertions(+)
Index: linux/block/cfq-iosched.c =================================================================== --- linux.orig/block/cfq-iosched.c 2010-11-09 10:20:38.000000000 +0800 +++ linux/block/cfq-iosched.c 2010-11-09 10:20:54.000000000 +0800 @@ -3265,6 +3265,10 @@ cfq_should_preempt(struct cfq_data *cfqd if (cfq_class_rt(new_cfqq) && !cfq_class_rt(cfqq)) return true; + /* An idle queue should not be idle now for some reason */ + if (RB_EMPTY_ROOT(&cfqq->sort_list) && !cfq_should_idle(cfqd, cfqq)) + return true; + if (!cfqd->active_cic || !cfq_cfqq_wait_request(cfqq)) return false;
| ![\](/images/icornerr.gif) |