Re: [PATCH v4] hw/nvme: Use ioeventfd to handle doorbell updates

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 2401 bytes --]

On Jul  5 22:24, Jinhao Fan wrote:
> Add property "ioeventfd" which is enabled by default. When this is
> enabled, updates on the doorbell registers will cause KVM to signal
> an event to the QEMU main loop to handle the doorbell updates.
> Therefore, instead of letting the vcpu thread run both guest VM and
> IO emulation, we now use the main loop thread to do IO emulation and
> thus the vcpu thread has more cycles for the guest VM.
> 

This is not entirely accurate.

Yes, the ioeventfd causes the doorbell write to be handled by the main
iothread, but previously we did not do any substantial device emulation
in the vcpu thread either. That is the reason why we only handle the
bare minimum of the doorbell write and then defer any work until the
timer fires on the main iothread.

But with this patch we just go ahead and do the work (nvme_process_sq)
immediately since we are already in the main iothread.

> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index c952c34f94..4b75c5f549 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -1374,7 +1374,14 @@ static void nvme_enqueue_req_completion(NvmeCQueue *cq, NvmeRequest *req)
>  
>      QTAILQ_REMOVE(&req->sq->out_req_list, req, entry);
>      QTAILQ_INSERT_TAIL(&cq->req_list, req, entry);
> -    timer_mod(cq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
> +
> +    if (req->sq->ioeventfd_enabled) {
> +        /* Post CQE directly since we are in main loop thread */
> +        nvme_post_cqes(cq);
> +    } else {
> +        /* Schedule the timer to post CQE later since we are in vcpu thread */
> +        timer_mod(cq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
> +    }

Actually, we are only in the vcpu thread if we come here from
nvme_process_db that in very rare circumstances may enqueue the
completion of an AER due to an invalid doorbell write.

In general, nvme_enqueue_req_completion is only ever called from the
main iothread. Which actually causes me to wonder why we defer this work
in the first place. It does have the benefit that we queue up several
completions before posting them in one go and raising the interrupt.
But I wonder if that could be handled better.

Anyway, it doesnt affect the correctness of your patch - just an
observation that we should look into possibly.


LGTM,

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]