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

Jason Wang
2022-03-11T05:01:48Z

在 2022/3/10 下午12:46, Xuan Zhuo 写道:
> On Wed, 9 Mar 2022 15:55:44 +0800, Jason Wang <jasowang@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@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;