[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] linux-aio: keep processing events if MAX_EVENTS
From: |
Roman Penyaev |
Subject: |
Re: [Qemu-devel] [PATCH] linux-aio: keep processing events if MAX_EVENTS reached |
Date: |
Tue, 12 Jul 2016 18:34:55 +0200 |
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