Re: [RFC PATCH 13/13] HACK: schedule fence return on main AIO context

[Stefan Hajnoczi <stefanha@gmail.com>]

On Thu, 20 Apr 2023 at 21:13, Gurchetan Singh
<gurchetansingh@chromium.org> wrote:
>
> gfxstream and both cross-domain (and even newer versions
> virglrenderer: see VIRGL_RENDERER_ASYNC_FENCE_CB) like to signal
> fence completion on threads ("callback threads") that are
> different from the thread that processes the command queue
> ("main thread").
>
> This is generally possible with locking, and this what we do
> in crosvm and other virtio-gpu1.1 implementations.  However, on
> QEMU a deadlock is observed if virtio_gpu_ctrl_response_nodata(..)
> [used in the fence callback] is used from a thread that is not the
> main thread.
>
> The reason is the main thread takes the big QEMU lock (bql) somewhere
> when processing the command queue, and virtio_gpu_ctrl_response_nodata(..)
> needs that lock.  If you add in the lock needed to protect &g->fenceq
> from concurrent access by the main thread and the callback threads,
> you end can end up with deadlocks.
>
> It's possible to workaround this by scheduling the return of the fence
> descriptors via aio_bh_schedule_oneshot_full(..), but that somewhat
> negates the rationale for the asynchronous callbacks.
>
> I also played around with aio_context_acquire()/aio_context_release(),
> doesn't seem to help.
>
> Is signaling the virtio_queue outside of the main thread possible?  If
> so, how?

Yes, if you want a specific thread to process a virtqueue, monitor the
virtqueue's host_notifier (aka ioeventfd) from the thread. That's how
--device virtio-blk,iothread= works. It attaches the host_notifier to
the IOThread's AioContext. Whenever the guest kicks the virtqueue, the
IOThread will respond instead of QEMU's main loop thread.

That said, I don't know the threading model of your code. Could you
explain which threads are involved? Do you want to process all buffers
from virtqueue in a specific thread or only these fence buffers?

>
> Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> ---
>  hw/display/virtio-gpu-rutabaga.c | 29 ++++++++++++++++++++++++++---
>  include/hw/virtio/virtio-gpu.h   |  1 +
>  2 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-rutabaga.c b/hw/display/virtio-gpu-rutabaga.c
> index 196267aac2..5c296aeef1 100644
> --- a/hw/display/virtio-gpu-rutabaga.c
> +++ b/hw/display/virtio-gpu-rutabaga.c
> @@ -31,6 +31,11 @@ static int virtio_gpu_rutabaga_init(VirtIOGPU *g);
>
>  #define CHECK_RESULT(result, cmd) CHECK(result == 0, cmd)
>
> +struct rutabaga_aio_data {
> +    struct VirtIOGPUGL *virtio_gpu;
> +    struct rutabaga_fence fence;
> +};
> +
>  static void
>  virtio_gpu_rutabaga_update_cursor(VirtIOGPU *g, struct virtio_gpu_scanout *s,
>                                    uint32_t resource_id)
> @@ -823,10 +828,11 @@ virtio_gpu_rutabaga_process_cmd(VirtIOGPU *g,
>  }
>
>  static void
> -virtio_gpu_rutabaga_fence_cb(uint64_t user_data,
> -                             struct rutabaga_fence fence_data)
> +virtio_gpu_rutabaga_aio_cb(void *opaque)
>  {
> -    VirtIOGPU *g = (VirtIOGPU *)(void*)(uintptr_t)user_data;
> +    struct rutabaga_aio_data *data =  (struct rutabaga_aio_data *)opaque;
> +    VirtIOGPU *g = (VirtIOGPU *)data->virtio_gpu;
> +    struct rutabaga_fence fence_data = data->fence;
>      struct virtio_gpu_ctrl_command *cmd, *tmp;
>
>      bool signaled_ctx_specific = fence_data.flags & RUTABAGA_FLAG_INFO_RING_IDX;
> @@ -856,6 +862,22 @@ virtio_gpu_rutabaga_fence_cb(uint64_t user_data,
>          QTAILQ_REMOVE(&g->fenceq, cmd, next);
>          g_free(cmd);
>      }
> +
> +    g_free(data);
> +}
> +
> +static void
> +virtio_gpu_rutabaga_fence_cb(uint64_t user_data,
> +                             struct rutabaga_fence fence_data) {
> +    struct rutabaga_aio_data *data;
> +    VirtIOGPU *g = (VirtIOGPU *)(void*)(uintptr_t)user_data;
> +    GET_VIRTIO_GPU_GL(g);
> +
> +    data = g_new0(struct rutabaga_aio_data, 1);
> +    data->virtio_gpu = virtio_gpu;
> +    data->fence = fence_data;
> +    aio_bh_schedule_oneshot_full(virtio_gpu->ctx, virtio_gpu_rutabaga_aio_cb,
> +                                 (void *)data, "aio");
>  }
>
>  static int virtio_gpu_rutabaga_init(VirtIOGPU *g)
> @@ -912,6 +934,7 @@ static int virtio_gpu_rutabaga_init(VirtIOGPU *g)
>          free(channels.channels);
>      }
>
> +    virtio_gpu->ctx = qemu_get_aio_context();
>      return result;
>  }
>
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index 034c71e8f5..b33ad0c68f 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -225,6 +225,7 @@ struct VirtIOGPUGL {
>      char *wayland_socket_path;
>      uint32_t num_capsets;
>      void *rutabaga;
> +    AioContext *ctx;
>  };
>
>  struct VhostUserGPU {
> --
> 2.40.0.634.g4ca3ef3211-goog
>
>