cfq-iosched: fix a kbuild regression
in [Kernel]
|
Prev: [GIT PULL updated] led updates for 2.6.34-rc2
Next: [PATCH 17/17] Staging: comedi: fix indent coding style issue in adl_pci9118.c
From: Jens Axboe on 17 Mar 2010 09:40 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? > > Reported-by: Alex Shi <alex.shi(a)intel.com> > Signed-off-by: Shaohua Li <shaohua.li(a)intel.com> > > diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c > index dee9d93..fcae456 100644 > --- a/block/cfq-iosched.c > +++ b/block/cfq-iosched.c > @@ -46,7 +46,7 @@ static const int cfq_hist_divisor = 4; > #define CFQ_HW_QUEUE_MIN (5) > #define CFQ_SERVICE_SHIFT 12 > > -#define CFQQ_SEEK_THR (sector_t)(8 * 100) > +#define CFQQ_SEEK_THR (sector_t)(8 * 1024) > #define CFQQ_SECT_THR_NONROT (sector_t)(2 * 32) > #define CFQQ_SEEKY(cfqq) (hweight32(cfqq->seek_history) > 32/8) > -- Jens Axboe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Corrado Zoccolo on 17 Mar 2010 14:30 On Wed, Mar 17, 2010 at 2:30 PM, Jens Axboe <jens.axboe(a)oracle.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). 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). Corrado > >> >> Reported-by: Alex Shi <alex.shi(a)intel.com> >> Signed-off-by: Shaohua Li <shaohua.li(a)intel.com> >> >> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c >> index dee9d93..fcae456 100644 >> --- a/block/cfq-iosched.c >> +++ b/block/cfq-iosched.c >> @@ -46,7 +46,7 @@ static const int cfq_hist_divisor = 4; >> #define CFQ_HW_QUEUE_MIN (5) >> #define CFQ_SERVICE_SHIFT 12 >> >> -#define CFQQ_SEEK_THR (sector_t)(8 * 100) >> +#define CFQQ_SEEK_THR (sector_t)(8 * 1024) >> #define CFQQ_SECT_THR_NONROT (sector_t)(2 * 32) >> #define CFQQ_SEEKY(cfqq) (hweight32(cfqq->seek_history) > 32/8) >> > > -- > Jens Axboe > > -- __________________________________________________________________________ dott. Corrado Zoccolo mailto:czoccolo(a)gmail.com PhD - Department of Computer Science - University of Pisa, Italy -------------------------------------------------------------------------- The self-confidence of a warrior is not the self-confidence of the average man. The average man seeks certainty in the eyes of the onlooker and calls that self-confidence. The warrior seeks impeccability in his own eyes and calls that humbleness. Tales of Power - C. Castaneda -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Shaohua Li on 17 Mar 2010 21:10 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(a)oracle.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(a)intel.com> Signed-off-by: Shaohua Li <shaohua.li(a)intel.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(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Corrado Zoccolo on 18 Mar 2010 13:00 On Thu, Mar 18, 2010 at 2:05 AM, Shaohua Li <shaohua.li(a)intel.com> wrote: > 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(a)oracle.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(a)intel.com> > Signed-off-by: Shaohua Li <shaohua.li(a)intel.com> Acked-by: Corrado Zoccolo <czoccolo(a)gmail.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(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jens Axboe on 19 Mar 2010 04:00
On Thu, Mar 18 2010, Shaohua Li wrote: > 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(a)oracle.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. Thanks, I have applied it. -- Jens Axboe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |