Re: [Qemu-devel] [PATCH V2 1/1] linux-aio: prevent submitting more than MAX_EVENTS

[Roman Penyaev]

On Wed, Jul 13, 2016 at 1:45 PM, Kevin Wolf <address@hidden> wrote:
> Am 13.07.2016 um 13:33 hat Roman Penyaev geschrieben:
>> Just to be sure that we are on the same page:
>>
>> 1. We have this commit "linux-aio: Cancel BH if not needed" which
>>
>>    a) introduces performance regression on my fio workloads on the
>>       following config: "iothread=1, VCPU=8, MQ=8". Performance
>>       dropped from 1878MB/s to 1606MB/s with Stefan's fix, that is
>>       ~14%.
>
> Do we already understand why the performance regresses with the patch?

This is very good question from the author of the patch.  Speaking for
myself, I do not understand.

> As long as we don't, everything we do is just guesswork.

Kevin, did you miss the ending "with Stefan's fix" ?  Since your patch
reproduces another problem it is impossible to test it isolated or IO
hangs.

I tested four variations and invite you to do the same, since you are
the author of the debatable patch:

1. as-is, i.e.
   + Stefan's "virtio-blk: dataplane multiqueue support"
   + yours "linux-aio: Cancel BH if not needed"

   As we already discussed - IO hangs.

2. + Stefan's "linux-aio: keep processing events if MAX_EVENTS reached"

   READ: io=48199MB, aggrb=1606.5MB/s, minb=1606.5MB/s,
maxb=1606.5MB/s, mint=30003msec, maxt=30003msec
  WRITE: io=48056MB, aggrb=1601.8MB/s, minb=1601.8MB/s,
maxb=1601.8MB/s, mint=30003msec, maxt=30003msec

3. - Stefan's "linux-aio: keep processing events if MAX_EVENTS reached"
   + my "linux-aio: prevent submitting more than MAX_EVENTS"

   READ: io=53294MB, aggrb=1776.3MB/s, minb=1776.3MB/s,
maxb=1776.3MB/s, mint=30003msec, maxt=30003msec
  WRITE: io=53177MB, aggrb=1772.4MB/s, minb=1772.4MB/s,
maxb=1772.4MB/s, mint=30003msec, maxt=30003msec

4. - my "linux-aio: prevent submitting more than MAX_EVENTS"
   - yours "linux-aio: Cancel BH if not needed"

   READ: io=56362MB, aggrb=1878.4MB/s, minb=1878.4MB/s,
maxb=1878.4MB/s, mint=30007msec, maxt=30007msec
  WRITE: io=56255MB, aggrb=1874.8MB/s, minb=1874.8MB/s,
maxb=1874.8MB/s, mint=30007msec, maxt=30007msec

The drop from 1878MB/s to 1776MB/s is ~5% and probably can be ignored
(I say probably because would be nice to have some other numbers from
 you or anybody else)

The drop from 1878MB/s to 1606Mb/s is ~14% and seems like a serious
degradation.

Also to go deeper and to avoid possible suspicions I tested isolated
Stefan's "linux-aio: keep processing events if MAX_EVENTS reached"
patch, without yours "linux-aio: Cancel BH if not needed":

   READ: io=109970MB, aggrb=1832.8MB/s, minb=1832.8MB/s,
maxb=1832.8MB/s, mint=60003msec, maxt=60003msec
  WRITE: io=109820MB, aggrb=1830.3MB/s, minb=1830.3MB/s,
maxb=1830.3MB/s, mint=60003msec, maxt=60003msec

As you can see no significant drop.  That means that only the following pair:

   Stefan's "linux-aio: keep processing events if MAX_EVENTS reached"
   yours "linux-aio: Cancel BH if not needed"

impacts the performance.

As I already told we have different choices to follow and one of them
(the simplest) is to revert everything.

--
Roman

>
> Kevin
>
>>    b) reproduces IO hang, because of in-flights > MAX_EVENTS.
>>
>>  So probably this commit should be reverted because of a) not b).
>>
>> 2. Stefan has fix for 1.b) issue repeating io_getevents(), which
>>    is obviously an excess for generic cases where MQ=1 (queue depth
>>    for virtio_blk is also set to 128 i.e. equal to MAX_EVENTS on
>>    QEMU side).
>>
>> 3. The current patch also aims to fix 1.b) issue restricting number
>>    of in-flights.
>>
>>
>> Reverting 1. will fix all the problems, without any need to apply
>> 2. or 3.  The most lazy variant.
>>
>> Restricting in-flights is also a step forward, since submitting
>> till -EAGAIN is also possible, but leads (as we already know) to
>> IO hang on specific loads and conditions.
>>
>> But 2. and 3. are mutual exclusive and should not be applied
>> together.
>>
>> So we have several alternatives and a choice what to follow.
>>
>> --
>> Roman