public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] cfq-iosched: requests "in flight" vs "in driver" clarification
@ 2010-02-27 16:03 Corrado Zoccolo
  2010-02-28 18:44 ` Jens Axboe
  2010-03-01 14:11 ` Vivek Goyal
  0 siblings, 2 replies; 4+ messages in thread
From: Corrado Zoccolo @ 2010-02-27 16:03 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Linux-Kernel, Jeff Moyer, Vivek Goyal, Shaohua Li, Gui Jianfeng,
	Corrado Zoccolo

Counters for requests "in flight" and "in driver" are used asymmetrically
in cfq_may_dispatch, and have slightly different meaning.
We split the rq_in_flight counter (was sync_flight) to count both sync
and async requests, in order to use this one, which is more accurate in
some corner cases.
The rq_in_driver counter is coalesced, since individual sync/async counts
are not used any more.

Signed-off-by: Corrado Zoccolo <czoccolo@gmail.com>
---
 block/cfq-iosched.c |   44 ++++++++++++++++++--------------------------
 1 files changed, 18 insertions(+), 26 deletions(-)

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 10eb286..eed649c 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -223,8 +223,8 @@ struct cfq_data {
 
 	unsigned int busy_queues;
 
-	int rq_in_driver[2];
-	int sync_flight;
+	int rq_in_driver;
+	int rq_in_flight[2];
 
 	/*
 	 * queue-depth detection
@@ -417,11 +417,6 @@ static struct cfq_queue *cfq_get_queue(struct cfq_data *, bool,
 static struct cfq_io_context *cfq_cic_lookup(struct cfq_data *,
 						struct io_context *);
 
-static inline int rq_in_driver(struct cfq_data *cfqd)
-{
-	return cfqd->rq_in_driver[0] + cfqd->rq_in_driver[1];
-}
-
 static inline struct cfq_queue *cic_to_cfqq(struct cfq_io_context *cic,
 					    bool is_sync)
 {
@@ -1415,9 +1410,9 @@ static void cfq_activate_request(struct request_queue *q, struct request *rq)
 {
 	struct cfq_data *cfqd = q->elevator->elevator_data;
 
-	cfqd->rq_in_driver[rq_is_sync(rq)]++;
+	cfqd->rq_in_driver++;
 	cfq_log_cfqq(cfqd, RQ_CFQQ(rq), "activate rq, drv=%d",
-						rq_in_driver(cfqd));
+						cfqd->rq_in_driver);
 
 	cfqd->last_position = blk_rq_pos(rq) + blk_rq_sectors(rq);
 }
@@ -1425,12 +1420,11 @@ static void cfq_activate_request(struct request_queue *q, struct request *rq)
 static void cfq_deactivate_request(struct request_queue *q, struct request *rq)
 {
 	struct cfq_data *cfqd = q->elevator->elevator_data;
-	const int sync = rq_is_sync(rq);
 
-	WARN_ON(!cfqd->rq_in_driver[sync]);
-	cfqd->rq_in_driver[sync]--;
+	WARN_ON(!cfqd->rq_in_driver);
+	cfqd->rq_in_driver--;
 	cfq_log_cfqq(cfqd, RQ_CFQQ(rq), "deactivate rq, drv=%d",
-						rq_in_driver(cfqd));
+						cfqd->rq_in_driver);
 }
 
 static void cfq_remove_request(struct request *rq)
@@ -1873,8 +1867,7 @@ static void cfq_dispatch_insert(struct request_queue *q, struct request *rq)
 	cfqq->dispatched++;
 	elv_dispatch_sort(q, rq);
 
-	if (cfq_cfqq_sync(cfqq))
-		cfqd->sync_flight++;
+	cfqd->rq_in_flight[cfq_cfqq_sync(cfqq)]++;
 	cfqq->nr_sectors += blk_rq_sectors(rq);
 }
 
@@ -2221,13 +2214,13 @@ static bool cfq_may_dispatch(struct cfq_data *cfqd, struct cfq_queue *cfqq)
 	/*
 	 * Drain async requests before we start sync IO
 	 */
-	if (cfq_should_idle(cfqd, cfqq) && cfqd->rq_in_driver[BLK_RW_ASYNC])
+	if (cfq_should_idle(cfqd, cfqq) && cfqd->rq_in_flight[BLK_RW_ASYNC])
 		return false;
 
 	/*
 	 * If this is an async queue and we have sync IO in flight, let it wait
 	 */
-	if (cfqd->sync_flight && !cfq_cfqq_sync(cfqq))
+	if (cfqd->rq_in_flight[BLK_RW_SYNC] && !cfq_cfqq_sync(cfqq))
 		return false;
 
 	max_dispatch = cfqd->cfq_quantum;
@@ -3210,14 +3203,14 @@ static void cfq_update_hw_tag(struct cfq_data *cfqd)
 {
 	struct cfq_queue *cfqq = cfqd->active_queue;
 
-	if (rq_in_driver(cfqd) > cfqd->hw_tag_est_depth)
-		cfqd->hw_tag_est_depth = rq_in_driver(cfqd);
+	if (cfqd->rq_in_driver > cfqd->hw_tag_est_depth)
+		cfqd->hw_tag_est_depth = cfqd->rq_in_driver;
 
 	if (cfqd->hw_tag == 1)
 		return;
 
 	if (cfqd->rq_queued <= CFQ_HW_QUEUE_MIN &&
-	    rq_in_driver(cfqd) <= CFQ_HW_QUEUE_MIN)
+	    cfqd->rq_in_driver <= CFQ_HW_QUEUE_MIN)
 		return;
 
 	/*
@@ -3227,7 +3220,7 @@ static void cfq_update_hw_tag(struct cfq_data *cfqd)
 	 */
 	if (cfqq && cfq_cfqq_idle_window(cfqq) &&
 	    cfqq->dispatched + cfqq->queued[0] + cfqq->queued[1] <
-	    CFQ_HW_QUEUE_MIN && rq_in_driver(cfqd) < CFQ_HW_QUEUE_MIN)
+	    CFQ_HW_QUEUE_MIN && cfqd->rq_in_driver < CFQ_HW_QUEUE_MIN)
 		return;
 
 	if (cfqd->hw_tag_samples++ < 50)
@@ -3280,13 +3273,12 @@ static void cfq_completed_request(struct request_queue *q, struct request *rq)
 
 	cfq_update_hw_tag(cfqd);
 
-	WARN_ON(!cfqd->rq_in_driver[sync]);
+	WARN_ON(!cfqd->rq_in_driver);
 	WARN_ON(!cfqq->dispatched);
-	cfqd->rq_in_driver[sync]--;
+	cfqd->rq_in_driver--;
 	cfqq->dispatched--;
 
-	if (cfq_cfqq_sync(cfqq))
-		cfqd->sync_flight--;
+	cfqd->rq_in_flight[cfq_cfqq_sync(cfqq)]--;
 
 	if (sync) {
 		RQ_CIC(rq)->last_end_request = now;
@@ -3340,7 +3332,7 @@ static void cfq_completed_request(struct request_queue *q, struct request *rq)
 		}
 	}
 
-	if (!rq_in_driver(cfqd))
+	if (!cfqd->rq_in_driver)
 		cfq_schedule_dispatch(cfqd);
 }
 
-- 
1.6.4.4


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] cfq-iosched: replace sync_flight by rq_in_driver[BLK_RW_SYNC]
@ 2009-12-30 21:16 Jens Axboe
  2009-12-30 22:24 ` [PATCH] cfq-iosched: requests "in flight" vs "in driver" clarification Corrado Zoccolo
  0 siblings, 1 reply; 4+ messages in thread
From: Jens Axboe @ 2009-12-30 21:16 UTC (permalink / raw)
  To: Corrado Zoccolo
  Cc: Linux-Kernel, Jeff Moyer, Vivek Goyal, Shaohua Li, Gui Jianfeng

On Wed, Dec 30 2009, Corrado Zoccolo wrote:
> On Wed, Dec 30, 2009 at 7:47 PM, Jens Axboe <jens.axboe@oracle.com> wrote:
> > On Wed, Dec 30 2009, Corrado Zoccolo wrote:
> >> According to my intuition (and brief testing), sync_flight is always
> >> equal to rq_in_driver[BLK_RW_SYNC] at the point of usage, so it can
> >> be removed and replaced by the other.
> >
> > They are not fully identical. ->sync_flight is incremented on insertion
> > on the dispatch list, ->rq_in_driver not until the request is activated
> > (eg the driver has retrieved it and wants to dispatch to the hardware).
> Usually (only exceptions are forced dispatch, or when a conflict in the
> rb tree is found), a request is activated as soon as cfq returns from
> cfq_dispatch_requests.

Yes, but then it may be deactivated immediately for requeue.

> > They will usually be identical, but that may not be true for requeues
> > for instance.
> 
> For our purpose, it is sufficient that in cfq_may_dispatch, they are either
> both 0 or both non-0.
> Since we have sync_flight >= rq_in_driver[1], the only question is:
> can the number of requests in the driver drop to 0 with requests still
> in flight?

It's mostly a theoretical issue, but yes it could happen. I'm assuming
you mean ->rq_in_driver[1] == 0 while ->sync_flight != 0. But then we
are into the area of some starvation problem, in hardware or in the
kernel. So it's not likely, but still.

> I'm asking because to drain async requests, we are using the rq_in_driver
> counter instead. Maybe they need the same treatment.

In theory, yes the same applies there. Normal operations would not have
that distinction between activated and on dispatch list.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-03-01 14:11 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-02-27 16:03 [PATCH] cfq-iosched: requests "in flight" vs "in driver" clarification Corrado Zoccolo
2010-02-28 18:44 ` Jens Axboe
2010-03-01 14:11 ` Vivek Goyal
  -- strict thread matches above, loose matches on Subject: below --
2009-12-30 21:16 [PATCH] cfq-iosched: replace sync_flight by rq_in_driver[BLK_RW_SYNC] Jens Axboe
2009-12-30 22:24 ` [PATCH] cfq-iosched: requests "in flight" vs "in driver" clarification Corrado Zoccolo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox