Re: Re: Re: [PATCH 1/2] virtio: abstract virtqueue related methods
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 Signed-off-by: zhenwei pi --- 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! -- zhen
Re: Re: [PATCH 1/2] virtio: abstract virtqueue related methods
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 > > > Signed-off-by: zhenwei pi > > > --- > > > 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_
Re: Re: [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 Signed-off-by: zhenwei pi --- 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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization