Re: [PATCH v5 2/6] vdpa: Allocate SVQ unconditionally

[Michael S. Tsirkin]

On Tue, Oct 11, 2022 at 12:41:50PM +0200, Eugenio Pérez wrote:
> SVQ may run or not in a device depending on runtime conditions (for
> example, if the device can move CVQ to its own group or not).
> 
> Allocate the resources unconditionally, and decide later if to use them
> or not.
> 
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>

I applied this for now but I really dislike it that we are wasting
resources like this.

Can I just drop this patch from the series? It looks like things
will just work anyway ...

I know, when one works on a feature it seems like everyone should
enable it - but the reality is qemu already works quite well for
most users and it is our resposibility to first do no harm.


> ---
>  hw/virtio/vhost-vdpa.c | 33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 7f0ff4df5b..d966966131 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -410,6 +410,21 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, 
> struct vhost_vdpa *v,
>      int r;
>      bool ok;
>  
> +    shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_svq_free);
> +    for (unsigned n = 0; n < hdev->nvqs; ++n) {
> +        g_autoptr(VhostShadowVirtqueue) svq;
> +
> +        svq = vhost_svq_new(v->iova_tree, v->shadow_vq_ops,
> +                            v->shadow_vq_ops_opaque);
> +        if (unlikely(!svq)) {
> +            error_setg(errp, "Cannot create svq %u", n);
> +            return -1;
> +        }
> +        g_ptr_array_add(shadow_vqs, g_steal_pointer(&svq));
> +    }
> +
> +    v->shadow_vqs = g_steal_pointer(&shadow_vqs);
> +
>      if (!v->shadow_vqs_enabled) {
>          return 0;
>      }
> @@ -426,20 +441,6 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, 
> struct vhost_vdpa *v,
>          return -1;
>      }
>  
> -    shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_svq_free);
> -    for (unsigned n = 0; n < hdev->nvqs; ++n) {
> -        g_autoptr(VhostShadowVirtqueue) svq;
> -
> -        svq = vhost_svq_new(v->iova_tree, v->shadow_vq_ops,
> -                            v->shadow_vq_ops_opaque);
> -        if (unlikely(!svq)) {
> -            error_setg(errp, "Cannot create svq %u", n);
> -            return -1;
> -        }
> -        g_ptr_array_add(shadow_vqs, g_steal_pointer(&svq));
> -    }
> -
> -    v->shadow_vqs = g_steal_pointer(&shadow_vqs);
>      return 0;
>  }
>  
> @@ -580,10 +581,6 @@ static void vhost_vdpa_svq_cleanup(struct vhost_dev *dev)
>      struct vhost_vdpa *v = dev->opaque;
>      size_t idx;
>  
> -    if (!v->shadow_vqs) {
> -        return;
> -    }
> -
>      for (idx = 0; idx < v->shadow_vqs->len; ++idx) {
>          vhost_svq_stop(g_ptr_array_index(v->shadow_vqs, idx));
>      }
> -- 
> 2.31.1