[patch 2/3]cfq-iosched: schedule dispatch for noidle queue

[patch 2/3]cfq-iosched: schedule dispatch for noidle queue

[Shaohua Li]

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
+	 * */
+	cfqq = cfqd->active_queue;
+	if (!cfqq)
+		return;
+
+	if (RB_EMPTY_ROOT(&cfqq->sort_list) && !cfq_should_idle(cfqd, cfqq) &&
+	    (!cfqd->cfq_group_idle || cfqq->cfqg->nr_cfqq > 1)) {
+		cfq_del_timer(cfqd, cfqq);
+		cfq_schedule_dispatch(cfqd);
+	}
 }
 
 /*

Re: [patch 2/3]cfq-iosched: schedule dispatch for noidle queue

[Vivek Goyal]

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>

Hi Shaohua,

I am unable to understand what problem are you solving here. So a queue
currently has property as "idle" and we idle on that. And then some event
happens which makes it "noidle". What is that event?

To my knowledge generally it is arrival of a new request. If that's the case,
then upon arrival of new request in same queue, we will dispatch it and then
when device asks for more request, we should be expering the cfqq and dispatch
requests from other queues.

So I am really not sure what's the problem here which you are trying to 
fix. Can you give more details.

Vivek

 

> 
> ---
>  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
> +	 * */
> +	cfqq = cfqd->active_queue;
> +	if (!cfqq)
> +		return;
> +
> +	if (RB_EMPTY_ROOT(&cfqq->sort_list) && !cfq_should_idle(cfqd, cfqq) &&
> +	    (!cfqd->cfq_group_idle || cfqq->cfqg->nr_cfqq > 1)) {
> +		cfq_del_timer(cfqd, cfqq);
> +		cfq_schedule_dispatch(cfqd);
> +	}
>  }
>  
>  /*
> 

Re: [patch 2/3]cfq-iosched: schedule dispatch for noidle queue

[Vivek Goyal]

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"?

Vivek

> +	cfqq = cfqd->active_queue;
> +	if (!cfqq)
> +		return;
> +
> +	if (RB_EMPTY_ROOT(&cfqq->sort_list) && !cfq_should_idle(cfqd, cfqq) &&
> +	    (!cfqd->cfq_group_idle || cfqq->cfqg->nr_cfqq > 1)) {
> +		cfq_del_timer(cfqd, cfqq);
> +		cfq_schedule_dispatch(cfqd);
> +	}
>  }
>  
>  /*
> 

Re: [patch 2/3]cfq-iosched: schedule dispatch for noidle queue

[Shaohua Li]

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?

Thanks,
Shaohua

Re: [patch 2/3]cfq-iosched: schedule dispatch for noidle queue

[Vivek Goyal]

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.

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.

Vivek

Re: [patch 2/3]cfq-iosched: schedule dispatch for noidle queue

[Vivek Goyal]

On Mon, Nov 08, 2010 at 09:15:40PM -0500, 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.
> 
> 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.

Thinking more about it. I suspect modification to cfq_completed_request()
is a dead code. In the sense, status of a queue will change from idle to
noidle, when a new request comes in (for most of the cases). If that's
the case then we don't have to modify cfq_completed_request() code at
all. An extra condition in cfq_should_preempt() should cover the case of
preempting the existing empty queue which is noidle.

Can you give me an example in what cases cfq_completed_request() will
really wake up the thread which will result in early dispatch of request
then it would have been if we relied on driver asking for next request.
One such example could be that somebody set slice_idle=0 but that's one
time event and I really don't think we should be optimizing for that 
corner case.

Thanks
Vivek

Re: [patch 2/3]cfq-iosched: schedule dispatch for noidle queue

[Shaohua Li]

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;
 

Re: [patch 2/3]cfq-iosched: schedule dispatch for noidle queue

[Vivek Goyal]

On Tue, Nov 09, 2010 at 10:28:36AM +0800, Shaohua Li wrote:

[..]
> > > > 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.
> 

Thanks. Jens has already applied the patches on for-2.6.38/core branch of
block tree. I think you shall have to generate an incremental patch
which revert the bits introduced in cfq_completed_request().

Thanks
Vivek

> > 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;
>  
> 
> 

Re: [patch 2/3]cfq-iosched: schedule dispatch for noidle queue

[Shaohua Li]

On Tue, 2010-11-09 at 10:39 +0800, Vivek Goyal wrote:
> On Tue, Nov 09, 2010 at 10:28:36AM +0800, Shaohua Li wrote:
> 
> [..]
> > > > > 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.
> > 
> 
> Thanks. Jens has already applied the patches on for-2.6.38/core branch of
> block tree. I think you shall have to generate an incremental patch
> which revert the bits introduced in cfq_completed_request().
Jens, how to handle this? if you want to an incremental patch, here it
is.

Subject: cfq-iosched: don't schedule a dispatch for a non-idle queue

Vivek suggests we don't need schedule a dispatch when an idle queue
becomes nonidle. And he is right, cfq_should_preempt already covers
the logic.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>

---
 block/cfq-iosched.c |   19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

Index: linux/block/cfq-iosched.c
===================================================================
--- linux.orig/block/cfq-iosched.c	2010-11-09 10:48:08.000000000 +0800
+++ linux/block/cfq-iosched.c	2010-11-09 10:48:44.000000000 +0800
@@ -3512,25 +3512,8 @@ 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
-	 * */
-	cfqq = cfqd->active_queue;
-	if (!cfqq)
-		return;
-
-	if (RB_EMPTY_ROOT(&cfqq->sort_list) && !cfq_should_idle(cfqd, cfqq) &&
-	    (!cfqd->cfq_group_idle || cfqq->cfqg->nr_cfqq > 1)) {
-		cfq_del_timer(cfqd, cfqq);
-		cfq_schedule_dispatch(cfqd);
-	}
 }
 
 /*

Re: [patch 2/3]cfq-iosched: schedule dispatch for noidle queue

[Jens Axboe]

On 2010-11-09 03:58, Shaohua Li wrote:
> On Tue, 2010-11-09 at 10:39 +0800, Vivek Goyal wrote:
>> On Tue, Nov 09, 2010 at 10:28:36AM +0800, Shaohua Li wrote:
>>
>> [..]
>>>>>> 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.
>>>
>>
>> Thanks. Jens has already applied the patches on for-2.6.38/core branch of
>> block tree. I think you shall have to generate an incremental patch
>> which revert the bits introduced in cfq_completed_request().
> Jens, how to handle this? if you want to an incremental patch, here it
> is.
> 
> Subject: cfq-iosched: don't schedule a dispatch for a non-idle queue
> 
> Vivek suggests we don't need schedule a dispatch when an idle queue
> becomes nonidle. And he is right, cfq_should_preempt already covers
> the logic.

Thanks Vivek, I agree with your analysis. I have applied this one as well.

-- 
Jens Axboe