[PATCH] cfq-iosched: replace sync_flight by rq_in_driver[BLK_RW_SYNC]

[PATCH] cfq-iosched: replace sync_flight by rq_in_driver[BLK_RW_SYNC]

[Corrado Zoccolo]

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.

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

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 01bb0f3..4865e08 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -228,7 +228,6 @@ struct cfq_data {
 	unsigned int busy_queues;
 
 	int rq_in_driver[2];
-	int sync_flight;
 
 	/*
 	 * queue-depth detection
@@ -1875,8 +1874,6 @@ 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++;
 	cfqq->nr_sectors += blk_rq_sectors(rq);
 }
 
@@ -2229,7 +2226,7 @@ static bool cfq_may_dispatch(struct cfq_data *cfqd, struct cfq_queue *cfqq)
 	/*
 	 * 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_driver[BLK_RW_SYNC] && !cfq_cfqq_sync(cfqq))
 		return false;
 
 	max_dispatch = cfqd->cfq_quantum;
@@ -3294,9 +3291,6 @@ static void cfq_completed_request(struct request_queue *q, struct request *rq)
 	cfqd->rq_in_driver[sync]--;
 	cfqq->dispatched--;
 
-	if (cfq_cfqq_sync(cfqq))
-		cfqd->sync_flight--;
-
 	if (sync) {
 		RQ_CIC(rq)->last_end_request = now;
 		if (!time_after(rq->start_time + cfqd->cfq_fifo_expire[1], now))
-- 
1.6.4.4

Re: [PATCH] cfq-iosched: replace sync_flight by rq_in_driver[BLK_RW_SYNC]

[Jens Axboe]

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).
They will usually be identical, but that may not be true for requeues
for instance.

-- 
Jens Axboe

Re: [PATCH] cfq-iosched: replace sync_flight by rq_in_driver[BLK_RW_SYNC]

[Corrado Zoccolo]

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.

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

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

Thanks
Corrado

>
> --
> Jens Axboe
>
>

Re: [PATCH] cfq-iosched: replace sync_flight by rq_in_driver[BLK_RW_SYNC]

[Jens Axboe]

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

[PATCH] cfq-iosched: requests "in flight" vs "in driver" clarification

[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 7da9391..c6d5678 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -227,8 +227,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
@@ -419,11 +419,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)
 {
@@ -1423,9 +1418,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);
 }
@@ -1433,12 +1428,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)
@@ -1876,8 +1870,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);
 }
 
@@ -2224,13 +2217,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;
@@ -3220,14 +3213,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;
 
 	/*
@@ -3237,7 +3230,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)
@@ -3290,13 +3283,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;
@@ -3350,7 +3342,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