Re: Re: Re: [PATCH 1/2] virtio: abstract virtqueue related methods

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 5/12/23 19:35, Michael S. Tsirkin wrote:
On Fri, May 12, 2023 at 07:09:40PM +0800, zhenwei pi wrote:
On 5/12/23 18:46, Michael S. Tsirkin wrote:
On Fri, May 12, 2023 at 05:46:17PM +0800, zhenwei pi wrote:
There is already a virtqueue abstract structure in virtio subsystem
(see struct virtqueue in include/linux/virtio.h), however the vring
based virtqueue is used only in the past years, the virtqueue related
methods mix much with vring(just look like the codes, virtqueue_xxx
functions are implemented in virtio_ring.c).

Abstract virtqueue related methods(see struct virtqueue_ops), and
separate virtqueue_xxx symbols from vring. This allows a non-vring
based transport in the future. With this change, the following symbols
are exported from virtio.ko instead of virtio_ring.ko:
    virtqueue_add_sgs
    virtqueue_add_outbuf
    virtqueue_add_inbuf
    virtqueue_add_inbuf_ctx
    virtqueue_kick_prepare
    virtqueue_notify
    virtqueue_kick
    virtqueue_enable_cb_prepare
    virtqueue_enable_cb
    virtqueue_enable_cb_delayed
    virtqueue_disable_cb
    virtqueue_poll
    virtqueue_get_buf_ctx
    virtqueue_get_buf
    virtqueue_detach_unused_buf
    virtqueue_get_vring_size
    virtqueue_resize
    virtqueue_is_broken
    virtio_break_device
    __virtio_unbreak_device

Cc: Stefan Hajnoczi <stefanha@xxxxxxxxxx>
Signed-off-by: zhenwei pi <pizhenwei@xxxxxxxxxxxxx>
---
   drivers/virtio/virtio.c      | 362 +++++++++++++++++++++++++++++++++++
   drivers/virtio/virtio_ring.c | 282 +++++----------------------
   include/linux/virtio.h       |  29 +++
   3 files changed, 443 insertions(+), 230 deletions(-)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 3893dc29eb26..8d8715a10f26 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -553,6 +553,368 @@ int virtio_device_restore(struct virtio_device *dev)
   EXPORT_SYMBOL_GPL(virtio_device_restore);
   #endif
+/**
+ * virtqueue_add_sgs - expose buffers to other end
+ * @vq: the struct virtqueue we're talking about.
+ * @sgs: array of terminated scatterlists.
+ * @out_sgs: the number of scatterlists readable by other side
+ * @in_sgs: the number of scatterlists which are writable (after readable ones)
+ * @data: the token identifying the buffer.
+ * @gfp: how to do memory allocations (if necessary).
+ *
+ * Caller must ensure we don't call this with other virtqueue operations
+ * at the same time (except where noted).
+ *
+ * Returns zero or a negative error (ie. ENOSPC, ENOMEM, EIO).
+ */
+int virtqueue_add_sgs(struct virtqueue *vq, struct scatterlist *sgs[],
+		      unsigned int out_sgs, unsigned int in_sgs,
+		      void *data, gfp_t gfp)
+{
+	unsigned int i, total_sg = 0;
+
+	/* Count them first. */
+	for (i = 0; i < out_sgs + in_sgs; i++) {
+		struct scatterlist *sg;
+
+		for (sg = sgs[i]; sg; sg = sg_next(sg))
+			total_sg++;
+	}
+	return vq->ops->add_sgs(vq, sgs, total_sg, out_sgs, in_sgs,
+				data, NULL, gfp);
+}
+EXPORT_SYMBOL_GPL(virtqueue_add_sgs);


Hmm this kind of indirection on data path is going to be costly
performance-wise especially when retpolines are in place.

Any data on this?


Hi,

1, What about moving these functions into virtio.h and declare them as
static inline?

This will do nothing to remove indirection.

2, what about moving method fields into struct virtqueue?

This gets rid of one level of indirection but the big problem
is indirect function call due to retpolines, this remains.


Then we have struct like:
struct virtqueue {
	struct list_head list;
	...
	void *priv;

	/* virtqueue specific operations */
         int (*add_sgs)(struct virtqueue *vq, struct scatterlist *sgs[],
                        unsigned int total_sg,
                        unsigned int out_sgs, unsigned int in_sgs,
                        void *data, void *ctx, gfp_t gfp);
	...
}

and functions like:
static inline int virtqueue_add_sgs(...)
{
         unsigned int i, total_sg = 0;

         /* Count them first. */
         for (i = 0; i < out_sgs + in_sgs; i++) {
                 struct scatterlist *sg;

                 for (sg = sgs[i]; sg; sg = sg_next(sg))
                         total_sg++;
         }
         return vq->add_sgs(vq, sgs, total_sg, out_sgs, in_sgs,
                            data, NULL, gfp);
}

Maybe a flag in vq:
	bool abstract; /* use ops to add/get bufs and kick */
and then
	if (unlikely(vq->abstract))
		 return vq->ops->add_sgs(vq, sgs, total_sg, out_sgs, in_sgs,
				    	 data, NULL, gfp);

transport then just sets the ops if it wants abstract vqs,
and core then skips the vring.


If [1] is acceptable, we can also reduce changes in patch 'tools/virtio:
implement virtqueue in test'.

Yea that one shouldn't be there.

--
zhenwei pi


OK, I'll try and send a next version a few days later. Thanks!

--
zhenwei pi
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization



[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux