Virtualization - May 2023 - [PATCH 1/2] virtio: abstract virtqueue related methods

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 at redhat.com>
>> Signed-off-by: zhenwei pi <pizhenwei at bytedance.com>
>> ---
>>   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

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 at redhat.com>
> > > Signed-off-by: zhenwei pi <pizhenwei at bytedance.com>
> > > ---
> > >   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