Virtualization - Feb 2022 - [PATCH v3 03/17] virtio: queue_reset: struct virtio_config_ops add callbacks for queue_reset

? 2022/1/26 ??3:35, Xuan Zhuo ??:
> Performing reset on a queue is divided into two steps:
>
> 1. reset_vq: reset one vq
> 2. enable_reset_vq: re-enable the reset queue
>
> In the first step, these tasks will be completed:
>      1. notify the hardware queue to reset
>      2. recycle the buffer from vq
>      3. release the ring of the vq
>
> The second step is similar to find vqs,

Not sure, since find_vqs will usually try to allocate interrupts.

>   passing parameters callback and
> name, etc. Based on the original vq, the ring is re-allocated and
> configured to the backend.

I wonder whether we really have such requirement.

For example, do we really have a use case that may change:

vq callback, ctx, ring_num or even re-create the virtqueue?

Thanks

>
> So add two callbacks reset_vq, enable_reset_vq to struct
> virtio_config_ops.
>
> Add a structure for passing parameters. This will facilitate subsequent
> expansion of the parameters of enable reset vq.
> There is currently only one default extended parameter ring_num.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo at linux.alibaba.com>
> ---
>   include/linux/virtio_config.h | 43 ++++++++++++++++++++++++++++++++++-
>   1 file changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 4d107ad31149..51dd8461d1b6 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -16,6 +16,44 @@ struct virtio_shm_region {
>   	u64 len;
>   };
>   
> +typedef void vq_callback_t(struct virtqueue *);
> +
> +/* virtio_reset_vq: specify parameters for queue_reset
> + *
> + *	vdev: the device
> + *	queue_index: the queue index
> + *
> + *	free_unused_cb: callback to free unused bufs
> + *	data: used by free_unused_cb
> + *
> + *	callback: callback for the virtqueue, NULL for vq that do not need a
> + *	          callback
> + *	name: virtqueue names (mainly for debugging), NULL for vq unused by
> + *	      driver
> + *	ctx: ctx
> + *
> + *	ring_num: specify ring num for the vq to be re-enabled. 0 means use the
> + *	          default value. MUST be a power of 2.
> + */
> +struct virtio_reset_vq;
> +typedef void vq_reset_callback_t(struct virtio_reset_vq *param, void
*buf);
> +struct virtio_reset_vq {
> +	struct virtio_device *vdev;
> +	u16 queue_index;
> +
> +	/* reset vq param */
> +	vq_reset_callback_t *free_unused_cb;
> +	void *data;
> +
> +	/* enable reset vq param */
> +	vq_callback_t *callback;
> +	const char *name;
> +	const bool *ctx;
> +
> +	/* ext enable reset vq param */
> +	u16 ring_num;
> +};
> +
>   /**
>    * virtio_config_ops - operations for configuring a virtio device
>    * Note: Do not assume that a transport implements all of the operations
> @@ -74,8 +112,9 @@ struct virtio_shm_region {
>    * @set_vq_affinity: set the affinity for a virtqueue (optional).
>    * @get_vq_affinity: get the affinity for a virtqueue (optional).
>    * @get_shm_region: get a shared memory region based on the index.
> + * @reset_vq: reset a queue individually
> + * @enable_reset_vq: enable a reset queue
>    */
> -typedef void vq_callback_t(struct virtqueue *);
>   struct virtio_config_ops {
>   	void (*enable_cbs)(struct virtio_device *vdev);
>   	void (*get)(struct virtio_device *vdev, unsigned offset,
> @@ -100,6 +139,8 @@ struct virtio_config_ops {
>   			int index);
>   	bool (*get_shm_region)(struct virtio_device *vdev,
>   			       struct virtio_shm_region *region, u8 id);
> +	int (*reset_vq)(struct virtio_reset_vq *param);
> +	struct virtqueue *(*enable_reset_vq)(struct virtio_reset_vq *param);
>   };
>   
>   /* If driver didn't advertise the feature, it will never appear. */

On Mon, 7 Feb 2022 14:45:02 +0800, Jason Wang <jasowang at redhat.com>
wrote:
>
> ? 2022/1/26 ??3:35, Xuan Zhuo ??:
> > Performing reset on a queue is divided into two steps:
> >
> > 1. reset_vq: reset one vq
> > 2. enable_reset_vq: re-enable the reset queue
> >
> > In the first step, these tasks will be completed:
> >      1. notify the hardware queue to reset
> >      2. recycle the buffer from vq
> >      3. release the ring of the vq
> >
> > The second step is similar to find vqs,
>
>
> Not sure, since find_vqs will usually try to allocate interrupts.
>
>
Yes.

> >   passing parameters callback and
> > name, etc. Based on the original vq, the ring is re-allocated and
> > configured to the backend.
>
>
> I wonder whether we really have such requirement.
>
> For example, do we really have a use case that may change:
>
> vq callback, ctx, ring_num or even re-create the virtqueue?
1. virtqueue is not recreated
2. ring_num can be used to modify ring num by ethtool -G

There is really no scene to modify vq callback, ctx, name.

Do you mean we still use the old one instead of adding these parameters?

Thanks.
>
> Thanks
>
>
> >
> > So add two callbacks reset_vq, enable_reset_vq to struct
> > virtio_config_ops.
> >
> > Add a structure for passing parameters. This will facilitate
subsequent
> > expansion of the parameters of enable reset vq.
> > There is currently only one default extended parameter ring_num.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo at linux.alibaba.com>
> > ---
> >   include/linux/virtio_config.h | 43
++++++++++++++++++++++++++++++++++-
> >   1 file changed, 42 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/virtio_config.h
b/include/linux/virtio_config.h
> > index 4d107ad31149..51dd8461d1b6 100644
> > --- a/include/linux/virtio_config.h
> > +++ b/include/linux/virtio_config.h
> > @@ -16,6 +16,44 @@ struct virtio_shm_region {
> >   	u64 len;
> >   };
> >
> > +typedef void vq_callback_t(struct virtqueue *);
> > +
> > +/* virtio_reset_vq: specify parameters for queue_reset
> > + *
> > + *	vdev: the device
> > + *	queue_index: the queue index
> > + *
> > + *	free_unused_cb: callback to free unused bufs
> > + *	data: used by free_unused_cb
> > + *
> > + *	callback: callback for the virtqueue, NULL for vq that do not need
a
> > + *	          callback
> > + *	name: virtqueue names (mainly for debugging), NULL for vq unused
by
> > + *	      driver
> > + *	ctx: ctx
> > + *
> > + *	ring_num: specify ring num for the vq to be re-enabled. 0 means
use the
> > + *	          default value. MUST be a power of 2.
> > + */
> > +struct virtio_reset_vq;
> > +typedef void vq_reset_callback_t(struct virtio_reset_vq *param, void
*buf);
> > +struct virtio_reset_vq {
> > +	struct virtio_device *vdev;
> > +	u16 queue_index;
> > +
> > +	/* reset vq param */
> > +	vq_reset_callback_t *free_unused_cb;
> > +	void *data;
> > +
> > +	/* enable reset vq param */
> > +	vq_callback_t *callback;
> > +	const char *name;
> > +	const bool *ctx;
> > +
> > +	/* ext enable reset vq param */
> > +	u16 ring_num;
> > +};
> > +
> >   /**
> >    * virtio_config_ops - operations for configuring a virtio device
> >    * Note: Do not assume that a transport implements all of the
operations
> > @@ -74,8 +112,9 @@ struct virtio_shm_region {
> >    * @set_vq_affinity: set the affinity for a virtqueue (optional).
> >    * @get_vq_affinity: get the affinity for a virtqueue (optional).
> >    * @get_shm_region: get a shared memory region based on the index.
> > + * @reset_vq: reset a queue individually
> > + * @enable_reset_vq: enable a reset queue
> >    */
> > -typedef void vq_callback_t(struct virtqueue *);
> >   struct virtio_config_ops {
> >   	void (*enable_cbs)(struct virtio_device *vdev);
> >   	void (*get)(struct virtio_device *vdev, unsigned offset,
> > @@ -100,6 +139,8 @@ struct virtio_config_ops {
> >   			int index);
> >   	bool (*get_shm_region)(struct virtio_device *vdev,
> >   			       struct virtio_shm_region *region, u8 id);
> > +	int (*reset_vq)(struct virtio_reset_vq *param);
> > +	struct virtqueue *(*enable_reset_vq)(struct virtio_reset_vq *param);
> >   };
> >
> >   /* If driver didn't advertise the feature, it will never appear.
*/
>