[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 3/8] vhost: Expose vhost_svq_available_slots()
From: |
Hawkins Jiawei |
Subject: |
Re: [PATCH v4 3/8] vhost: Expose vhost_svq_available_slots() |
Date: |
Sun, 8 Oct 2023 09:35:01 +0800 |
在 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
>>
>