Re: [RFC v3 1/7] vhost: expose used buffers

----- Original Message -----

From: "jasowang" <redacted>
To: "Guo Zhi" <redacted>, "eperezma" <eperezma@redhat.com>, "sgarzare" <sgarzare@redhat.com>, "Michael
Tsirkin" [off-list ref]
Cc: "netdev" <redacted>, "linux-kernel" <redacted>, "kvm list" <redacted>,
"virtualization" [off-list ref]
Sent: Wednesday, September 7, 2022 12:21:06 PM
Subject: Re: [RFC v3 1/7] vhost: expose used buffers

在 2022/9/1 13:54, Guo Zhi 写道:

quoted

Follow VIRTIO 1.1 spec, only writing out a single used ring for a batch
of descriptors.

Signed-off-by: Guo Zhi <redacted>
---
  drivers/vhost/vhost.c | 16 +++++++++++++---
  1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 40097826cff0..26862c8bf751 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c

@@ -2376,10 +2376,20 @@ static int __vhost_add_used_n(struct vhost_virtqueue

*vq,
  	vring_used_elem_t __user *used;
  	u16 old, new;
  	int start;
+	int copy_n = count;
  
+	/**
+	 * If in order feature negotiated, devices can notify the use of a batch of
buffers to
+	 * the driver by only writing out a single used ring entry with the id
corresponding
+	 * to the head entry of the descriptor chain describing the last buffer in the
batch.
+	 */
+	if (vhost_has_feature(vq, VIRTIO_F_IN_ORDER)) {
+		copy_n = 1;
+		heads = &heads[count - 1];
+	}

Would it better to have a dedicated helper like
vhost_add_used_in_order() here?

That would be much more convenient and clear to implement. 
I think have a dedicated function for in order feature in vhost is better.

quoted

  	start = vq->last_used_idx & (vq->num - 1);
  	used = vq->used->ring + start;
-	if (vhost_put_used(vq, heads, start, count)) {
+	if (vhost_put_used(vq, heads, start, copy_n)) {
  		vq_err(vq, "Failed to write used");
  		return -EFAULT;
  	}

@@ -2388,7 +2398,7 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq,
  		smp_wmb();
  		/* Log used ring entry write. */
  		log_used(vq, ((void __user *)used - (void __user *)vq->used),
-			 count * sizeof *used);
+			 copy_n * sizeof(*used));
  	}
  	old = vq->last_used_idx;
  	new = (vq->last_used_idx += count);

@@ -2410,7 +2420,7 @@ int vhost_add_used_n(struct vhost_virtqueue *vq, struct

vring_used_elem *heads,
  
  	start = vq->last_used_idx & (vq->num - 1);
  	n = vq->num - start;
-	if (n < count) {
+	if (n < count && !vhost_has_feature(vq, VIRTIO_F_IN_ORDER)) {

This seems strange, any reason for this? (Actually if we support
in-order we only need one used slot which fit for the case here)

Thanks

If in order feature negotiated, even the count is larger than n,
we don't need to call __vhost_add_used_n again, because in order 
only use one slot.

Thanks

quoted

  		r = __vhost_add_used_n(vq, heads, n);
  		if (r < 0)
  			return r;