* [PATCH v2 2/3] virtio-balloon: fix memory leak while attach virtio-balloon device
2019-12-04 7:31 [PATCH v2 1/3] virtio: add ability to delete vq through a pointer pannengyuan
@ 2019-12-04 7:31 ` pannengyuan
2019-12-04 10:16 ` Laurent Vivier
2019-12-04 7:31 ` [PATCH v2 3/3] virtio-serial-bus: fix memory leak while attach virtio-serial-bus pannengyuan
` (5 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: pannengyuan @ 2019-12-04 7:31 UTC (permalink / raw)
To: mst; +Cc: liyiting, kuhn.chenqun, Pan Nengyuan, qemu-devel, zhang.zhanghailiang
From: Pan Nengyuan <pannengyuan@huawei.com>
ivq/dvq/svq/free_page_vq is forgot to cleanup in
virtio_balloon_device_unrealize, the memory leak stack is as follow:
Direct leak of 14336 byte(s) in 2 object(s) allocated from:
#0 0x7f99fd9d8560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
#1 0x7f99fcb20015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
#2 0x557d90638437 in virtio_add_queue hw/virtio/virtio.c:2327
#3 0x557d9064401d in virtio_balloon_device_realize hw/virtio/virtio-balloon.c:793
#4 0x557d906356f7 in virtio_device_realize hw/virtio/virtio.c:3504
#5 0x557d9073f081 in device_set_realized hw/core/qdev.c:876
#6 0x557d908b1f4d in property_set_bool qom/object.c:2080
#7 0x557d908b655e in object_property_set_qobject qom/qom-qobject.c:26
Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
Change v2 to v1:
- use virtio_delete_queue to cleanup vq through a vq pointer (suggested by
Michael S. Tsirkin)
---
hw/virtio/virtio-balloon.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 40b04f5..57f3b9f 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -831,6 +831,13 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
}
balloon_stats_destroy_timer(s);
qemu_remove_balloon_handler(s);
+
+ virtio_delete_queue(s->ivq);
+ virtio_delete_queue(s->dvq);
+ virtio_delete_queue(s->svq);
+ if (s->free_page_vq) {
+ virtio_delete_queue(s->free_page_vq);
+ }
virtio_cleanup(vdev);
}
--
2.7.2.windows.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/3] virtio-balloon: fix memory leak while attach virtio-balloon device
2019-12-04 7:31 ` [PATCH v2 2/3] virtio-balloon: fix memory leak while attach virtio-balloon device pannengyuan
@ 2019-12-04 10:16 ` Laurent Vivier
0 siblings, 0 replies; 21+ messages in thread
From: Laurent Vivier @ 2019-12-04 10:16 UTC (permalink / raw)
To: pannengyuan, mst; +Cc: liyiting, kuhn.chenqun, qemu-devel, zhang.zhanghailiang
On 04/12/2019 08:31, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
>
> ivq/dvq/svq/free_page_vq is forgot to cleanup in
> virtio_balloon_device_unrealize, the memory leak stack is as follow:
>
> Direct leak of 14336 byte(s) in 2 object(s) allocated from:
> #0 0x7f99fd9d8560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
> #1 0x7f99fcb20015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
> #2 0x557d90638437 in virtio_add_queue hw/virtio/virtio.c:2327
> #3 0x557d9064401d in virtio_balloon_device_realize hw/virtio/virtio-balloon.c:793
> #4 0x557d906356f7 in virtio_device_realize hw/virtio/virtio.c:3504
> #5 0x557d9073f081 in device_set_realized hw/core/qdev.c:876
> #6 0x557d908b1f4d in property_set_bool qom/object.c:2080
> #7 0x557d908b655e in object_property_set_qobject qom/qom-qobject.c:26
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
> Change v2 to v1:
> - use virtio_delete_queue to cleanup vq through a vq pointer (suggested by
> Michael S. Tsirkin)
> ---
> hw/virtio/virtio-balloon.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 40b04f5..57f3b9f 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -831,6 +831,13 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
> }
> balloon_stats_destroy_timer(s);
> qemu_remove_balloon_handler(s);
> +
> + virtio_delete_queue(s->ivq);
> + virtio_delete_queue(s->dvq);
> + virtio_delete_queue(s->svq);
> + if (s->free_page_vq) {
> + virtio_delete_queue(s->free_page_vq);
> + }
> virtio_cleanup(vdev);
> }
>
>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 3/3] virtio-serial-bus: fix memory leak while attach virtio-serial-bus
2019-12-04 7:31 [PATCH v2 1/3] virtio: add ability to delete vq through a pointer pannengyuan
2019-12-04 7:31 ` [PATCH v2 2/3] virtio-balloon: fix memory leak while attach virtio-balloon device pannengyuan
@ 2019-12-04 7:31 ` pannengyuan
2019-12-04 10:17 ` Laurent Vivier
` (2 more replies)
2019-12-04 8:33 ` [PATCH v2 1/3] virtio: add ability to delete vq through a pointer Pankaj Gupta
` (4 subsequent siblings)
6 siblings, 3 replies; 21+ messages in thread
From: pannengyuan @ 2019-12-04 7:31 UTC (permalink / raw)
To: mst
Cc: liyiting, Laurent Vivier, zhang.zhanghailiang, Amit Shah,
Pan Nengyuan, qemu-devel, Paolo Bonzini, Marc-André Lureau,
kuhn.chenqun
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="y", Size: 2195 bytes --]
From: Pan Nengyuan <pannengyuan@huawei.com>
ivqs/ovqs/c_ivq/c_ovq is forgot to cleanup in
virtio_serial_device_unrealize, the memory leak stack is as bellow:
Direct leak of 1290240 byte(s) in 180 object(s) allocated from:
#0 0x7fc9bfc27560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
#1 0x7fc9bed6f015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
#2 0x5650e02b83e7 in virtio_add_queue hw/virtio/virtio.c:2327
#3 0x5650e02847b5 in virtio_serial_device_realize hw/char/virtio-serial-bus.c:1089
#4 0x5650e02b56a7 in virtio_device_realize hw/virtio/virtio.c:3504
#5 0x5650e03bf031 in device_set_realized hw/core/qdev.c:876
#6 0x5650e0531efd in property_set_bool qom/object.c:2080
#7 0x5650e053650e in object_property_set_qobject qom/qom-qobject.c:26
#8 0x5650e0533e14 in object_property_set_bool qom/object.c:1338
#9 0x5650e04c0e37 in virtio_pci_realize hw/virtio/virtio-pci.c:1801
Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
Cc: Laurent Vivier <lvivier@redhat.com>
Cc: Amit Shah <amit@kernel.org>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
Changes v2 to v1:
- use virtio_delete_queue to cleanup vq through a vq pointer (suggested by
Michael S. Tsirkin)
---
hw/char/virtio-serial-bus.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 3325904..e1cbce3 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -1126,9 +1126,17 @@ static void virtio_serial_device_unrealize(DeviceState *dev, Error **errp)
{
VirtIODevice *vdev = VIRTIO_DEVICE(dev);
VirtIOSerial *vser = VIRTIO_SERIAL(dev);
+ int i;
QLIST_REMOVE(vser, next);
+ virtio_delete_queue(vser->c_ivq);
+ virtio_delete_queue(vser->c_ovq);
+ for (i = 0; i < vser->bus.max_nr_ports; i++) {
+ virtio_delete_queue(vser->ivqs[i]);
+ virtio_delete_queue(vser->ovqs[i]);
+ }
+
g_free(vser->ivqs);
g_free(vser->ovqs);
g_free(vser->ports_map);
--
2.7.2.windows.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/3] virtio-serial-bus: fix memory leak while attach virtio-serial-bus
2019-12-04 7:31 ` [PATCH v2 3/3] virtio-serial-bus: fix memory leak while attach virtio-serial-bus pannengyuan
@ 2019-12-04 10:17 ` Laurent Vivier
2019-12-04 14:41 ` Eric Blake
2019-12-09 16:51 ` Michael S. Tsirkin
2 siblings, 0 replies; 21+ messages in thread
From: Laurent Vivier @ 2019-12-04 10:17 UTC (permalink / raw)
To: pannengyuan, mst
Cc: liyiting, zhang.zhanghailiang, kuhn.chenqun, Amit Shah,
qemu-devel, Marc-André Lureau, Paolo Bonzini
On 04/12/2019 08:31, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
>
> ivqs/ovqs/c_ivq/c_ovq is forgot to cleanup in
> virtio_serial_device_unrealize, the memory leak stack is as bellow:
>
> Direct leak of 1290240 byte(s) in 180 object(s) allocated from:
> #0 0x7fc9bfc27560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
> #1 0x7fc9bed6f015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
> #2 0x5650e02b83e7 in virtio_add_queue hw/virtio/virtio.c:2327
> #3 0x5650e02847b5 in virtio_serial_device_realize hw/char/virtio-serial-bus.c:1089
> #4 0x5650e02b56a7 in virtio_device_realize hw/virtio/virtio.c:3504
> #5 0x5650e03bf031 in device_set_realized hw/core/qdev.c:876
> #6 0x5650e0531efd in property_set_bool qom/object.c:2080
> #7 0x5650e053650e in object_property_set_qobject qom/qom-qobject.c:26
> #8 0x5650e0533e14 in object_property_set_bool qom/object.c:1338
> #9 0x5650e04c0e37 in virtio_pci_realize hw/virtio/virtio-pci.c:1801
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> Cc: Laurent Vivier <lvivier@redhat.com>
> Cc: Amit Shah <amit@kernel.org>
> Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
> Changes v2 to v1:
> - use virtio_delete_queue to cleanup vq through a vq pointer (suggested by
> Michael S. Tsirkin)
> ---
> hw/char/virtio-serial-bus.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 3325904..e1cbce3 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -1126,9 +1126,17 @@ static void virtio_serial_device_unrealize(DeviceState *dev, Error **errp)
> {
> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> VirtIOSerial *vser = VIRTIO_SERIAL(dev);
> + int i;
>
> QLIST_REMOVE(vser, next);
>
> + virtio_delete_queue(vser->c_ivq);
> + virtio_delete_queue(vser->c_ovq);
> + for (i = 0; i < vser->bus.max_nr_ports; i++) {
> + virtio_delete_queue(vser->ivqs[i]);
> + virtio_delete_queue(vser->ovqs[i]);
> + }
> +
> g_free(vser->ivqs);
> g_free(vser->ovqs);
> g_free(vser->ports_map);
>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/3] virtio-serial-bus: fix memory leak while attach virtio-serial-bus
2019-12-04 7:31 ` [PATCH v2 3/3] virtio-serial-bus: fix memory leak while attach virtio-serial-bus pannengyuan
2019-12-04 10:17 ` Laurent Vivier
@ 2019-12-04 14:41 ` Eric Blake
2019-12-09 16:51 ` Michael S. Tsirkin
2 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2019-12-04 14:41 UTC (permalink / raw)
To: pannengyuan, mst
Cc: liyiting, Laurent Vivier, zhang.zhanghailiang, kuhn.chenqun,
Amit Shah, qemu-devel, Marc-André Lureau, Paolo Bonzini
On 12/4/19 1:31 AM, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
>
> ivqs/ovqs/c_ivq/c_ovq is forgot to cleanup in
s/is //
> virtio_serial_device_unrealize, the memory leak stack is as bellow:
below
>
> Direct leak of 1290240 byte(s) in 180 object(s) allocated from:
> #0 0x7fc9bfc27560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
> #1 0x7fc9bed6f015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
> #2 0x5650e02b83e7 in virtio_add_queue hw/virtio/virtio.c:2327
> #3 0x5650e02847b5 in virtio_serial_device_realize hw/char/virtio-serial-bus.c:1089
> #4 0x5650e02b56a7 in virtio_device_realize hw/virtio/virtio.c:3504
> #5 0x5650e03bf031 in device_set_realized hw/core/qdev.c:876
> #6 0x5650e0531efd in property_set_bool qom/object.c:2080
> #7 0x5650e053650e in object_property_set_qobject qom/qom-qobject.c:26
> #8 0x5650e0533e14 in object_property_set_bool qom/object.c:1338
> #9 0x5650e04c0e37 in virtio_pci_realize hw/virtio/virtio-pci.c:1801
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> Cc: Laurent Vivier <lvivier@redhat.com>
> Cc: Amit Shah <amit@kernel.org>
> Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/3] virtio-serial-bus: fix memory leak while attach virtio-serial-bus
2019-12-04 7:31 ` [PATCH v2 3/3] virtio-serial-bus: fix memory leak while attach virtio-serial-bus pannengyuan
2019-12-04 10:17 ` Laurent Vivier
2019-12-04 14:41 ` Eric Blake
@ 2019-12-09 16:51 ` Michael S. Tsirkin
2 siblings, 0 replies; 21+ messages in thread
From: Michael S. Tsirkin @ 2019-12-09 16:51 UTC (permalink / raw)
To: pannengyuan
Cc: liyiting, Laurent Vivier, zhang.zhanghailiang, Amit Shah,
qemu-devel, Paolo Bonzini, Marc-André Lureau, kuhn.chenqun
Headers are wrong for this one: charset="y" confuses git am.
On Wed, Dec 04, 2019 at 03:31:56PM +0800, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
>
> ivqs/ovqs/c_ivq/c_ovq is forgot to cleanup in
> virtio_serial_device_unrealize, the memory leak stack is as bellow:
>
> Direct leak of 1290240 byte(s) in 180 object(s) allocated from:
> #0 0x7fc9bfc27560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
> #1 0x7fc9bed6f015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
> #2 0x5650e02b83e7 in virtio_add_queue hw/virtio/virtio.c:2327
> #3 0x5650e02847b5 in virtio_serial_device_realize hw/char/virtio-serial-bus.c:1089
> #4 0x5650e02b56a7 in virtio_device_realize hw/virtio/virtio.c:3504
> #5 0x5650e03bf031 in device_set_realized hw/core/qdev.c:876
> #6 0x5650e0531efd in property_set_bool qom/object.c:2080
> #7 0x5650e053650e in object_property_set_qobject qom/qom-qobject.c:26
> #8 0x5650e0533e14 in object_property_set_bool qom/object.c:1338
> #9 0x5650e04c0e37 in virtio_pci_realize hw/virtio/virtio-pci.c:1801
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> Cc: Laurent Vivier <lvivier@redhat.com>
> Cc: Amit Shah <amit@kernel.org>
> Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
> Changes v2 to v1:
> - use virtio_delete_queue to cleanup vq through a vq pointer (suggested by
> Michael S. Tsirkin)
> ---
> hw/char/virtio-serial-bus.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 3325904..e1cbce3 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -1126,9 +1126,17 @@ static void virtio_serial_device_unrealize(DeviceState *dev, Error **errp)
> {
> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> VirtIOSerial *vser = VIRTIO_SERIAL(dev);
> + int i;
>
> QLIST_REMOVE(vser, next);
>
> + virtio_delete_queue(vser->c_ivq);
> + virtio_delete_queue(vser->c_ovq);
> + for (i = 0; i < vser->bus.max_nr_ports; i++) {
> + virtio_delete_queue(vser->ivqs[i]);
> + virtio_delete_queue(vser->ovqs[i]);
> + }
> +
> g_free(vser->ivqs);
> g_free(vser->ovqs);
> g_free(vser->ports_map);
> --
> 2.7.2.windows.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer
2019-12-04 7:31 [PATCH v2 1/3] virtio: add ability to delete vq through a pointer pannengyuan
2019-12-04 7:31 ` [PATCH v2 2/3] virtio-balloon: fix memory leak while attach virtio-balloon device pannengyuan
2019-12-04 7:31 ` [PATCH v2 3/3] virtio-serial-bus: fix memory leak while attach virtio-serial-bus pannengyuan
@ 2019-12-04 8:33 ` Pankaj Gupta
2019-12-05 2:30 ` Pan Nengyuan
2019-12-09 15:58 ` Michael S. Tsirkin
2019-12-04 9:40 ` Laurent Vivier
` (3 subsequent siblings)
6 siblings, 2 replies; 21+ messages in thread
From: Pankaj Gupta @ 2019-12-04 8:33 UTC (permalink / raw)
To: pannengyuan; +Cc: liyiting, kuhn chenqun, zhang zhanghailiang, qemu-devel, mst
> From: Pan Nengyuan <pannengyuan@huawei.com>
>
> Devices tend to maintain vq pointers, allow deleting them trough a vq
> pointer.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
> Changes v2 to v1:
> - add a new function virtio_delete_queue to cleanup vq through a vq pointer
> ---
> hw/virtio/virtio.c | 16 +++++++++++-----
> include/hw/virtio/virtio.h | 2 ++
> 2 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 04716b5..6de3cfd 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int
> queue_size,
> return &vdev->vq[i];
> }
>
> +void virtio_delete_queue(VirtQueue *vq)
> +{
> + vq->vring.num = 0;
> + vq->vring.num_default = 0;
> + vq->handle_output = NULL;
> + vq->handle_aio_output = NULL;
> + g_free(vq->used_elems);
> + vq->used_elems = NULL;
> +}
> +
> void virtio_del_queue(VirtIODevice *vdev, int n)
> {
> if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
> abort();
> }
>
> - vdev->vq[n].vring.num = 0;
> - vdev->vq[n].vring.num_default = 0;
> - vdev->vq[n].handle_output = NULL;
> - vdev->vq[n].handle_aio_output = NULL;
> - g_free(vdev->vq[n].used_elems);
> + virtio_delete_queue(&vdev->vq[n]);
> }
>
> static void virtio_set_isr(VirtIODevice *vdev, int value)
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index c32a815..e18756d 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int
> queue_size,
>
> void virtio_del_queue(VirtIODevice *vdev, int n);
>
> +void virtio_delete_queue(VirtQueue *vq);
> +
> void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
> unsigned int len);
> void virtqueue_flush(VirtQueue *vq, unsigned int count);
> --
> 2.7.2.windows.1
>
>
Overall it ooks good to me.
Just one point: e.g in virtio_rng: "virtio_rng_device_unrealize" function
We are doing : virtio_del_queue(vdev, 0);
One can directly call "virtio_delete_queue". It can become confusing
to call multiple functions for same purpose. Instead, Can we make
"virtio_delete_queue" static inline?
Other than that:
Reviewed-by: Pankaj Gupta <pagupta@redhat.com>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer
2019-12-04 8:33 ` [PATCH v2 1/3] virtio: add ability to delete vq through a pointer Pankaj Gupta
@ 2019-12-05 2:30 ` Pan Nengyuan
2019-12-05 4:51 ` Pankaj Gupta
2019-12-09 15:58 ` Michael S. Tsirkin
1 sibling, 1 reply; 21+ messages in thread
From: Pan Nengyuan @ 2019-12-05 2:30 UTC (permalink / raw)
To: Pankaj Gupta; +Cc: liyiting, kuhn chenqun, zhang zhanghailiang, qemu-devel, mst
On 2019/12/4 16:33, Pankaj Gupta wrote:
>
>> From: Pan Nengyuan <pannengyuan@huawei.com>
>>
>> Devices tend to maintain vq pointers, allow deleting them trough a vq
>> pointer.
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
>> Changes v2 to v1:
>> - add a new function virtio_delete_queue to cleanup vq through a vq pointer
>> ---
>> hw/virtio/virtio.c | 16 +++++++++++-----
>> include/hw/virtio/virtio.h | 2 ++
>> 2 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 04716b5..6de3cfd 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int
>> queue_size,
>> return &vdev->vq[i];
>> }
>>
>> +void virtio_delete_queue(VirtQueue *vq)
>> +{
>> + vq->vring.num = 0;
>> + vq->vring.num_default = 0;
>> + vq->handle_output = NULL;
>> + vq->handle_aio_output = NULL;
>> + g_free(vq->used_elems);
>> + vq->used_elems = NULL;
>> +}
>> +
>> void virtio_del_queue(VirtIODevice *vdev, int n)
>> {
>> if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
>> abort();
>> }
>>
>> - vdev->vq[n].vring.num = 0;
>> - vdev->vq[n].vring.num_default = 0;
>> - vdev->vq[n].handle_output = NULL;
>> - vdev->vq[n].handle_aio_output = NULL;
>> - g_free(vdev->vq[n].used_elems);
>> + virtio_delete_queue(&vdev->vq[n]);
>> }
>>
>> static void virtio_set_isr(VirtIODevice *vdev, int value)
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index c32a815..e18756d 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int
>> queue_size,
>>
>> void virtio_del_queue(VirtIODevice *vdev, int n);
>>
>> +void virtio_delete_queue(VirtQueue *vq);
>> +
>> void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
>> unsigned int len);
>> void virtqueue_flush(VirtQueue *vq, unsigned int count);
>> --
>> 2.7.2.windows.1
>>
>>
> Overall it ooks good to me.
>
> Just one point: e.g in virtio_rng: "virtio_rng_device_unrealize" function
> We are doing : virtio_del_queue(vdev, 0);
>
> One can directly call "virtio_delete_queue". It can become confusing
> to call multiple functions for same purpose. Instead, Can we make
> "virtio_delete_queue" static inline?
>
yes, It will be a little confused, but I think it will have the same
problem if we make "virtio_delete_queue" static inline. We can directly
call it aslo. (e.g virtio-serial-bus.c virtio-balloon.c).
How about replacing the function name to make it more clear (e.g
virtio_delete_queue -> virtio_queue_cleanup) ? It's too similar between
"virtio_del_queue" and "virtio_delete_queue".
> Other than that:
> Reviewed-by: Pankaj Gupta <pagupta@redhat.com>
>
>>
>>
>
>
> .
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer
2019-12-05 2:30 ` Pan Nengyuan
@ 2019-12-05 4:51 ` Pankaj Gupta
0 siblings, 0 replies; 21+ messages in thread
From: Pankaj Gupta @ 2019-12-05 4:51 UTC (permalink / raw)
To: Pan Nengyuan; +Cc: liyiting, kuhn chenqun, mst, zhang zhanghailiang, qemu-devel
>
> On 2019/12/4 16:33, Pankaj Gupta wrote:
> >
> >> From: Pan Nengyuan <pannengyuan@huawei.com>
> >>
> >> Devices tend to maintain vq pointers, allow deleting them trough a vq
> >> pointer.
> >>
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> >> ---
> >> Changes v2 to v1:
> >> - add a new function virtio_delete_queue to cleanup vq through a vq
> >> pointer
> >> ---
> >> hw/virtio/virtio.c | 16 +++++++++++-----
> >> include/hw/virtio/virtio.h | 2 ++
> >> 2 files changed, 13 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >> index 04716b5..6de3cfd 100644
> >> --- a/hw/virtio/virtio.c
> >> +++ b/hw/virtio/virtio.c
> >> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev,
> >> int
> >> queue_size,
> >> return &vdev->vq[i];
> >> }
> >>
> >> +void virtio_delete_queue(VirtQueue *vq)
> >> +{
> >> + vq->vring.num = 0;
> >> + vq->vring.num_default = 0;
> >> + vq->handle_output = NULL;
> >> + vq->handle_aio_output = NULL;
> >> + g_free(vq->used_elems);
> >> + vq->used_elems = NULL;
> >> +}
> >> +
> >> void virtio_del_queue(VirtIODevice *vdev, int n)
> >> {
> >> if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
> >> abort();
> >> }
> >>
> >> - vdev->vq[n].vring.num = 0;
> >> - vdev->vq[n].vring.num_default = 0;
> >> - vdev->vq[n].handle_output = NULL;
> >> - vdev->vq[n].handle_aio_output = NULL;
> >> - g_free(vdev->vq[n].used_elems);
> >> + virtio_delete_queue(&vdev->vq[n]);
> >> }
> >>
> >> static void virtio_set_isr(VirtIODevice *vdev, int value)
> >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> >> index c32a815..e18756d 100644
> >> --- a/include/hw/virtio/virtio.h
> >> +++ b/include/hw/virtio/virtio.h
> >> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int
> >> queue_size,
> >>
> >> void virtio_del_queue(VirtIODevice *vdev, int n);
> >>
> >> +void virtio_delete_queue(VirtQueue *vq);
> >> +
> >> void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
> >> unsigned int len);
> >> void virtqueue_flush(VirtQueue *vq, unsigned int count);
> >> --
> >> 2.7.2.windows.1
> >>
> >>
> > Overall it ooks good to me.
> >
> > Just one point: e.g in virtio_rng: "virtio_rng_device_unrealize" function
> > We are doing : virtio_del_queue(vdev, 0);
> >
> > One can directly call "virtio_delete_queue". It can become confusing
> > to call multiple functions for same purpose. Instead, Can we make
> > "virtio_delete_queue" static inline?
> >
> yes, It will be a little confused, but I think it will have the same
> problem if we make "virtio_delete_queue" static inline. We can directly
> call it aslo. (e.g virtio-serial-bus.c virtio-balloon.c).
>
> How about replacing the function name to make it more clear (e.g
> virtio_delete_queue -> virtio_queue_cleanup) ? It's too similar between
> "virtio_del_queue" and "virtio_delete_queue".
I am just thinking if we need these two separate functions.
Yes, changing name of virtio_delete_queue -> virtio_queue_cleanup
should be good enough.
Thanks,
Pankaj
>
> > Other than that:
> > Reviewed-by: Pankaj Gupta <pagupta@redhat.com>
> >
> >>
> >>
> >
> >
> > .
> >
>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer
2019-12-04 8:33 ` [PATCH v2 1/3] virtio: add ability to delete vq through a pointer Pankaj Gupta
2019-12-05 2:30 ` Pan Nengyuan
@ 2019-12-09 15:58 ` Michael S. Tsirkin
1 sibling, 0 replies; 21+ messages in thread
From: Michael S. Tsirkin @ 2019-12-09 15:58 UTC (permalink / raw)
To: Pankaj Gupta
Cc: liyiting, kuhn chenqun, pannengyuan, qemu-devel, zhang zhanghailiang
On Wed, Dec 04, 2019 at 03:33:07AM -0500, Pankaj Gupta wrote:
>
> > From: Pan Nengyuan <pannengyuan@huawei.com>
> >
> > Devices tend to maintain vq pointers, allow deleting them trough a vq
> > pointer.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> > ---
> > Changes v2 to v1:
> > - add a new function virtio_delete_queue to cleanup vq through a vq pointer
> > ---
> > hw/virtio/virtio.c | 16 +++++++++++-----
> > include/hw/virtio/virtio.h | 2 ++
> > 2 files changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 04716b5..6de3cfd 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int
> > queue_size,
> > return &vdev->vq[i];
> > }
> >
> > +void virtio_delete_queue(VirtQueue *vq)
> > +{
> > + vq->vring.num = 0;
> > + vq->vring.num_default = 0;
> > + vq->handle_output = NULL;
> > + vq->handle_aio_output = NULL;
> > + g_free(vq->used_elems);
> > + vq->used_elems = NULL;
> > +}
> > +
> > void virtio_del_queue(VirtIODevice *vdev, int n)
> > {
> > if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
> > abort();
> > }
> >
> > - vdev->vq[n].vring.num = 0;
> > - vdev->vq[n].vring.num_default = 0;
> > - vdev->vq[n].handle_output = NULL;
> > - vdev->vq[n].handle_aio_output = NULL;
> > - g_free(vdev->vq[n].used_elems);
> > + virtio_delete_queue(&vdev->vq[n]);
> > }
> >
> > static void virtio_set_isr(VirtIODevice *vdev, int value)
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index c32a815..e18756d 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int
> > queue_size,
> >
> > void virtio_del_queue(VirtIODevice *vdev, int n);
> >
> > +void virtio_delete_queue(VirtQueue *vq);
> > +
> > void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
> > unsigned int len);
> > void virtqueue_flush(VirtQueue *vq, unsigned int count);
> > --
> > 2.7.2.windows.1
> >
> >
> Overall it ooks good to me.
>
> Just one point: e.g in virtio_rng: "virtio_rng_device_unrealize" function
> We are doing : virtio_del_queue(vdev, 0);
Yea. Let's just bite the bullet and convert all callers.
Not so many of them.
> One can directly call "virtio_delete_queue". It can become confusing
> to call multiple functions for same purpose. Instead, Can we make
> "virtio_delete_queue" static inline?
We can't really.
> Other than that:
> Reviewed-by: Pankaj Gupta <pagupta@redhat.com>
>
> >
> >
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer
2019-12-04 7:31 [PATCH v2 1/3] virtio: add ability to delete vq through a pointer pannengyuan
` (2 preceding siblings ...)
2019-12-04 8:33 ` [PATCH v2 1/3] virtio: add ability to delete vq through a pointer Pankaj Gupta
@ 2019-12-04 9:40 ` Laurent Vivier
2019-12-04 14:40 ` Eric Blake
` (2 subsequent siblings)
6 siblings, 0 replies; 21+ messages in thread
From: Laurent Vivier @ 2019-12-04 9:40 UTC (permalink / raw)
To: pannengyuan, mst; +Cc: liyiting, kuhn.chenqun, qemu-devel, zhang.zhanghailiang
On 04/12/2019 08:31, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
>
> Devices tend to maintain vq pointers, allow deleting them trough a vq pointer.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
> Changes v2 to v1:
> - add a new function virtio_delete_queue to cleanup vq through a vq pointer
> ---
> hw/virtio/virtio.c | 16 +++++++++++-----
> include/hw/virtio/virtio.h | 2 ++
> 2 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 04716b5..6de3cfd 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
> return &vdev->vq[i];
> }
>
> +void virtio_delete_queue(VirtQueue *vq)
> +{
> + vq->vring.num = 0;
> + vq->vring.num_default = 0;
> + vq->handle_output = NULL;
> + vq->handle_aio_output = NULL;
> + g_free(vq->used_elems);
> + vq->used_elems = NULL;
> +}
> +
> void virtio_del_queue(VirtIODevice *vdev, int n)
> {
> if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
> abort();
> }
>
> - vdev->vq[n].vring.num = 0;
> - vdev->vq[n].vring.num_default = 0;
> - vdev->vq[n].handle_output = NULL;
> - vdev->vq[n].handle_aio_output = NULL;
> - g_free(vdev->vq[n].used_elems);
> + virtio_delete_queue(&vdev->vq[n]);
> }
>
> static void virtio_set_isr(VirtIODevice *vdev, int value)
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index c32a815..e18756d 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
>
> void virtio_del_queue(VirtIODevice *vdev, int n);
>
> +void virtio_delete_queue(VirtQueue *vq);
> +
> void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
> unsigned int len);
> void virtqueue_flush(VirtQueue *vq, unsigned int count);
>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer
2019-12-04 7:31 [PATCH v2 1/3] virtio: add ability to delete vq through a pointer pannengyuan
` (3 preceding siblings ...)
2019-12-04 9:40 ` Laurent Vivier
@ 2019-12-04 14:40 ` Eric Blake
2019-12-05 2:35 ` Pan Nengyuan
2019-12-05 16:45 ` Amit Shah
2019-12-09 16:43 ` Michael S. Tsirkin
6 siblings, 1 reply; 21+ messages in thread
From: Eric Blake @ 2019-12-04 14:40 UTC (permalink / raw)
To: pannengyuan, mst; +Cc: liyiting, kuhn.chenqun, qemu-devel, zhang.zhanghailiang
On 12/4/19 1:31 AM, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
>
> Devices tend to maintain vq pointers, allow deleting them trough a vq pointer.
through
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
Also, don't forget to send a 0/3 cover letter (any series longer than
one patch should have a cover letter; it is possible to configure git to
do this automatically: https://wiki.qemu.org/Contribute/SubmitAPatch has
this tip and others)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer
2019-12-04 14:40 ` Eric Blake
@ 2019-12-05 2:35 ` Pan Nengyuan
0 siblings, 0 replies; 21+ messages in thread
From: Pan Nengyuan @ 2019-12-05 2:35 UTC (permalink / raw)
To: Eric Blake, mst; +Cc: liyiting, kuhn.chenqun, qemu-devel, zhang.zhanghailiang
On 2019/12/4 22:40, Eric Blake wrote:
> On 12/4/19 1:31 AM, pannengyuan@huawei.com wrote:
>> From: Pan Nengyuan <pannengyuan@huawei.com>
>>
>> Devices tend to maintain vq pointers, allow deleting them trough a vq
>> pointer.
>
> through
Thanks. I'm sorry for my carelessness.
>
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
>
> Also, don't forget to send a 0/3 cover letter (any series longer than
> one patch should have a cover letter; it is possible to configure git to
> do this automatically: https://wiki.qemu.org/Contribute/SubmitAPatch has
> this tip and others)
ok, thanks.
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer
2019-12-04 7:31 [PATCH v2 1/3] virtio: add ability to delete vq through a pointer pannengyuan
` (4 preceding siblings ...)
2019-12-04 14:40 ` Eric Blake
@ 2019-12-05 16:45 ` Amit Shah
2019-12-06 2:17 ` Pan Nengyuan
2019-12-09 16:43 ` Michael S. Tsirkin
6 siblings, 1 reply; 21+ messages in thread
From: Amit Shah @ 2019-12-05 16:45 UTC (permalink / raw)
To: pannengyuan, mst; +Cc: liyiting, kuhn.chenqun, qemu-devel, zhang.zhanghailiang
On Wed, 2019-12-04 at 15:31 +0800, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
Shouldn't this be From: mst?
I didn't find a ref to the original patch to confirm if you had to
adapt it in any way, though.
> Devices tend to maintain vq pointers, allow deleting them trough a vq
> pointer.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
> Changes v2 to v1:
> - add a new function virtio_delete_queue to cleanup vq through a vq
> pointer
> ---
> hw/virtio/virtio.c | 16 +++++++++++-----
> include/hw/virtio/virtio.h | 2 ++
> 2 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 04716b5..6de3cfd 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice
> *vdev, int queue_size,
> return &vdev->vq[i];
> }
>
> +void virtio_delete_queue(VirtQueue *vq)
> +{
> + vq->vring.num = 0;
> + vq->vring.num_default = 0;
> + vq->handle_output = NULL;
> + vq->handle_aio_output = NULL;
> + g_free(vq->used_elems);
> + vq->used_elems = NULL;
> +}
> +
> void virtio_del_queue(VirtIODevice *vdev, int n)
> {
> if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
> abort();
> }
>
> - vdev->vq[n].vring.num = 0;
> - vdev->vq[n].vring.num_default = 0;
> - vdev->vq[n].handle_output = NULL;
> - vdev->vq[n].handle_aio_output = NULL;
> - g_free(vdev->vq[n].used_elems);
> + virtio_delete_queue(&vdev->vq[n]);
> }
>
> static void virtio_set_isr(VirtIODevice *vdev, int value)
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index c32a815..e18756d 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev,
> int queue_size,
>
> void virtio_del_queue(VirtIODevice *vdev, int n);
>
> +void virtio_delete_queue(VirtQueue *vq);
> +
> void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
> unsigned int len);
> void virtqueue_flush(VirtQueue *vq, unsigned int count);
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer
2019-12-05 16:45 ` Amit Shah
@ 2019-12-06 2:17 ` Pan Nengyuan
2019-12-06 8:56 ` Amit Shah
0 siblings, 1 reply; 21+ messages in thread
From: Pan Nengyuan @ 2019-12-06 2:17 UTC (permalink / raw)
To: Amit Shah, mst; +Cc: liyiting, kuhn.chenqun, qemu-devel, zhang.zhanghailiang
On 2019/12/6 0:45, Amit Shah wrote:
> On Wed, 2019-12-04 at 15:31 +0800, pannengyuan@huawei.com wrote:
>> From: Pan Nengyuan <pannengyuan@huawei.com>
>
> Shouldn't this be From: mst?
>
> I didn't find a ref to the original patch to confirm if you had to
> adapt it in any way, though.
>
Here is the original
patch: https://lists.nongnu.org/archive/html/qemu-devel/2019-12/msg00402.html
I just change one line(set used_elems to NULL). In next version, I will
change function name from virtio_delete_queue to virtio_queue_cleanup
(It's too similar between "virtio_del_queue" and "virtio_delete_queue"):
https://lists.nongnu.org/archive/html/qemu-devel/2019-12/msg00877.html.
According to these, should I change it in next version?
Thanks.
>> Devices tend to maintain vq pointers, allow deleting them trough a vq
>> pointer.
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
>> Changes v2 to v1:
>> - add a new function virtio_delete_queue to cleanup vq through a vq
>> pointer
>> ---
>> hw/virtio/virtio.c | 16 +++++++++++-----
>> include/hw/virtio/virtio.h | 2 ++
>> 2 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 04716b5..6de3cfd 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice
>> *vdev, int queue_size,
>> return &vdev->vq[i];
>> }
>>
>> +void virtio_delete_queue(VirtQueue *vq)
>> +{
>> + vq->vring.num = 0;
>> + vq->vring.num_default = 0;
>> + vq->handle_output = NULL;
>> + vq->handle_aio_output = NULL;
>> + g_free(vq->used_elems);
>> + vq->used_elems = NULL;
>> +}
>> +
>> void virtio_del_queue(VirtIODevice *vdev, int n)
>> {
>> if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
>> abort();
>> }
>>
>> - vdev->vq[n].vring.num = 0;
>> - vdev->vq[n].vring.num_default = 0;
>> - vdev->vq[n].handle_output = NULL;
>> - vdev->vq[n].handle_aio_output = NULL;
>> - g_free(vdev->vq[n].used_elems);
>> + virtio_delete_queue(&vdev->vq[n]);
>> }
>>
>> static void virtio_set_isr(VirtIODevice *vdev, int value)
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index c32a815..e18756d 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev,
>> int queue_size,
>>
>> void virtio_del_queue(VirtIODevice *vdev, int n);
>>
>> +void virtio_delete_queue(VirtQueue *vq);
>> +
>> void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
>> unsigned int len);
>> void virtqueue_flush(VirtQueue *vq, unsigned int count);
>
>
> .
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer
2019-12-06 2:17 ` Pan Nengyuan
@ 2019-12-06 8:56 ` Amit Shah
2019-12-06 9:00 ` Pan Nengyuan
0 siblings, 1 reply; 21+ messages in thread
From: Amit Shah @ 2019-12-06 8:56 UTC (permalink / raw)
To: Pan Nengyuan, mst; +Cc: liyiting, kuhn.chenqun, qemu-devel, zhang.zhanghailiang
On Fri, 2019-12-06 at 10:17 +0800, Pan Nengyuan wrote:
> On 2019/12/6 0:45, Amit Shah wrote:
> > On Wed, 2019-12-04 at 15:31 +0800, pannengyuan@huawei.com wrote:
> > > From: Pan Nengyuan <pannengyuan@huawei.com>
> >
> > Shouldn't this be From: mst?
> >
> > I didn't find a ref to the original patch to confirm if you had to
> > adapt it in any way, though.
> >
>
> Here is the original
> patch:
> https://lists.nongnu.org/archive/html/qemu-devel/2019-12/msg00402.html
>
> I just change one line(set used_elems to NULL). In next version, I
> will
> change function name from virtio_delete_queue to virtio_queue_cleanup
> (It's too similar between "virtio_del_queue" and
> "virtio_delete_queue"):
>
https://lists.nongnu.org/archive/html/qemu-devel/2019-12/msg00877.html
> .
>
> According to these, should I change it in next version?
Sure, please change it. Please ensure 'From:' is Michael, but in the
sign-off area, you can mention how you changed the original patch, e.g.
see the "[PMM: ...]" in
https://lore.kernel.org/qemu-devel/20191126141239.8219-5-peter.maydell@linaro.org/
Also, please CC me on the entire series.
Thanks,
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer
2019-12-06 8:56 ` Amit Shah
@ 2019-12-06 9:00 ` Pan Nengyuan
0 siblings, 0 replies; 21+ messages in thread
From: Pan Nengyuan @ 2019-12-06 9:00 UTC (permalink / raw)
To: Amit Shah, mst; +Cc: liyiting, kuhn.chenqun, qemu-devel, zhang.zhanghailiang
On 2019/12/6 16:56, Amit Shah wrote:
> On Fri, 2019-12-06 at 10:17 +0800, Pan Nengyuan wrote:
>> On 2019/12/6 0:45, Amit Shah wrote:
>>> On Wed, 2019-12-04 at 15:31 +0800, pannengyuan@huawei.com wrote:
>>>> From: Pan Nengyuan <pannengyuan@huawei.com>
>>>
>>> Shouldn't this be From: mst?
>>>
>>> I didn't find a ref to the original patch to confirm if you had to
>>> adapt it in any way, though.
>>>
>>
>> Here is the original
>> patch:
>> https://lists.nongnu.org/archive/html/qemu-devel/2019-12/msg00402.html
>>
>> I just change one line(set used_elems to NULL). In next version, I
>> will
>> change function name from virtio_delete_queue to virtio_queue_cleanup
>> (It's too similar between "virtio_del_queue" and
>> "virtio_delete_queue"):
>>
> https://lists.nongnu.org/archive/html/qemu-devel/2019-12/msg00877.html
>> .
>>
>> According to these, should I change it in next version?
>
> Sure, please change it. Please ensure 'From:' is Michael, but in the
> sign-off area, you can mention how you changed the original patch, e.g.
> see the "[PMM: ...]" in
>
>
>
> https://lore.kernel.org/qemu-devel/20191126141239.8219-5-peter.maydell@linaro.org/
>
> Also, please CC me on the entire series.
>
> Thanks,
>
>
OK, I will change it.
Thanks.
>
> .
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer
2019-12-04 7:31 [PATCH v2 1/3] virtio: add ability to delete vq through a pointer pannengyuan
` (5 preceding siblings ...)
2019-12-05 16:45 ` Amit Shah
@ 2019-12-09 16:43 ` Michael S. Tsirkin
2019-12-09 16:58 ` Michael S. Tsirkin
6 siblings, 1 reply; 21+ messages in thread
From: Michael S. Tsirkin @ 2019-12-09 16:43 UTC (permalink / raw)
To: pannengyuan; +Cc: liyiting, kuhn.chenqun, qemu-devel, zhang.zhanghailiang
On Wed, Dec 04, 2019 at 03:31:54PM +0800, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
>
> Devices tend to maintain vq pointers, allow deleting them trough a vq pointer.
You want to also mention something about clearing
.used_elems to avoid chances of double free.
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
So let's just name the new one virtio_del_queue then,
and drop the old one.
> ---
> Changes v2 to v1:
> - add a new function virtio_delete_queue to cleanup vq through a vq pointer
> ---
> hw/virtio/virtio.c | 16 +++++++++++-----
> include/hw/virtio/virtio.h | 2 ++
> 2 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 04716b5..6de3cfd 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
> return &vdev->vq[i];
> }
>
> +void virtio_delete_queue(VirtQueue *vq)
> +{
> + vq->vring.num = 0;
> + vq->vring.num_default = 0;
> + vq->handle_output = NULL;
> + vq->handle_aio_output = NULL;
> + g_free(vq->used_elems);
> + vq->used_elems = NULL;
> +}
> +
> void virtio_del_queue(VirtIODevice *vdev, int n)
> {
> if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
> abort();
> }
>
> - vdev->vq[n].vring.num = 0;
> - vdev->vq[n].vring.num_default = 0;
> - vdev->vq[n].handle_output = NULL;
> - vdev->vq[n].handle_aio_output = NULL;
> - g_free(vdev->vq[n].used_elems);
> + virtio_delete_queue(&vdev->vq[n]);
> }
>
> static void virtio_set_isr(VirtIODevice *vdev, int value)
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index c32a815..e18756d 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
>
> void virtio_del_queue(VirtIODevice *vdev, int n);
>
> +void virtio_delete_queue(VirtQueue *vq);
> +
> void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
> unsigned int len);
> void virtqueue_flush(VirtQueue *vq, unsigned int count);
> --
> 2.7.2.windows.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer
2019-12-09 16:43 ` Michael S. Tsirkin
@ 2019-12-09 16:58 ` Michael S. Tsirkin
2019-12-10 2:08 ` Pan Nengyuan
0 siblings, 1 reply; 21+ messages in thread
From: Michael S. Tsirkin @ 2019-12-09 16:58 UTC (permalink / raw)
To: pannengyuan; +Cc: liyiting, kuhn.chenqun, qemu-devel, zhang.zhanghailiang
On Mon, Dec 09, 2019 at 11:43:20AM -0500, Michael S. Tsirkin wrote:
> On Wed, Dec 04, 2019 at 03:31:54PM +0800, pannengyuan@huawei.com wrote:
> > From: Pan Nengyuan <pannengyuan@huawei.com>
> >
> > Devices tend to maintain vq pointers, allow deleting them trough a vq pointer.
>
> You want to also mention something about clearing
> .used_elems to avoid chances of double free.
>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>
>
> So let's just name the new one virtio_del_queue then,
> and drop the old one.
I tried but it seems like too much work.
>
> > ---
> > Changes v2 to v1:
> > - add a new function virtio_delete_queue to cleanup vq through a vq pointer
> > ---
> > hw/virtio/virtio.c | 16 +++++++++++-----
> > include/hw/virtio/virtio.h | 2 ++
> > 2 files changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 04716b5..6de3cfd 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
> > return &vdev->vq[i];
> > }
> >
> > +void virtio_delete_queue(VirtQueue *vq)
> > +{
> > + vq->vring.num = 0;
> > + vq->vring.num_default = 0;
> > + vq->handle_output = NULL;
> > + vq->handle_aio_output = NULL;
> > + g_free(vq->used_elems);
> > + vq->used_elems = NULL;
> > +}
> > +
> > void virtio_del_queue(VirtIODevice *vdev, int n)
> > {
> > if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
> > abort();
> > }
> >
> > - vdev->vq[n].vring.num = 0;
> > - vdev->vq[n].vring.num_default = 0;
> > - vdev->vq[n].handle_output = NULL;
> > - vdev->vq[n].handle_aio_output = NULL;
> > - g_free(vdev->vq[n].used_elems);
> > + virtio_delete_queue(&vdev->vq[n]);
> > }
> >
> > static void virtio_set_isr(VirtIODevice *vdev, int value)
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index c32a815..e18756d 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
> >
> > void virtio_del_queue(VirtIODevice *vdev, int n);
> >
> > +void virtio_delete_queue(VirtQueue *vq);
> > +
> > void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
> > unsigned int len);
> > void virtqueue_flush(VirtQueue *vq, unsigned int count);
> > --
> > 2.7.2.windows.1
> >
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer
2019-12-09 16:58 ` Michael S. Tsirkin
@ 2019-12-10 2:08 ` Pan Nengyuan
0 siblings, 0 replies; 21+ messages in thread
From: Pan Nengyuan @ 2019-12-10 2:08 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: liyiting, kuhn.chenqun, qemu-devel, zhang.zhanghailiang
On 2019/12/10 0:58, Michael S. Tsirkin wrote:
> On Mon, Dec 09, 2019 at 11:43:20AM -0500, Michael S. Tsirkin wrote:
>> On Wed, Dec 04, 2019 at 03:31:54PM +0800, pannengyuan@huawei.com wrote:
>>> From: Pan Nengyuan <pannengyuan@huawei.com>
>>>
>>> Devices tend to maintain vq pointers, allow deleting them trough a vq pointer.
>>
>> You want to also mention something about clearing
>> .used_elems to avoid chances of double free.
>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>>
>>
>> So let's just name the new one virtio_del_queue then,
>> and drop the old one.
>
> I tried but it seems like too much work.
Yes, some of them do not maintain the vq pointer, so can we just rename
the virtio_delete_queue to avoid confusion?
I have sent a new version before your reply, can you check whether it's
appropriate or not?
>
>>
>>> ---
>>> Changes v2 to v1:
>>> - add a new function virtio_delete_queue to cleanup vq through a vq pointer
>>> ---
>>> hw/virtio/virtio.c | 16 +++++++++++-----
>>> include/hw/virtio/virtio.h | 2 ++
>>> 2 files changed, 13 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>> index 04716b5..6de3cfd 100644
>>> --- a/hw/virtio/virtio.c
>>> +++ b/hw/virtio/virtio.c
>>> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
>>> return &vdev->vq[i];
>>> }
>>>
>>> +void virtio_delete_queue(VirtQueue *vq)
>>> +{
>>> + vq->vring.num = 0;
>>> + vq->vring.num_default = 0;
>>> + vq->handle_output = NULL;
>>> + vq->handle_aio_output = NULL;
>>> + g_free(vq->used_elems);
>>> + vq->used_elems = NULL;
>>> +}
>>> +
>>> void virtio_del_queue(VirtIODevice *vdev, int n)
>>> {
>>> if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
>>> abort();
>>> }
>>>
>>> - vdev->vq[n].vring.num = 0;
>>> - vdev->vq[n].vring.num_default = 0;
>>> - vdev->vq[n].handle_output = NULL;
>>> - vdev->vq[n].handle_aio_output = NULL;
>>> - g_free(vdev->vq[n].used_elems);
>>> + virtio_delete_queue(&vdev->vq[n]);
>>> }
>>>
>>> static void virtio_set_isr(VirtIODevice *vdev, int value)
>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>>> index c32a815..e18756d 100644
>>> --- a/include/hw/virtio/virtio.h
>>> +++ b/include/hw/virtio/virtio.h
>>> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
>>>
>>> void virtio_del_queue(VirtIODevice *vdev, int n);
>>>
>>> +void virtio_delete_queue(VirtQueue *vq);
>>> +
>>> void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
>>> unsigned int len);
>>> void virtqueue_flush(VirtQueue *vq, unsigned int count);
>>> --
>>> 2.7.2.windows.1
>>>
>
>
> .
>
^ permalink raw reply [flat|nested] 21+ messages in thread