Re: performance "regression" in cfq compared to anticipatory, deadline and noop

[Jens Axboe]

On Fri, May 16 2008, Jens Axboe wrote:
> On Fri, May 16 2008, Fabio Checconi wrote:
> > > From: Jens Axboe <jens.axboe@xxxxxxxxxx>
> > > Date: Fri, May 16, 2008 08:40:03AM +0200
> > >
> > ...
> > > I think we can improve this further without getting too involved. If a
> > > 2nd request is seen in cfq_rq_enqueued(), then DO schedule a dispatch
> > > since this likely means that we wont be doing more merges on the first
> > > one.
> > > 
> > 
> > But isn't there the risk that even the second request would be
> > dispatched, while it still could have grown?
> 
> Certainly, you'd only want to dispatch the first request. Ideally we'd
> just get rid of this logic of 'did empty dispatch round' and only
> dispatch requests once merging is done, it's basically the wrong thing
> to do to make it visible to the io scheduler so soon. Well of course
> even more ideally we'd always get big requests submitted, but
> unfortunately many producers aren't that nice.
> 
> The per-process plugging actually solves this nicely, since we do the
> merging outside of the io scheduler. Perhaps just not dispatch on a
> plugged queue would help a bit. I'm somewhat against this principle of
> messing too much with dispatch logic in the schedulers, it'd be nicer to
> solve this higher up.

Something like this...

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 5dfb7b9..5ab1a17 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -1775,6 +1775,9 @@ cfq_rq_enqueued(struct cfq_data *cfqd, struct cfq_queue *cfqq,
 
 	cic->last_request_pos = rq->sector + rq->nr_sectors;
 
+	if (blk_queue_plugged(cfqd->queue))
+		return;
+
 	if (cfqq == cfqd->active_queue) {
 		/*
 		 * if we are waiting for a request for this queue, let it rip
@@ -1784,7 +1787,7 @@ cfq_rq_enqueued(struct cfq_data *cfqd, struct cfq_queue *cfqq,
 		if (cfq_cfqq_wait_request(cfqq)) {
 			cfq_mark_cfqq_must_dispatch(cfqq);
 			del_timer(&cfqd->idle_slice_timer);
-			blk_start_queueing(cfqd->queue);
+			cfq_schedule_dispatch(cfqd);
 		}
 	} else if (cfq_should_preempt(cfqd, cfqq, rq)) {
 		/*
@@ -1794,7 +1797,7 @@ cfq_rq_enqueued(struct cfq_data *cfqd, struct cfq_queue *cfqq,
 		 */
 		cfq_preempt_queue(cfqd, cfqq);
 		cfq_mark_cfqq_must_dispatch(cfqq);
-		blk_start_queueing(cfqd->queue);
+		cfq_schedule_dispatch(cfqd);
 	}
 }
 
@@ -1997,11 +2000,10 @@ static void cfq_kick_queue(struct work_struct *work)
 	struct cfq_data *cfqd =
 		container_of(work, struct cfq_data, unplug_work);
 	struct request_queue *q = cfqd->queue;
-	unsigned long flags;
 
-	spin_lock_irqsave(q->queue_lock, flags);
+	spin_lock_irq(q->queue_lock);
 	blk_start_queueing(q);
-	spin_unlock_irqrestore(q->queue_lock, flags);
+	spin_unlock_irq(q->queue_lock);
 }
 
 /*

-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/