linux-kernel - Re: [RFC]cfq-iosched: fix a kbuild regression

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100318010459.GA26716@sli10-desk.sh.intel.com>
Date:	Thu, 18 Mar 2010 09:05:00 +0800
From:	Shaohua Li <shaohua.li@...el.com>
To:	Corrado Zoccolo <czoccolo@...il.com>
Cc:	Jens Axboe <jens.axboe@...cle.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"vgoyal@...hat.com" <vgoyal@...hat.com>,
	"jmoyer@...hat.com" <jmoyer@...hat.com>,
	"guijianfeng@...fujitsu.com" <guijianfeng@...fujitsu.com>,
	"Shi, Alex" <alex.shi@...el.com>
Subject: Re: [RFC]cfq-iosched: fix a kbuild regression

On Thu, Mar 18, 2010 at 02:24:10AM +0800, Corrado Zoccolo wrote:
> On Wed, Mar 17, 2010 at 2:30 PM, Jens Axboe <jens.axboe@...cle.com> wrote:
> > On Tue, Mar 16 2010, Shaohua Li wrote:
> >> Alex Shi reported a kbuild regression which is about 10% performance lost.
> >> He bisected to this commit: 3dde36ddea3e07dd025c4c1ba47edec91606fec0.
> >> The reason is cfqq_close() can't find close cooperator. If we store the seek
> >> distance to the value before the commit like below, the regression fully goes
> >> away. If this is too invasive, just changing the cfq_rq_close() for the
> >> !for_preempt is ok too.
> >
> > Corrado, any objections to widening the seek threshold?
> 
> The previous seek threshold value was meant to be compared with the
> average seek distance, so it was large to account for variations.
> Since we handle the variations differently, we should have a smaller
> value now (the 100 * 8 was the result of a benchmark run on several
> disks).
Our test doesn't find any issue with the seek threshold so far, so it proberly
is ok.

> I agreee, though, that for merging queues, it is now too small, so we
> should have two thresholds, the current one used to determine if a
> request causes a seek, and an other to be used inside cfq_close (with
> the older value, regardless of for_preempt).
That makes sense. Updated patch.


Alex Shi reported a kbuild regression which is about 10% performance lost.
He bisected to this commit: 3dde36ddea3e07dd025c4c1ba47edec91606fec0.
The reason is cfqq_close() can't find close cooperator. Restoring 
cfq_rq_close()'s threshold to original value makes the regression go away.

Since for_preempt parameter isn't used anymore, this patch deletes it.

Reported-by: Alex Shi <alex.shi@...el.com>
Signed-off-by: Shaohua Li <shaohua.li@...el.com>

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index dee9d93..8d5a2f2 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -47,6 +47,7 @@ static const int cfq_hist_divisor = 4;
 #define CFQ_SERVICE_SHIFT       12
 
 #define CFQQ_SEEK_THR		(sector_t)(8 * 100)
+#define CFQQ_CLOSE_THR		(sector_t)(8 * 1024)
 #define CFQQ_SECT_THR_NONROT	(sector_t)(2 * 32)
 #define CFQQ_SEEKY(cfqq)	(hweight32(cfqq->seek_history) > 32/8)
 
@@ -1660,9 +1661,9 @@ static inline sector_t cfq_dist_from_last(struct cfq_data *cfqd,
 }
 
 static inline int cfq_rq_close(struct cfq_data *cfqd, struct cfq_queue *cfqq,
-			       struct request *rq, bool for_preempt)
+			       struct request *rq)
 {
-	return cfq_dist_from_last(cfqd, rq) <= CFQQ_SEEK_THR;
+	return cfq_dist_from_last(cfqd, rq) <= CFQQ_CLOSE_THR;
 }
 
 static struct cfq_queue *cfqq_close(struct cfq_data *cfqd,
@@ -1689,7 +1690,7 @@ static struct cfq_queue *cfqq_close(struct cfq_data *cfqd,
 	 * will contain the closest sector.
 	 */
 	__cfqq = rb_entry(parent, struct cfq_queue, p_node);
-	if (cfq_rq_close(cfqd, cur_cfqq, __cfqq->next_rq, false))
+	if (cfq_rq_close(cfqd, cur_cfqq, __cfqq->next_rq))
 		return __cfqq;
 
 	if (blk_rq_pos(__cfqq->next_rq) < sector)
@@ -1700,7 +1701,7 @@ static struct cfq_queue *cfqq_close(struct cfq_data *cfqd,
 		return NULL;
 
 	__cfqq = rb_entry(node, struct cfq_queue, p_node);
-	if (cfq_rq_close(cfqd, cur_cfqq, __cfqq->next_rq, false))
+	if (cfq_rq_close(cfqd, cur_cfqq, __cfqq->next_rq))
 		return __cfqq;
 
 	return NULL;
@@ -3103,7 +3104,7 @@ cfq_should_preempt(struct cfq_data *cfqd, struct cfq_queue *new_cfqq,
 	 * if this request is as-good as one we would expect from the
 	 * current cfqq, let it preempt
 	 */
-	if (cfq_rq_close(cfqd, cfqq, rq, true))
+	if (cfq_rq_close(cfqd, cfqq, rq))
 		return true;
 
 	return false;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/