Re: [PATCH 3/3] linux-aio: limit the batch size using `aio-max-batch` parameter

[Stefano Garzarella]

On Tue, Jul 13, 2021 at 03:58:04PM +0100, Stefan Hajnoczi wrote:
> On Wed, Jul 07, 2021 at 05:00:19PM +0200, Stefano Garzarella wrote:
> > @@ -371,7 +375,7 @@ static int laio_do_submit(int fd, struct qemu_laiocb 
> > *laiocb, off_t offset,
> >      s->io_q.in_queue++;
> >      if (!s->io_q.blocked &&
> >          (!s->io_q.plugged ||
> > -         s->io_q.in_flight + s->io_q.in_queue >= MAX_EVENTS)) {
> > +         s->io_q.in_queue >= max_batch)) {
>
> Is it safe to drop the MAX_EVENTS case?

I think it is safe since in ioq_submit() we have this check while 
dequeueing:

        QSIMPLEQ_FOREACH(aiocb, &s->io_q.pending, next) {
            iocbs[len++] = &aiocb->iocb;
            if (s->io_q.in_flight + len >= MAX_EVENTS) {
                break;
            }
        }

But in term of performance, I think is better what you're suggesting, 
because if we have fewer slots available than `max_batch`, here we were 
delaying the call to io_submit().

> Perhaps the following can be used:
>
>  int64_t max_batch = s->aio_context->aio_max_batch ?: DEFAULT_MAX_BATCH;
>  max_batch = MIN_NON_ZERO(MAX_EVENTS - s->io_q.in_flight + s->io_q.in_queue, 
> max_batch);

Since we will compare `in_queue` with `max_batch`, should we remove it 
from this expression?

I mean:

  int64_t max_batch = s->aio_context->aio_max_batch ?: DEFAULT_MAX_BATCH;
  max_batch = MIN_NON_ZERO(MAX_EVENTS - s->io_q.in_flight, max_batch);

then as it is in this patch:

  s->io_q.in_queue++;
  if (!s->io_q.blocked &&
      (!s->io_q.plugged ||
       s->io_q.in_queue >= max_batch)) {
      ioq_submit(s);
  }

> Here we'll only need to check against max_batch but it takes into
> account MAX_EVENT and in_flight.

Thanks,
Stefano