On 2023/8/18 23:48, Eugenio Perez Martin wrote:
> On Wed, Jul 19, 2023 at 9:54 AM Hawkins Jiawei <yin31...@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 <yin31...@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
>>
>
