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

[Corrado Zoccolo]

On Thu, Mar 18, 2010 at 2:05 AM, Shaohua Li <shaohua.li@xxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Shaohua Li <shaohua.li@xxxxxxxxx>
Acked-by: Corrado Zoccolo <czoccolo@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/