[PATCH v7 09/26] virtio_ring: split: implement virtqueue_reset_vring_split()

[Jason Wang]

在 2022/3/10 下午12:46, Xuan Zhuo 写道:
> On Wed, 9 Mar 2022 15:55:44 +0800, Jason Wang <jasowang at redhat.com> wrote:
>> 在 2022/3/8 下午8:35, Xuan Zhuo 写道:
>>> virtio ring supports reset.
>>>
>>> Queue reset is divided into several stages.
>>>
>>> 1. notify device queue reset
>>> 2. vring release
>>> 3. attach new vring
>>> 4. notify device queue re-enable
>>>
>>> After the first step is completed, the vring reset operation can be
>>> performed. If the newly set vring num does not change, then just reset
>>> the vq related value.
>>>
>>> Otherwise, the vring will be released and the vring will be reallocated.
>>> And the vring will be attached to the vq. If this process fails, the
>>> function will exit, and the state of the vq will be the vring release
>>> state. You can call this function again to reallocate the vring.
>>>
>>> In addition, vring_align, may_reduce_num are necessary for reallocating
>>> vring, so they are retained when creating vq.
>>>
>>> Signed-off-by: Xuan Zhuo <xuanzhuo at linux.alibaba.com>
>>> ---
>>>    drivers/virtio/virtio_ring.c | 69 ++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 69 insertions(+)
>>>
>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>>> index e0422c04c903..148fb1fd3d5a 100644
>>> --- a/drivers/virtio/virtio_ring.c
>>> +++ b/drivers/virtio/virtio_ring.c
>>> @@ -158,6 +158,12 @@ struct vring_virtqueue {
>>>    			/* DMA address and size information */
>>>    			dma_addr_t queue_dma_addr;
>>>    			size_t queue_size_in_bytes;
>>> +
>>> +			/* The parameters for creating vrings are reserved for
>>> +			 * creating new vrings when enabling reset queue.
>>> +			 */
>>> +			u32 vring_align;
>>> +			bool may_reduce_num;
>>>    		} split;
>>>
>>>    		/* Available for packed ring */
>>> @@ -217,6 +223,12 @@ struct vring_virtqueue {
>>>    #endif
>>>    };
>>>
>>> +static void vring_free(struct virtqueue *vq);
>>> +static void __vring_virtqueue_init_split(struct vring_virtqueue *vq,
>>> +					 struct virtio_device *vdev);
>>> +static int __vring_virtqueue_attach_split(struct vring_virtqueue *vq,
>>> +					  struct virtio_device *vdev,
>>> +					  struct vring vring);
>>>
>>>    /*
>>>     * Helpers.
>>> @@ -1012,6 +1024,8 @@ static struct virtqueue *vring_create_virtqueue_split(
>>>    		return NULL;
>>>    	}
>>>
>>> +	to_vvq(vq)->split.vring_align = vring_align;
>>> +	to_vvq(vq)->split.may_reduce_num = may_reduce_num;
>>>    	to_vvq(vq)->split.queue_dma_addr = vring.dma_addr;
>>>    	to_vvq(vq)->split.queue_size_in_bytes = vring.queue_size_in_bytes;
>>>    	to_vvq(vq)->we_own_ring = true;
>>> @@ -1019,6 +1033,59 @@ static struct virtqueue *vring_create_virtqueue_split(
>>>    	return vq;
>>>    }
>>>
>>> +static int virtqueue_reset_vring_split(struct virtqueue *_vq, u32 num)
>>> +{
>>
>> So what this function does is to resize the virtqueue actually, I
>> suggest to rename it as virtqueue_resize_split().
> In addition to resize, when num is 0, the function is to reinitialize vq ring
> related variables. For example avail_idx_shadow.


We need to move those logic to virtio_reset_vq() (I think we agree to 
have a better name of it).


> So I think 'reset' is more appropriate.


The name is confusing at least to me, since we've already had 
virtio_reset_vq() and most of the logic is to do the resize.

Thanks


>
> Thanks.
>
>>
>>> +	struct vring_virtqueue *vq = to_vvq(_vq);
>>> +	struct virtio_device *vdev = _vq->vdev;
>>> +	struct vring_split vring;
>>> +	int err;
>>> +
>>> +	if (num > _vq->num_max)
>>> +		return -E2BIG;
>>> +
>>> +	switch (vq->vq.reset) {
>>> +	case VIRTIO_VQ_RESET_STEP_NONE:
>>> +		return -ENOENT;
>>> +
>>> +	case VIRTIO_VQ_RESET_STEP_VRING_ATTACH:
>>> +	case VIRTIO_VQ_RESET_STEP_DEVICE:
>>> +		if (vq->split.vring.num == num || !num)
>>> +			break;
>>> +
>>> +		vring_free(_vq);
>>> +
>>> +		fallthrough;
>>> +
>>> +	case VIRTIO_VQ_RESET_STEP_VRING_RELEASE:
>>> +		if (!num)
>>> +			num = vq->split.vring.num;
>>> +
>>> +		err = vring_create_vring_split(&vring, vdev,
>>> +					       vq->split.vring_align,
>>> +					       vq->weak_barriers,
>>> +					       vq->split.may_reduce_num, num);
>>> +		if (err)
>>> +			return -ENOMEM;
>>
>> We'd better need a safe fallback here like:
>>
>> If we can't allocate new memory, we can keep using the current one.
>> Otherwise an ethtool -G fail may make the device not usable.
>>
>> This could be done by not freeing the old vring and virtqueue states
>> until new is allocated.
>>
>>
>>> +
>>> +		err = __vring_virtqueue_attach_split(vq, vdev, vring.vring);
>>> +		if (err) {
>>> +			vring_free_queue(vdev, vring.queue_size_in_bytes,
>>> +					 vring.queue,
>>> +					 vring.dma_addr);
>>> +			return -ENOMEM;
>>> +		}
>>> +
>>> +		vq->split.queue_dma_addr = vring.dma_addr;
>>> +		vq->split.queue_size_in_bytes = vring.queue_size_in_bytes;
>>> +	}
>>> +
>>> +	__vring_virtqueue_init_split(vq, vdev);
>>> +	vq->we_own_ring = true;
>>
>> This seems wrong, we have the transport (rproc/mlxtbf) that allocate the
>> vring by themselves. I think we need to fail the resize for we_own_ring
>> == false.
>>
>> Thanks
>>
>>
>>
>>> +	vq->vq.reset = VIRTIO_VQ_RESET_STEP_VRING_ATTACH;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>
>>>    /*
>>>     * Packed ring specific functions - *_packed().
>>> @@ -2317,6 +2384,8 @@ static int __vring_virtqueue_attach_split(struct vring_virtqueue *vq,
>>>    static void __vring_virtqueue_init_split(struct vring_virtqueue *vq,
>>>    					 struct virtio_device *vdev)
>>>    {
>>> +	vq->vq.reset = VIRTIO_VQ_RESET_STEP_NONE;
>>> +
>>>    	vq->packed_ring = false;
>>>    	vq->we_own_ring = false;
>>>    	vq->broken = false;