Re: [PATCH] cfq-iosched: NCQ SSDs do not need read queue merging

[Corrado Zoccolo <czoccolo@gmail.com>]

On Mon, Jan 11, 2010 at 6:07 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Mon, Jan 11, 2010 at 06:00:51PM +0100, Corrado Zoccolo wrote:
>> On Mon, Jan 11, 2010 at 5:44 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > On Mon, Jan 11, 2010 at 03:53:00PM +0100, Corrado Zoccolo wrote:
>> >> On Mon, Jan 11, 2010 at 2:18 PM, Jeff Garzik <jeff@garzik.org> wrote:
>> >> > On 01/11/2010 08:13 AM, Jens Axboe wrote:
>> >> >>
>> >> >> On Mon, Jan 11 2010, Corrado Zoccolo wrote:
>> >> >>>
>> >> >>> On Mon, Jan 11, 2010 at 12:25 PM, Jeff Garzik<jeff@garzik.org>  wrote:
>> >> >>>>
>> >> >>>> On 01/10/2010 04:04 PM, Corrado Zoccolo wrote:
>> >> >>>>>
>> >> >>>>> NCQ SSDs' performances are not affected by
>> >> >>>>> distance of read requests, so there is no point in having
>> >> >>>>> overhead to merge such queues.
>> >> >>>>>
>> >> >>>>> Non-NCQ SSDs showed regression in some special cases, so
>> >> >>>>> they are ruled out by this patch.
>> >> >>>>>
>> >> >>>>> This patch intentionally doesn't affect writes, so
>> >> >>>>> it changes the queued[] field, to be indexed by
>> >> >>>>> READ/WRITE instead of SYNC/ASYNC, and only compute proximity
>> >> >>>>> for queues with WRITE requests.
>> >> >>>>>
>> >> >>>>> Signed-off-by: Corrado Zoccolo<czoccolo@gmail.com>
>> >> >>>>
>> >> >>>> That's not really true.  Overhead always increases as the total number
>> >> >>>> of
>> >> >>>> ATA commands issued increases.
>> >> >>>
>> >> >>> Jeff Moyer tested the patch on the workload that mostly benefit of
>> >> >>> queue merging, and found that
>> >> >>> the performance was improved by the patch.
>> >> >>> So removing the CPU overhead helps much more than the marginal gain
>> >> >>> given by merging on this hardware.
>> >> >>
>> >> >> It's not always going to be true. On SATA the command overhead is fairly
>> >> >> low, but on other hardware that may not be the case. Unless you are CPU
>> >> >> bound by your IO device, then merging will always be beneficial. I'm a
>> >> >> little behind on emails after my vacation, Jeff what numbers did you
>> >> >> generate and on what hardware?
>> >> >
>> >> >  ...and on what workload?   "the workload that mostly benefit of queue
>> >> > merging" is highly subjective, and likely does not cover most workloads SSDs
>> >> > will see in the field.
>> >> Hi Jeff,
>> >> exactly.
>> >> The workloads that benefits from queue merging are the ones in which a
>> >> sequential
>> >> read is actually splitt, and carried out by different processes in
>> >> different I/O context, each
>> >> sending requests with strides. This is clearly not the best way of
>> >> doing sequential access
>> >> (I would happily declare those programs as buggy).
>> >> CFQ has code that merges queues in this case. I'm disabling the READ
>> >> part for NCQ SSDs,
>> >> since, as Jeff measured, the code overhead outweight the gain from
>> >> merging (if any).
>> >
>> > Hi Corrado,
>> >
>> > In Jeff's test case of running read-test2, I am not even sure if any
>> > merging between the queues took place or not as on NCQ SSD, we are driving
>> > deeper queue depths and unless read-test2 is creating more than 32
>> > threads, there might not be any merging taking place at all.
>>
>> Jeff's test was modeled after real use cases: widely used, legacy
>> programs like dump.
>> Since we often said that splitting the sequential stream in multiple
>> threads was not the
>> correct approach, and we did introduce the change in the kernel just
>> to support those
>> programs (not to encourage writing more of this league), we can assume
>> that if they
>> do not drive deeper queues, no one will. So the overhead is just
>> overhead, and will never
>> give any benefit.
>>
>> I therefore want to remove it, since for SSD it matters.
>> >
>> > We also don't have any data/numbers what kind of cpu savings does this
>> > patch bring in.
>>
>> Jeff's test showed larger bandwidth with merge disabled, so it implies
>> some saving is present.
>
> Following is what Jeff had posted.
>
> ==> vanilla <==
> Mean: 163.22728
> Population Std. Dev.: 0.55401
>
> ==> patched <==
> Mean: 162.91558
> Population Std. Dev.: 1.08612
>
>
> I see that with patched kernel(your patches), "Mean" BW of 50 runs has gone
> down slightly. So where is the improvement in terms of BW? (Are you referring
> to higher standard deviation, that means some of the runs observed higher BW
> and concluding something from that?)

Sorry, I wrongly remembered the numbers where the opposite.

Corrado

>
> Vivek
>
>>
>> Thanks,
>> Corrado
>>
>> >
>> > Vivek
>> >
>> >>
>> >> As you said, most workloads don't benefit from queue merging. On those
>> >> ones, the patch
>> >> just removes an overhead.
>> >>
>> >> Thanks,
>> >> Corrado
>> >>
>> >> >        Jeff
>> >> >
>> >> >
>> >> >
>> >> >