Re: [PATCH v3 6/8] vdpa: Move vhost_svq_poll() to the caller of vhost_vdpa_net_cvq_add()

[Hawkins Jiawei]

On 2023/8/18 23:48, Eugenio Perez Martin wrote:
> On Wed, Jul 19, 2023 at 9:54 AM Hawkins Jiawei <yin31149@gmail.com> wrote:
>>
>> This patch moves vhost_svq_poll() to the caller of
>> vhost_vdpa_net_cvq_add() and introduces a helper funtion.
>>
>> By making this change, next patches in this series is
>> able to refactor vhost_vdpa_net_load_x() only to delay
>> the polling and checking process until either the SVQ
>> is full or control commands shadow buffers are full.
>>
>> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
>> ---
>>   net/vhost-vdpa.c | 50 ++++++++++++++++++++++++++++++++++++++----------
>>   1 file changed, 40 insertions(+), 10 deletions(-)
>>
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index fe0ba19724..d06f38403f 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -609,15 +609,21 @@ static ssize_t vhost_vdpa_net_cvq_add(VhostVDPAState 
>> *s,
>>               qemu_log_mask(LOG_GUEST_ERROR, "%s: No space on device 
>> queue\n",
>>                             __func__);
>>           }
>> -        return r;
>>       }
>>
>> -    /*
>> -     * We can poll here since we've had BQL from the time we sent the
>> -     * descriptor. Also, we need to take the answer before SVQ pulls by 
>> itself,
>> -     * when BQL is released
>> -     */
>> -    return vhost_svq_poll(svq, 1);
>> +    return r;
>> +}
>> +
>> +/*
>> + * Convenience wrapper to poll SVQ for multiple control commands.
>> + *
>> + * Caller should hold the BQL when invoking this function, and should take
>> + * the answer before SVQ pulls by itself when BQL is released.
>> + */
>> +static ssize_t vhost_vdpa_net_svq_poll(VhostVDPAState *s, size_t 
>> cmds_in_flight)
>> +{
>> +    VhostShadowVirtqueue *svq = g_ptr_array_index(s->vhost_vdpa.shadow_vqs, 
>> 0);
>> +    return vhost_svq_poll(svq, cmds_in_flight);
>>   }
>>
>>   /* Convenience wrapper to get number of available SVQ descriptors */
>> @@ -645,6 +651,7 @@ static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState 
>> *s, uint8_t class,
>>           .iov_base = s->status,
>>           .iov_len = sizeof(*s->status),
>>       };
>> +    ssize_t r;
>>
>>       assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - sizeof(ctrl));
>>       /* Each CVQ command has one out descriptor and one in descriptor */
>> @@ -657,7 +664,16 @@ static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState 
>> *s, uint8_t class,
>>       iov_to_buf(data_sg, data_num, 0,
>>                  s->cvq_cmd_out_buffer + sizeof(ctrl), data_size);
>>
>> -    return vhost_vdpa_net_cvq_add(s, &out, 1, &in, 1);
>> +    r = vhost_vdpa_net_cvq_add(s, &out, 1, &in, 1);
>> +    if (unlikely(r < 0)) {
>> +        return r;
>> +    }
>> +
>> +    /*
>> +     * We can poll here since we've had BQL from the time
>> +     * we sent the descriptor.
>> +     */
>> +    return vhost_vdpa_net_svq_poll(s, 1);
>>   }
>>
>>   static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
>> @@ -1152,6 +1168,12 @@ static int 
>> vhost_vdpa_net_excessive_mac_filter_cvq_add(VhostVDPAState *s,
>>       if (unlikely(r < 0)) {
>>           return r;
>>       }
>> +
>> +    /*
>> +     * We can poll here since we've had BQL from the time
>> +     * we sent the descriptor.
>> +     */
>> +    vhost_vdpa_net_svq_poll(s, 1);
>
> Don't we need to check the return value of vhost_vdpa_net_svq_poll here?

Hi Eugenio,

Yes, we should always check the return value of
vhost_vdpa_net_svq_poll(). I will fix this problem
in the v4 patch.

Thanks!


>
>>       if (*s->status != VIRTIO_NET_OK) {
>>           return sizeof(*s->status);
>>       }
>> @@ -1266,10 +1288,18 @@ static int 
>> vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
>>               goto out;
>>           }
>>       } else {
>> -        dev_written = vhost_vdpa_net_cvq_add(s, &out, 1, &in, 1);
>> -        if (unlikely(dev_written < 0)) {
>> +        ssize_t r;
>> +        r = vhost_vdpa_net_cvq_add(s, &out, 1, &in, 1);
>> +        if (unlikely(r < 0)) {
>> +            dev_written = r;
>>               goto out;
>>           }
>> +
>> +        /*
>> +         * We can poll here since we've had BQL from the time
>> +         * we sent the descriptor.
>> +         */
>> +        dev_written = vhost_vdpa_net_svq_poll(s, 1);
>>       }
>>
>>       if (unlikely(dev_written < sizeof(status))) {
>> --
>> 2.25.1
>>
>