* [RFC]cfq-iosched: fix a kbuild regression @ 2010-03-16 2:56 Shaohua Li 2010-03-17 13:30 ` Jens Axboe 0 siblings, 1 reply; 6+ messages in thread From: Shaohua Li @ 2010-03-16 2:56 UTC (permalink / raw) To: linux-kernel Cc: jens.axboe, czoccolo, vgoyal, jmoyer, guijianfeng, alex.shi, shaohua.li 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. Reported-by: Alex Shi <alex.shi@intel.com> Signed-off-by: Shaohua Li <shaohua.li@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) ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC]cfq-iosched: fix a kbuild regression 2010-03-16 2:56 [RFC]cfq-iosched: fix a kbuild regression Shaohua Li @ 2010-03-17 13:30 ` Jens Axboe 2010-03-17 18:24 ` Corrado Zoccolo 0 siblings, 1 reply; 6+ messages in thread From: Jens Axboe @ 2010-03-17 13:30 UTC (permalink / raw) To: Shaohua Li; +Cc: linux-kernel, czoccolo, vgoyal, jmoyer, guijianfeng, alex.shi 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@intel.com> > Signed-off-by: Shaohua Li <shaohua.li@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC]cfq-iosched: fix a kbuild regression 2010-03-17 13:30 ` Jens Axboe @ 2010-03-17 18:24 ` Corrado Zoccolo 2010-03-18 1:05 ` Shaohua Li 0 siblings, 1 reply; 6+ messages in thread From: Corrado Zoccolo @ 2010-03-17 18:24 UTC (permalink / raw) To: Jens Axboe Cc: Shaohua Li, linux-kernel, vgoyal, jmoyer, guijianfeng, alex.shi On Wed, Mar 17, 2010 at 2:30 PM, Jens Axboe <jens.axboe@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@intel.com> >> Signed-off-by: Shaohua Li <shaohua.li@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@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC]cfq-iosched: fix a kbuild regression 2010-03-17 18:24 ` Corrado Zoccolo @ 2010-03-18 1:05 ` Shaohua Li 2010-03-18 16:55 ` Corrado Zoccolo 2010-03-19 7:57 ` Jens Axboe 0 siblings, 2 replies; 6+ messages in thread From: Shaohua Li @ 2010-03-18 1:05 UTC (permalink / raw) To: Corrado Zoccolo Cc: Jens Axboe, linux-kernel, vgoyal, jmoyer, guijianfeng, Shi, Alex 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@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@intel.com> Signed-off-by: Shaohua Li <shaohua.li@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; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC]cfq-iosched: fix a kbuild regression 2010-03-18 1:05 ` Shaohua Li @ 2010-03-18 16:55 ` Corrado Zoccolo 2010-03-19 7:57 ` Jens Axboe 1 sibling, 0 replies; 6+ messages in thread From: Corrado Zoccolo @ 2010-03-18 16:55 UTC (permalink / raw) To: Shaohua Li Cc: Jens Axboe, linux-kernel, vgoyal, jmoyer, guijianfeng, Shi, Alex On Thu, Mar 18, 2010 at 2:05 AM, Shaohua Li <shaohua.li@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@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@intel.com> > Signed-off-by: Shaohua Li <shaohua.li@intel.com> Acked-by: Corrado Zoccolo <czoccolo@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; > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC]cfq-iosched: fix a kbuild regression 2010-03-18 1:05 ` Shaohua Li 2010-03-18 16:55 ` Corrado Zoccolo @ 2010-03-19 7:57 ` Jens Axboe 1 sibling, 0 replies; 6+ messages in thread From: Jens Axboe @ 2010-03-19 7:57 UTC (permalink / raw) To: Shaohua Li Cc: Corrado Zoccolo, linux-kernel, vgoyal, jmoyer, guijianfeng, Shi, Alex 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@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-03-19 7:57 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-03-16 2:56 [RFC]cfq-iosched: fix a kbuild regression Shaohua Li 2010-03-17 13:30 ` Jens Axboe 2010-03-17 18:24 ` Corrado Zoccolo 2010-03-18 1:05 ` Shaohua Li 2010-03-18 16:55 ` Corrado Zoccolo 2010-03-19 7:57 ` Jens Axboe
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.