From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Blue Swirl" <blauwirbel@gmail.com>
Subject: Re: [Qemu-devel] [RFC] Replace posix-aio with custom thread pool
Date: Sat, 6 Dec 2008 11:03:30 +0200
Message-ID: <f43fc5580812060103o32c2aec9x5d6bcdc344d0ed8f@mail.gmail.com>
References: <1228512061-25398-1-git-send-email-aliguori@us.ibm.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Cc: "Anthony Liguori" <aliguori@us.ibm.com>,
	kvm-devel <kvm@vger.kernel.org>
To: qemu-devel@nongnu.org
Return-path: <kvm-owner@vger.kernel.org>
Received: from fk-out-0910.google.com ([209.85.128.187]:2378 "EHLO
	fk-out-0910.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751250AbYLFJDc (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sat, 6 Dec 2008 04:03:32 -0500
Received: by fk-out-0910.google.com with SMTP id 18so352428fkq.5
        for <kvm@vger.kernel.org>; Sat, 06 Dec 2008 01:03:30 -0800 (PST)
In-Reply-To: <1228512061-25398-1-git-send-email-aliguori@us.ibm.com>
Content-Disposition: inline
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On 12/5/08, Anthony Liguori <aliguori@us.ibm.com> wrote:
> glibc implements posix-aio as a thread pool and imposes a number of limitations.
>
>  1) it limits one request per-file descriptor.  we hack around this by dup()'ing
>  file descriptors which is hideously ugly
>
>  2) it's impossible to add new interfaces and we need a vectored read/write
>  operation to properly support a zero-copy API.
>
>  What has been suggested to me by glibc folks, is to implement whatever new
>  interfaces we want and then it can eventually be proposed for standardization.
>  This requires that we implement our own posix-aio implementation though.
>
>  This patch implements posix-aio using pthreads.  It immediately eliminates the
>  need for fd pooling.
>
>  It performs at least as well as the current posix-aio code (in some
>  circumstances, even better).
>
>  My only concern here is non-Linux Unices like FreeBSD.  They have kernel support
>  for posix-aio.  Since we cannot extend those interfaces though, I think that
>  even on those platforms we should still use a thread pool.
>
>  Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

>  @@ -895,6 +824,7 @@ BlockDriver bdrv_raw = {
>      .bdrv_aio_cancel = raw_aio_cancel,
>      .aiocb_size = sizeof(RawAIOCB),
>   #endif
>  +
>  @@ -1252,6 +1178,7 @@ BlockDriver bdrv_host_device = {
>      .bdrv_aio_cancel = raw_aio_cancel,
>      .aiocb_size = sizeof(RawAIOCB),
>   #endif
>  +

Some cleanup needed here?

>  +int _compat_aio_init(struct aioinit *aioinit)
>  +static int _compat_aio_submit(struct aiocb *aiocb, int is_write)
>  +int _compat_aio_read(struct aiocb *aiocb)
>  +int _compat_aio_write(struct aiocb *aiocb)
>  +ssize_t _compat_aio_return(struct aiocb *aiocb)
>  +int _compat_aio_error(struct aiocb *aiocb)
>  +int _compat_aio_cancel(int fd, struct aiocb *aiocb)

The names should not begin with an underscore.

>  +struct aiocb
>  +{
>  +    int aio_fildes;
>  +    void *aio_buf;
>  +    size_t aio_nbytes;
>  +    struct sigevent aio_sigevent;
>  +    off_t aio_offset;
>  +
>  +    /* private */
>  +    TAILQ_ENTRY(aiocb) node;
>  +    int is_write;
>  +    ssize_t ret;
>  +    int active;
>  +};
>  +
>  +struct aioinit
>  +{
>  +    int aio_threads;
>  +    int aio_num;
>  +    int aio_idle_time;
>  +};

These structs should probably be named qemu_aiocb and qemu_aioinit to
avoid conflict with system types.

I like to use unsigned types whenever possible, IIRC compilers may
generate better code with those.