Re: [Qemu-devel] [PATCH] linux-aio: keep processing events if MAX_EVENTS reached

[Roman Penyaev]

On Tue, Jul 12, 2016 at 4:59 PM, Paolo Bonzini <address@hidden> wrote:
>
>
> On 12/07/2016 16:12, Roman Penyaev wrote:
>> But what is the most important thing here is, that reverting
>> "linux-aio: Cancel BH if not needed" brings these numbers:
>>
>>    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
>>
>> So, it seems to me that "linux-aio: Cancel BH if not needed" introduces
>> regression.
>
> It's possible that this depends on the workload.  But, thanks for
> measuring it.  It's not a small effect (~5%).  We should consider indeed
> reverting the patch.

Without any doubts it depends.  But I can't say that my fio config
is something special, take a look:

[global]
bssplit=512/20:1k/16:2k/9:4k/12:8k/19:16k/10:32k/8:64k/4
fadvise_hint=0
rw=randrw:2
direct=1

ioengine=libaio
iodepth=64
iodepth_batch_submit=64
iodepth_batch_complete=64
numjobs=8
gtod_reduce=1
group_reporting=1

time_based=1
runtime=30

[job]
filename=/dev/vda

>
>> -    unsigned int n;
>> +    unsigned int inqueue;
>> +    unsigned int inflight;
>
> Please use in_queue (or queue_length) and in_flight.

Yeah, but that was just a quick exercise.
If Stefan or you want this as a normal patch, I will resend
with all tweaks made.

>
>> @@ -203,9 +206,12 @@ static void ioq_submit(LinuxAioState *s)
>>
>>      do {
>>          len = 0;
>> +        if (s->io_q.inflight >= MAX_EVENTS)
>> +            break;
>>          QSIMPLEQ_FOREACH(aiocb, &s->io_q.pending, next) {
>>              iocbs[len++] = &aiocb->iocb;
>> -            if (len == MAX_QUEUED_IO) {
>> +            if (len == MAX_QUEUED_IO ||
>> +                (s->io_q.inflight + len) >= MAX_EVENTS) {
>
> Interesting!
>
> I suggest an additional cleanup here, which is to replace MAX_QUEUED_IO
> with MAX_EVENTS everywhere and remove the "len == MAX_QUEUED_IO" part.

Yes, that makes sense.

>
> Also, please remove the parentheses on the left of >=.

Ok.

>
>>      if (!s->io_q.blocked &&
>> -        (!s->io_q.plugged || s->io_q.n >= MAX_QUEUED_IO)) {
>> +        (!s->io_q.plugged || s->io_q.inqueue >= MAX_QUEUED_IO)) {
>
> This should now be
>
>     s->io_q.inflight + s->io_q.inqueue >= MAX_EVENTS
>
> (the logic is: if we can do a "full" io_submit, do it).

Ok.

--
Roman