| Message ID | 20221108170755.92768-2-eperezma@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASID support in vhost-vdpa net | expand |
On Wed, Nov 9, 2022 at 1:08 AM Eugenio Pérez <eperezma@redhat.com> wrote: > > This function used to trust in v->shadow_vqs != NULL to know if it must > start svq or not. > > This is not going to be valid anymore, as qemu is going to allocate svq > unconditionally (but it will only start them conditionally). It might be a waste of memory if we did this. Any reason for this? Thanks > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > --- > hw/virtio/vhost-vdpa.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > index 7468e44b87..7f0ff4df5b 100644 > --- a/hw/virtio/vhost-vdpa.c > +++ b/hw/virtio/vhost-vdpa.c > @@ -1029,7 +1029,7 @@ static bool vhost_vdpa_svqs_start(struct vhost_dev *dev) > Error *err = NULL; > unsigned i; > > - if (!v->shadow_vqs) { > + if (!v->shadow_vqs_enabled) { > return true; > } > > @@ -1082,7 +1082,7 @@ static void vhost_vdpa_svqs_stop(struct vhost_dev *dev) > { > struct vhost_vdpa *v = dev->opaque; > > - if (!v->shadow_vqs) { > + if (!v->shadow_vqs_enabled) { > return; > } > > -- > 2.31.1 >
On Thu, Nov 10, 2022 at 6:22 AM Jason Wang <jasowang@redhat.com> wrote: > > On Wed, Nov 9, 2022 at 1:08 AM Eugenio Pérez <eperezma@redhat.com> wrote: > > > > This function used to trust in v->shadow_vqs != NULL to know if it must > > start svq or not. > > > > This is not going to be valid anymore, as qemu is going to allocate svq > > unconditionally (but it will only start them conditionally). > > It might be a waste of memory if we did this. Any reason for this? > Well, it's modelled after vhost_vdpa notifier member [1]. But sure we can reduce the memory usage if SVQ is not used. The first function that needs it is vhost_set_vring_kick. But I think it is not a good function to place the delayed allocation. Would it work to move the allocation to vhost_set_features vhost op? It seems unlikely to me to call callbacks that can affect SVQ earlier than that one. Or maybe to create a new one and call it the first on vhost.c:vhost_dev_start? Thanks! [1] The notifier member already allocates VIRTIO_QUEUE_MAX VhostVDPAHostNotifier for each vhost_vdpa, It is easy to reduce at least to the number of virtqueues on a vhost_vdpa. Should I send a patch for this one? > Thanks > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > --- > > hw/virtio/vhost-vdpa.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > > index 7468e44b87..7f0ff4df5b 100644 > > --- a/hw/virtio/vhost-vdpa.c > > +++ b/hw/virtio/vhost-vdpa.c > > @@ -1029,7 +1029,7 @@ static bool vhost_vdpa_svqs_start(struct vhost_dev *dev) > > Error *err = NULL; > > unsigned i; > > > > - if (!v->shadow_vqs) { > > + if (!v->shadow_vqs_enabled) { > > return true; > > } > > > > @@ -1082,7 +1082,7 @@ static void vhost_vdpa_svqs_stop(struct vhost_dev *dev) > > { > > struct vhost_vdpa *v = dev->opaque; > > > > - if (!v->shadow_vqs) { > > + if (!v->shadow_vqs_enabled) { > > return; > > } > > > > -- > > 2.31.1 > > >
在 2022/11/10 20:54, Eugenio Perez Martin 写道: > On Thu, Nov 10, 2022 at 6:22 AM Jason Wang <jasowang@redhat.com> wrote: >> On Wed, Nov 9, 2022 at 1:08 AM Eugenio Pérez <eperezma@redhat.com> wrote: >>> This function used to trust in v->shadow_vqs != NULL to know if it must >>> start svq or not. >>> >>> This is not going to be valid anymore, as qemu is going to allocate svq >>> unconditionally (but it will only start them conditionally). >> It might be a waste of memory if we did this. Any reason for this? >> > Well, it's modelled after vhost_vdpa notifier member [1]. Right, this could be optimized in the future as well. > > But sure we can reduce the memory usage if SVQ is not used. The first > function that needs it is vhost_set_vring_kick. But I think it is not > a good function to place the delayed allocation. > > Would it work to move the allocation to vhost_set_features vhost op? > It seems unlikely to me to call callbacks that can affect SVQ earlier > than that one. Or maybe to create a new one and call it the first on > vhost.c:vhost_dev_start? Rethink about this, so I think we can leave this in the future. Thanks > > Thanks! > > [1] The notifier member already allocates VIRTIO_QUEUE_MAX > VhostVDPAHostNotifier for each vhost_vdpa, It is easy to reduce at > least to the number of virtqueues on a vhost_vdpa. Should I send a > patch for this one? > > >> Thanks >> >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com> >>> --- >>> hw/virtio/vhost-vdpa.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c >>> index 7468e44b87..7f0ff4df5b 100644 >>> --- a/hw/virtio/vhost-vdpa.c >>> +++ b/hw/virtio/vhost-vdpa.c >>> @@ -1029,7 +1029,7 @@ static bool vhost_vdpa_svqs_start(struct vhost_dev *dev) >>> Error *err = NULL; >>> unsigned i; >>> >>> - if (!v->shadow_vqs) { >>> + if (!v->shadow_vqs_enabled) { >>> return true; >>> } >>> >>> @@ -1082,7 +1082,7 @@ static void vhost_vdpa_svqs_stop(struct vhost_dev *dev) >>> { >>> struct vhost_vdpa *v = dev->opaque; >>> >>> - if (!v->shadow_vqs) { >>> + if (!v->shadow_vqs_enabled) { >>> return; >>> } >>> >>> -- >>> 2.31.1 >>>
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index 7468e44b87..7f0ff4df5b 100644 --- a/hw/virtio/vhost-vdpa.c +++ b/hw/virtio/vhost-vdpa.c @@ -1029,7 +1029,7 @@ static bool vhost_vdpa_svqs_start(struct vhost_dev *dev) Error *err = NULL; unsigned i; - if (!v->shadow_vqs) { + if (!v->shadow_vqs_enabled) { return true; } @@ -1082,7 +1082,7 @@ static void vhost_vdpa_svqs_stop(struct vhost_dev *dev) { struct vhost_vdpa *v = dev->opaque; - if (!v->shadow_vqs) { + if (!v->shadow_vqs_enabled) { return; }
This function used to trust in v->shadow_vqs != NULL to know if it must start svq or not. This is not going to be valid anymore, as qemu is going to allocate svq unconditionally (but it will only start them conditionally). Signed-off-by: Eugenio Pérez <eperezma@redhat.com> --- hw/virtio/vhost-vdpa.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)