Re: [RFC PATCH 13/27] vhost: Send buffers to device

From: Stefan Hajnoczi <stefanha@gmail.com>
To: "Eugenio Pérez" <eperezma@redhat.com>
Cc: qemu-devel@nongnu.org, Lars Ganrot <lars.ganrot@gmail.com>,
	virtualization@lists.linux-foundation.org,
	Salil Mehta <mehta.salil.lnk@gmail.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Liran Alon <liralon@gmail.com>,
	Rob Miller <rob.miller@broadcom.com>,
	Max Gurtovoy <maxgu14@gmail.com>,
	Alex Barba <alex.barba@broadcom.com>,
	Stefan Hajnoczi <stefanha@redhat.com>,
	Jim Harford <jim.harford@broadcom.com>,
	Jason Wang <jasowang@redhat.com>,
	Harpreet Singh Anand <hanand@xilinx.com>,
	Christophe Fontaine <cfontain@redhat.com>,
	vm <vmireyno@marvell.com>, Daniel Daly <dandaly0@gmail.com>,
	Michael Lilja <ml@napatech.com>,
	Stefano Garzarella <sgarzare@redhat.com>,
	Nitin Shrivastav <nitin.shrivastav@broadcom.com>,
	Lee Ballard <ballle98@gmail.com>,
	Dmytro Kazantsev <dmytro.kazantsev@gmail.com>,
	Juan Quintela <quintela@redhat.com>,
	kvm@vger.kernel.org, Howard Cai <howard.cai@gmail.com>,
	Xiao W Wang <xiao.w.wang@intel.com>,
	Sean Mooney <smooney@redhat.com>,
	Parav Pandit <parav@mellanox.com>, Eli Cohen <eli@mellanox.com>,
	Siwei Liu <loseweigh@gmail.com>,
	Stephen Finucane <stephenfin@redhat.com>
Subject: Re: [RFC PATCH 13/27] vhost: Send buffers to device
Date: Tue, 8 Dec 2020 08:16:21 +0000	[thread overview]
Message-ID: <20201208081621.GR203660@stefanha-x1.localdomain> (raw)
In-Reply-To: <20201120185105.279030-14-eperezma@redhat.com>

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

On Fri, Nov 20, 2020 at 07:50:51PM +0100, Eugenio Pérez wrote:
> -static inline bool vhost_vring_should_kick(VhostShadowVirtqueue *vq)
> +static bool vhost_vring_should_kick_rcu(VhostShadowVirtqueue *vq)

"vhost_vring_" is a confusing name. This is not related to
vhost_virtqueue or the vhost_vring_* structs.

vhost_shadow_vq_should_kick_rcu()?

>  {
> -    return virtio_queue_get_used_notify_split(vq->vq);
> +    VirtIODevice *vdev = vq->vdev;
> +    vq->num_added = 0;

I'm surprised that a bool function modifies state. Is this assignment
intentional?

> +/* virtqueue_add:
> + * @vq: The #VirtQueue
> + * @elem: The #VirtQueueElement

The copy-pasted doc comment doesn't match this function.

> +int vhost_vring_add(VhostShadowVirtqueue *vq, VirtQueueElement *elem)
> +{
> +    int host_head = vhost_vring_add_split(vq, elem);
> +    if (vq->ring_id_maps[host_head]) {
> +        g_free(vq->ring_id_maps[host_head]);
> +    }

VirtQueueElement is freed lazily? Is there a reason for this approach? I
would have expected it to be freed when the used element is process by
the kick fd handler.

> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 9352c56bfa..304e0baa61 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -956,8 +956,34 @@ static void handle_sw_lm_vq(VirtIODevice *vdev, VirtQueue *vq)
>      uint16_t idx = virtio_get_queue_index(vq);
>  
>      VhostShadowVirtqueue *svq = hdev->sw_lm_shadow_vq[idx];
> +    VirtQueueElement *elem;
>  
> -    vhost_vring_kick(svq);
> +    /*
> +     * Make available all buffers as possible.
> +     */
> +    do {
> +        if (virtio_queue_get_notification(vq)) {
> +            virtio_queue_set_notification(vq, false);
> +        }
> +
> +        while (true) {
> +            int r;
> +            if (virtio_queue_full(vq)) {
> +                break;
> +            }

Why is this check necessary? The guest cannot provide more descriptors
than there is ring space. If that happens somehow then it's a driver
error that is already reported in virtqueue_pop() below.

I wonder if you added this because the vring implementation above
doesn't support indirect descriptors? It's easy to exhaust the vhost
hdev vring while there is still room in the VirtIODevice's VirtQueue
vring.

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