Re: [PATCH v3 03/17] virtio: queue_reset: struct virtio_config_ops add callbacks for queue_reset

From: Jason Wang <jasowang@redhat.com>
To: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Jesper Dangaard Brouer <hawk@kernel.org>,
	John Fastabend <john.fastabend@gmail.com>,
	bpf@vger.kernel.org, virtualization@lists.linux-foundation.org,
	netdev@vger.kernel.org
Subject: Re: [PATCH v3 03/17] virtio: queue_reset: struct virtio_config_ops add callbacks for queue_reset
Date: Tue, 8 Feb 2022 10:58:45 +0800	[thread overview]
Message-ID: <b0debb2b-7548-c354-2128-2ddf56bf5c18@redhat.com> (raw)
In-Reply-To: <1644218386.0457659-1-xuanzhuo@linux.alibaba.com>

在 2022/2/7 下午3:19, Xuan Zhuo 写道:
> On Mon, 7 Feb 2022 14:45:02 +0800, Jason Wang <jasowang@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

It looks to me we don't support this right now.

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

Yes, I think for driver we need to implement the function that is needed 
for the first user (e.g AF_XDP). If there's no use case, we can leave 
those extension for the future.

Thanks

>
> 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@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. */