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

[zhenwei pi]

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?
2, what about moving method fields into struct virtqueue?

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);
}

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

--
zhenwei pi