Re: [PATCH RFC 2/2] virtio_ring: support packed ring

On Fri, Mar 16, 2018 at 07:36:47PM +0800, Jason Wang wrote:

quoted

quoted

@@ -1096,17 +1599,21 @@ struct virtqueue *vring_create_virtqueue(

quoted

quoted

quoted

quoted

quoted

     	if (!queue) {
     		/* Try to get a single page. You are my only hope! */
-		queue = vring_alloc_queue(vdev, vring_size(num, vring_align),
+		queue = vring_alloc_queue(vdev, __vring_size(num, vring_align,
+							     packed),
     					  &dma_addr, GFP_KERNEL|__GFP_ZERO);
     	}
     	if (!queue)
     		return NULL;
-	queue_size_in_bytes = vring_size(num, vring_align);
-	vring_init(&vring, num, queue, vring_align);
+	queue_size_in_bytes = __vring_size(num, vring_align, packed);
+	if (packed)
+		vring_packed_init(&vring.vring_packed, num, queue, vring_align);
+	else
+		vring_init(&vring.vring_split, num, queue, vring_align);

Let's rename vring_init to vring_init_split() like other helpers?

The vring_init() is a public API in include/uapi/linux/virtio_ring.h.
I don't think we can rename it.

I see, then this need more thoughts to unify the API.

My thought is to keep the old API as is, and introduce
new types and helpers for packed ring.

I admit it's not a fault of this patch. But we'd better think of this in the
future, consider we may have new kinds of ring.

quoted

More details can be found in this patch:
https://lkml.org/lkml/2018/2/23/243
(PS. The type which has bit fields is just for reference,
   and will be changed in next version.)

Do you have any other suggestions?

No.

Hmm.. Sorry, I didn't describe my question well.
I mean do you have any suggestions about the API
design for packed ring in uapi header? Currently
I introduced below two new helpers:

static inline void vring_packed_init(struct vring_packed *vr, unsigned int num,
				     void *p, unsigned long align);
static inline unsigned vring_packed_size(unsigned int num, unsigned long align);

When new rings are introduced in the future, above
helpers can't be reused. Maybe we should make the
helpers be able to determine the ring type?

Let's wait for Michael's comment here. Generally, I fail to understand why
vring_init() become a part of uapi. Git grep shows the only use cases are
virtio_test/vringh_test.

Thanks

For init - I think it's a mistake that stems from lguest which sometimes
made it less than obvious which code is where.  I don't see a reason to
add to it.

-- 
MST