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

zhenwei pi
2023-05-12T11:12:13Z

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?
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