Re: [RFC]cfq-iosched: quantum check tweak

[Corrado Zoccolo <czoccolo@gmail.com>]

Hi Shahoua,

On Thu, Jan 7, 2010 at 3:04 AM, Shaohua Li <shaohua.li@intel.com> wrote:
> On Mon, 2009-12-28 at 17:02 +0800, Corrado Zoccolo wrote:
>> Hi Shaohua,
>> On Mon, Dec 28, 2009 at 4:35 AM, Shaohua Li <shaohua.li@intel.com> wrote:
>> > On Fri, Dec 25, 2009 at 05:44:40PM +0800, Corrado Zoccolo wrote:
>> >> On Fri, Dec 25, 2009 at 10:10 AM, Shaohua Li <shaohua.li@intel.com> wrote:
>> >> > Currently a queue can only dispatch up to 4 requests if there are other queues.
>> >> > This isn't optimal, device can handle more requests, for example, AHCI can
>> >> > handle 31 requests. I can understand the limit is for fairness, but we could
>> >> > do some tweaks:
>> >> > 1. if the queue still has a lot of slice left, sounds we could ignore the limit
>> >> ok. You can even scale the limit proportionally to the remaining slice
>> >> (see below).
>> > I can't understand the meaning of below scale. cfq_slice_used_soon() means
>> > dispatched requests can finish before slice is used, so other queues will not be
>> > impacted. I thought/hope a cfq_slice_idle time is enough to finish the
>> > dispatched requests.
>> cfq_slice_idle is 8ms, that is the average time to complete 1 request
>> on most disks. If you have more requests dispatched on a
>> NCQ-rotational disk (non-RAID), it will take more time. Probably a
>> linear formula is not the most accurate, but still more accurate than
>> taking just 1 cfq_slice_idle. If you can experiment a bit, you could
>> also try:
>>  cfq_slice_idle * ilog2(nr_dispatched+1)
>>  cfq_slice_idle * (1<<(ilog2(nr_dispatched+1)>>1))
>>
>> >
>> >> > 2. we could keep the check only when cfq_latency is on. For uses who don't care
>> >> > about latency should be happy to have device fully piped on.
>> >> I wouldn't overload low_latency with this meaning. You can obtain the
>> >> same by setting the quantum to 32.
>> > As this impact fairness, so natually thought we could use low_latency. I'll remove
>> > the check in next post.
>> Great.
>> >> > I have a test of random direct io of two threads, each has 32 requests one time
>> >> > without patch: 78m/s
>> >> > with tweak 1: 138m/s
>> >> > with two tweaks and disable latency: 156m/s
>> >>
>> >> Please, test also with competing seq/random(depth1)/async workloads,
>> >> and measure also introduced latencies.
>> > depth1 should be ok, as if device can only send one request, it should not require
>> > more requests from ioscheduler.
>> I mean have a run with, at the same time:
>> * one seq reader,
>> * h random readers with depth 1 (non-aio)
>> * one async seq writer
>> * k random readers with large depth.
>> In this way, you can see if the changes you introduce to boost your
>> workload affect more realistic scenarios, in which various workloads
>> are mixed.
>> I explicitly add the depth1 random readers, since they are sceduled
>> differently than the large (>4) depth ones.
> I tried a fio script which does like your description, but the data
> isn't stable, especially the write speed, other kind of io speed is
> stable. Apply below patch doesn't make things worse (still write speed
> isn't stable, other io is stable), so I can't say if the patch passes
> the test, but it appears latency reported by fio hasn't change. I adopt
> the slice_idle * dispatched approach, which I thought should be safe.

I'm doing some tests right now on a single ncq rotational disk, and
the average service time when submitting with a high depth is halved
w.r.t. depth 1, so I think you could test also with the formula :
slice_idle * dispatched / 2. It should give a performance boost,
without noticeable impact on latency.

> Currently a queue can only dispatch up to 4 requests if there are other queues.
> This isn't optimal, device can handle more requests, for example, AHCI can
> handle 31 requests. I can understand the limit is for fairness, but we could
> do a tweak: if the queue still has a lot of slice left, sounds we could ignore
> the limit.
> For async io, 40ms/8ms = 5 - quantum = 1, we only send extra 1 request in maxium.
> For sync io, 100ms/8ms = 12 - quantum = 8, we might send extra 8 requests in maxium.
> This might cause latency issue if the queue is preempted at the very beginning.
>
> This patch boost my workload from 78m/s to 102m/s, which isn't that big as my last
> post, but also is a big improvement.

Acked-by: Corrado Zoccolo <czoccolo@gmail.com>

>
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> ---
>  block/cfq-iosched.c |   15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> Index: linux-2.6/block/cfq-iosched.c
> ===================================================================
> --- linux-2.6.orig/block/cfq-iosched.c
> +++ linux-2.6/block/cfq-iosched.c
> @@ -2242,6 +2242,19 @@ static int cfq_forced_dispatch(struct cf
>        return dispatched;
>  }
>
> +static inline bool cfq_slice_used_soon(struct cfq_data *cfqd,
> +       struct cfq_queue *cfqq)
> +{
> +       /* the queue hasn't finished any request, can't estimate */
> +       if (cfq_cfqq_slice_new(cfqq))
> +               return true;
> +       if (time_after(jiffies + cfqd->cfq_slice_idle * cfqq->dispatched,
> +               cfqq->slice_end))
> +               return true;
> +
> +       return false;
> +}
> +
>  static bool cfq_may_dispatch(struct cfq_data *cfqd, struct cfq_queue *cfqq)
>  {
>        unsigned int max_dispatch;
> @@ -2275,7 +2288,7 @@ static bool cfq_may_dispatch(struct cfq_
>                /*
>                 * We have other queues, don't allow more IO from this one
>                 */
> -               if (cfqd->busy_queues > 1)
> +               if (cfqd->busy_queues > 1 && cfq_slice_used_soon(cfqd, cfqq))
>                        return false;
>
>                /*
>
>