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

[zhenwei pi <pizhenwei@bytedance.com>]

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

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

-- 
zhenwei pi