Re: [PATCH v4 3/8] vhost: Expose vhost_svq_available_slots()

[Hawkins Jiawei <yin31149@gmail.com>]

在 2023/10/4 01:44, Eugenio Perez Martin 写道:
> On Tue, Aug 29, 2023 at 7:55 AM Hawkins Jiawei <yin31149@gmail.com> wrote:
>>
>> Next patches in this series will delay the polling
>> and checking of buffers until either the SVQ is
>> full or control commands shadow buffers are full,
>> no longer perform an immediate poll and check of
>> the device's used buffers for each CVQ state load command.
>>
>> To achieve this, this patch exposes
>> vhost_svq_available_slots() and introduces a helper function,
>> allowing QEMU to know whether the SVQ is full.
>>
>> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
>> Acked-by: Eugenio Pérez <eperezma@redhat.com>
>> ---
>>   hw/virtio/vhost-shadow-virtqueue.c | 2 +-
>>   hw/virtio/vhost-shadow-virtqueue.h | 1 +
>>   net/vhost-vdpa.c                   | 9 +++++++++
>>   3 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
>> index e731b1d2ea..fc5f408f77 100644
>> --- a/hw/virtio/vhost-shadow-virtqueue.c
>> +++ b/hw/virtio/vhost-shadow-virtqueue.c
>> @@ -66,7 +66,7 @@ bool vhost_svq_valid_features(uint64_t features, Error **errp)
>>    *
>>    * @svq: The svq
>>    */
>> -static uint16_t vhost_svq_available_slots(const VhostShadowVirtqueue *svq)
>> +uint16_t vhost_svq_available_slots(const VhostShadowVirtqueue *svq)
>>   {
>>       return svq->num_free;
>>   }
>> diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h
>> index 5bce67837b..19c842a15b 100644
>> --- a/hw/virtio/vhost-shadow-virtqueue.h
>> +++ b/hw/virtio/vhost-shadow-virtqueue.h
>> @@ -114,6 +114,7 @@ typedef struct VhostShadowVirtqueue {
>>
>>   bool vhost_svq_valid_features(uint64_t features, Error **errp);
>>
>> +uint16_t vhost_svq_available_slots(const VhostShadowVirtqueue *svq);
>>   void vhost_svq_push_elem(VhostShadowVirtqueue *svq,
>>                            const VirtQueueElement *elem, uint32_t len);
>>   int vhost_svq_add(VhostShadowVirtqueue *svq, const struct iovec *out_sg,
>
> I think it is ok to split this export in its own patch. If you decide
> to do it that way, you can add my Acked-by.

I will split this in its own patch, thanks for your suggestion!

>
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index a875767ee9..e6342b213f 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -620,6 +620,13 @@ static ssize_t vhost_vdpa_net_cvq_add(VhostVDPAState *s,
>>       return vhost_svq_poll(svq, 1);
>>   }
>>
>> +/* Convenience wrapper to get number of available SVQ descriptors */
>> +static uint16_t vhost_vdpa_net_svq_available_slots(VhostVDPAState *s)
>> +{
>> +    VhostShadowVirtqueue *svq = g_ptr_array_index(s->vhost_vdpa.shadow_vqs, 0);
>
> This is not really generic enough for all VhostVDPAState, as dataplane
> ones have two svqs.
>
> I think the best is to just inline the function in the caller, as
> there is only one, isn't it? If not, would it work to just replace
> _net_ by _cvq_ or similar?
>

Yes, there should be only one user for this function, I will inline
the function in the caller.

>> +    return vhost_svq_available_slots(svq);
>> +}
>> +
>>   static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, uint8_t class,
>>                                          uint8_t cmd, const struct iovec *data_sg,
>>                                          size_t data_num)
>> @@ -640,6 +647,8 @@ static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, uint8_t class,
>>       };
>>
>>       assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - sizeof(ctrl));
>> +    /* Each CVQ command has one out descriptor and one in descriptor */
>> +    assert(vhost_vdpa_net_svq_available_slots(s) >= 2);
>>
>
> I think we should remove this assertion. By the end of the series
> there is an "if" checks explicitly for the opposite condition, and
> flushing the queue in that case, so the code can never reach it.
>

Yes, you are right. I will remove this assertion.

Thanks!


>>       /* pack the CVQ command header */
>>       memcpy(s->cvq_cmd_out_buffer, &ctrl, sizeof(ctrl));
>> --
>> 2.25.1
>>
>