* [Qemu-devel] [PATCH V2] vhost: logs sharing
@ 2015-04-10 9:33 Jason Wang
2015-04-28 9:37 ` Michael S. Tsirkin
0 siblings, 1 reply; 8+ messages in thread
From: Jason Wang @ 2015-04-10 9:33 UTC (permalink / raw)
To: qemu-devel; +Cc: Jason Wang, mst
Currently we allocate one vhost log per vhost device. This is sub
optimal when:
- Guest has several device with vhost as backend
- Guest has multiqueue devices
In the above cases, we can avoid the memory allocation by sharing a
single vhost log among all the vhost devices. This is done through:
- Introducing a new vhost_log structure with refcnt inside.
- Using a global pointer to vhost_log structure that will be used. And
introduce helper to get the log with expected log size and helper to
- drop the refcnt to the old log.
- Each vhost device still keep track of a pointer to the log that was
used.
With above, if no resize happens, all vhost device will share a single
vhost log. During resize, a new vhost_log structure will be allocated
and made for the global pointer. And each vhost devices will drop the
refcnt to the old log.
Tested by doing scp during migration for a 2 queues virtio-net-pci.
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
Changes from V1:
- Drop the list of vhost log, instead, using a global pointer instead
---
hw/virtio/vhost.c | 66 ++++++++++++++++++++++++++++++++++++++---------
include/hw/virtio/vhost.h | 9 ++++++-
2 files changed, 62 insertions(+), 13 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 5a12861..e16c2db 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -22,15 +22,19 @@
#include "hw/virtio/virtio-bus.h"
#include "migration/migration.h"
+static struct vhost_log *vhost_log;
+
static void vhost_dev_sync_region(struct vhost_dev *dev,
MemoryRegionSection *section,
uint64_t mfirst, uint64_t mlast,
uint64_t rfirst, uint64_t rlast)
{
+ vhost_log_chunk_t *log = dev->log->log;
+
uint64_t start = MAX(mfirst, rfirst);
uint64_t end = MIN(mlast, rlast);
- vhost_log_chunk_t *from = dev->log + start / VHOST_LOG_CHUNK;
- vhost_log_chunk_t *to = dev->log + end / VHOST_LOG_CHUNK + 1;
+ vhost_log_chunk_t *from = log + start / VHOST_LOG_CHUNK;
+ vhost_log_chunk_t *to = log + end / VHOST_LOG_CHUNK + 1;
uint64_t addr = (start / VHOST_LOG_CHUNK) * VHOST_LOG_CHUNK;
if (end < start) {
@@ -280,22 +284,55 @@ static uint64_t vhost_get_log_size(struct vhost_dev *dev)
}
return log_size;
}
+static struct vhost_log *vhost_log_alloc(uint64_t size)
+{
+ struct vhost_log *log = g_malloc0(sizeof *log + size * sizeof(*(log->log)));
+
+ log->size = size;
+ log->refcnt = 1;
+
+ return log;
+}
+
+static struct vhost_log *vhost_log_get(uint64_t size)
+{
+ if (!vhost_log || vhost_log->size != size) {
+ vhost_log = vhost_log_alloc(size);
+ } else {
+ ++vhost_log->refcnt;
+ }
+
+ return vhost_log;
+}
+
+static void vhost_log_put(struct vhost_log *log)
+{
+ if (!log) {
+ return;
+ }
+
+ --log->refcnt;
+ if (log->refcnt == 0) {
+ if (vhost_log == log) {
+ vhost_log = NULL;
+ }
+ g_free(log);
+ }
+}
static inline void vhost_dev_log_resize(struct vhost_dev* dev, uint64_t size)
{
- vhost_log_chunk_t *log;
- uint64_t log_base;
+ struct vhost_log *log = vhost_log_get(size);
+ uint64_t log_base = (uint64_t)(unsigned long)log->log;
int r;
- log = g_malloc0(size * sizeof *log);
- log_base = (uint64_t)(unsigned long)log;
r = dev->vhost_ops->vhost_call(dev, VHOST_SET_LOG_BASE, &log_base);
assert(r >= 0);
/* Sync only the range covered by the old log */
if (dev->log_size) {
vhost_log_sync_range(dev, 0, dev->log_size * VHOST_LOG_CHUNK - 1);
}
- g_free(dev->log);
+ vhost_log_put(dev->log);
dev->log = log;
dev->log_size = size;
}
@@ -601,7 +638,7 @@ static int vhost_migration_log(MemoryListener *listener, int enable)
if (r < 0) {
return r;
}
- g_free(dev->log);
+ vhost_log_put(dev->log);
dev->log = NULL;
dev->log_size = 0;
} else {
@@ -1058,9 +1095,11 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
if (hdev->log_enabled) {
hdev->log_size = vhost_get_log_size(hdev);
- hdev->log = hdev->log_size ?
- g_malloc0(hdev->log_size * sizeof *hdev->log) : NULL;
- r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE, hdev->log);
+ if (hdev->log_size) {
+ hdev->log = vhost_log_get(hdev->log_size);
+ }
+ r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE,
+ hdev->log_size ? hdev->log->log : NULL);
if (r < 0) {
r = -errno;
goto fail_log;
@@ -1069,6 +1108,9 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
return 0;
fail_log:
+ if (hdev->log_size) {
+ vhost_log_put(hdev->log);
+ }
fail_vq:
while (--i >= 0) {
vhost_virtqueue_stop(hdev,
@@ -1098,7 +1140,7 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
vhost_log_sync_range(hdev, 0, ~0x0ull);
hdev->started = false;
- g_free(hdev->log);
+ vhost_log_put(hdev->log);
hdev->log = NULL;
hdev->log_size = 0;
}
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index d5593d1..3879778 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -28,6 +28,13 @@ typedef unsigned long vhost_log_chunk_t;
#define VHOST_LOG_CHUNK (VHOST_LOG_PAGE * VHOST_LOG_BITS)
#define VHOST_INVALID_FEATURE_BIT (0xff)
+struct vhost_log {
+ QLIST_ENTRY(vhost_log) node;
+ unsigned long long size;
+ int refcnt;
+ vhost_log_chunk_t log[0];
+};
+
struct vhost_memory;
struct vhost_dev {
MemoryListener memory_listener;
@@ -43,7 +50,6 @@ struct vhost_dev {
unsigned long long backend_features;
bool started;
bool log_enabled;
- vhost_log_chunk_t *log;
unsigned long long log_size;
Error *migration_blocker;
bool force;
@@ -52,6 +58,7 @@ struct vhost_dev {
hwaddr mem_changed_end_addr;
const VhostOps *vhost_ops;
void *opaque;
+ struct vhost_log *log;
};
int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
--
2.1.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH V2] vhost: logs sharing
2015-04-10 9:33 [Qemu-devel] [PATCH V2] vhost: logs sharing Jason Wang
@ 2015-04-28 9:37 ` Michael S. Tsirkin
2015-04-28 9:58 ` Jason Wang
0 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2015-04-28 9:37 UTC (permalink / raw)
To: Jason Wang; +Cc: qemu-devel
On Fri, Apr 10, 2015 at 05:33:35PM +0800, Jason Wang wrote:
> Currently we allocate one vhost log per vhost device. This is sub
> optimal when:
>
> - Guest has several device with vhost as backend
> - Guest has multiqueue devices
>
> In the above cases, we can avoid the memory allocation by sharing a
> single vhost log among all the vhost devices. This is done through:
>
> - Introducing a new vhost_log structure with refcnt inside.
> - Using a global pointer to vhost_log structure that will be used. And
> introduce helper to get the log with expected log size and helper to
> - drop the refcnt to the old log.
> - Each vhost device still keep track of a pointer to the log that was
> used.
>
> With above, if no resize happens, all vhost device will share a single
> vhost log. During resize, a new vhost_log structure will be allocated
> and made for the global pointer. And each vhost devices will drop the
> refcnt to the old log.
>
> Tested by doing scp during migration for a 2 queues virtio-net-pci.
>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> Changes from V1:
> - Drop the list of vhost log, instead, using a global pointer instead
I don't think it works like this. If you have a global pointer,
you also need a global listener, have that sync all devices.
> ---
> hw/virtio/vhost.c | 66 ++++++++++++++++++++++++++++++++++++++---------
> include/hw/virtio/vhost.h | 9 ++++++-
> 2 files changed, 62 insertions(+), 13 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 5a12861..e16c2db 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -22,15 +22,19 @@
> #include "hw/virtio/virtio-bus.h"
> #include "migration/migration.h"
>
> +static struct vhost_log *vhost_log;
> +
> static void vhost_dev_sync_region(struct vhost_dev *dev,
> MemoryRegionSection *section,
> uint64_t mfirst, uint64_t mlast,
> uint64_t rfirst, uint64_t rlast)
> {
> + vhost_log_chunk_t *log = dev->log->log;
> +
> uint64_t start = MAX(mfirst, rfirst);
> uint64_t end = MIN(mlast, rlast);
> - vhost_log_chunk_t *from = dev->log + start / VHOST_LOG_CHUNK;
> - vhost_log_chunk_t *to = dev->log + end / VHOST_LOG_CHUNK + 1;
> + vhost_log_chunk_t *from = log + start / VHOST_LOG_CHUNK;
> + vhost_log_chunk_t *to = log + end / VHOST_LOG_CHUNK + 1;
> uint64_t addr = (start / VHOST_LOG_CHUNK) * VHOST_LOG_CHUNK;
>
> if (end < start) {
> @@ -280,22 +284,55 @@ static uint64_t vhost_get_log_size(struct vhost_dev *dev)
> }
> return log_size;
> }
> +static struct vhost_log *vhost_log_alloc(uint64_t size)
> +{
> + struct vhost_log *log = g_malloc0(sizeof *log + size * sizeof(*(log->log)));
> +
> + log->size = size;
> + log->refcnt = 1;
> +
> + return log;
> +}
> +
> +static struct vhost_log *vhost_log_get(uint64_t size)
> +{
> + if (!vhost_log || vhost_log->size != size) {
> + vhost_log = vhost_log_alloc(size);
This just leaks the old log if size != size.
> + } else {
> + ++vhost_log->refcnt;
> + }
> +
> + return vhost_log;
> +}
> +
> +static void vhost_log_put(struct vhost_log *log)
> +{
> + if (!log) {
> + return;
> + }
> +
> + --log->refcnt;
> + if (log->refcnt == 0) {
> + if (vhost_log == log) {
> + vhost_log = NULL;
> + }
> + g_free(log);
> + }
> +}
>
> static inline void vhost_dev_log_resize(struct vhost_dev* dev, uint64_t size)
> {
> - vhost_log_chunk_t *log;
> - uint64_t log_base;
> + struct vhost_log *log = vhost_log_get(size);
At this point next device will try to use the
new log, but there is nothing there.
> + uint64_t log_base = (uint64_t)(unsigned long)log->log;
> int r;
>
> - log = g_malloc0(size * sizeof *log);
> - log_base = (uint64_t)(unsigned long)log;
> r = dev->vhost_ops->vhost_call(dev, VHOST_SET_LOG_BASE, &log_base);
> assert(r >= 0);
> /* Sync only the range covered by the old log */
> if (dev->log_size) {
> vhost_log_sync_range(dev, 0, dev->log_size * VHOST_LOG_CHUNK - 1);
> }
> - g_free(dev->log);
> + vhost_log_put(dev->log);
> dev->log = log;
> dev->log_size = size;
> }
> @@ -601,7 +638,7 @@ static int vhost_migration_log(MemoryListener *listener, int enable)
> if (r < 0) {
> return r;
> }
> - g_free(dev->log);
> + vhost_log_put(dev->log);
> dev->log = NULL;
> dev->log_size = 0;
> } else {
> @@ -1058,9 +1095,11 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
>
> if (hdev->log_enabled) {
> hdev->log_size = vhost_get_log_size(hdev);
> - hdev->log = hdev->log_size ?
> - g_malloc0(hdev->log_size * sizeof *hdev->log) : NULL;
> - r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE, hdev->log);
> + if (hdev->log_size) {
> + hdev->log = vhost_log_get(hdev->log_size);
> + }
> + r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE,
> + hdev->log_size ? hdev->log->log : NULL);
> if (r < 0) {
> r = -errno;
> goto fail_log;
> @@ -1069,6 +1108,9 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
>
> return 0;
> fail_log:
> + if (hdev->log_size) {
> + vhost_log_put(hdev->log);
> + }
> fail_vq:
> while (--i >= 0) {
> vhost_virtqueue_stop(hdev,
> @@ -1098,7 +1140,7 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
> vhost_log_sync_range(hdev, 0, ~0x0ull);
>
> hdev->started = false;
> - g_free(hdev->log);
> + vhost_log_put(hdev->log);
> hdev->log = NULL;
> hdev->log_size = 0;
> }
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index d5593d1..3879778 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -28,6 +28,13 @@ typedef unsigned long vhost_log_chunk_t;
> #define VHOST_LOG_CHUNK (VHOST_LOG_PAGE * VHOST_LOG_BITS)
> #define VHOST_INVALID_FEATURE_BIT (0xff)
>
> +struct vhost_log {
> + QLIST_ENTRY(vhost_log) node;
> + unsigned long long size;
> + int refcnt;
> + vhost_log_chunk_t log[0];
> +};
> +
Pls fix coding style here: typedef ... VhostLog.
> struct vhost_memory;
> struct vhost_dev {
> MemoryListener memory_listener;
> @@ -43,7 +50,6 @@ struct vhost_dev {
> unsigned long long backend_features;
> bool started;
> bool log_enabled;
> - vhost_log_chunk_t *log;
> unsigned long long log_size;
> Error *migration_blocker;
> bool force;
> @@ -52,6 +58,7 @@ struct vhost_dev {
> hwaddr mem_changed_end_addr;
> const VhostOps *vhost_ops;
> void *opaque;
> + struct vhost_log *log;
> };
>
> int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> --
> 2.1.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH V2] vhost: logs sharing
2015-04-28 9:37 ` Michael S. Tsirkin
@ 2015-04-28 9:58 ` Jason Wang
2015-04-28 10:30 ` Michael S. Tsirkin
0 siblings, 1 reply; 8+ messages in thread
From: Jason Wang @ 2015-04-28 9:58 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
On Tue, Apr 28, 2015 at 5:37 PM, Michael S. Tsirkin <mst@redhat.com>
wrote:
> On Fri, Apr 10, 2015 at 05:33:35PM +0800, Jason Wang wrote:
>> Currently we allocate one vhost log per vhost device. This is sub
>> optimal when:
>>
>> - Guest has several device with vhost as backend
>> - Guest has multiqueue devices
>>
>> In the above cases, we can avoid the memory allocation by sharing a
>> single vhost log among all the vhost devices. This is done through:
>>
>> - Introducing a new vhost_log structure with refcnt inside.
>> - Using a global pointer to vhost_log structure that will be used.
>> And
>> introduce helper to get the log with expected log size and helper
>> to
>> - drop the refcnt to the old log.
>> - Each vhost device still keep track of a pointer to the log that
>> was
>> used.
>>
>> With above, if no resize happens, all vhost device will share a
>> single
>> vhost log. During resize, a new vhost_log structure will be
>> allocated
>> and made for the global pointer. And each vhost devices will drop
>> the
>> refcnt to the old log.
>>
>> Tested by doing scp during migration for a 2 queues virtio-net-pci.
>>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> Changes from V1:
>> - Drop the list of vhost log, instead, using a global pointer
>> instead
>
> I don't think it works like this. If you have a global pointer,
> you also need a global listener, have that sync all devices.
It doesn't conflict, see my comments below.
>
>
>
>> ---
>> hw/virtio/vhost.c | 66
>> ++++++++++++++++++++++++++++++++++++++---------
>> include/hw/virtio/vhost.h | 9 ++++++-
>> 2 files changed, 62 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index 5a12861..e16c2db 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -22,15 +22,19 @@
>> #include "hw/virtio/virtio-bus.h"
>> #include "migration/migration.h"
>>
>> +static struct vhost_log *vhost_log;
>> +
>> static void vhost_dev_sync_region(struct vhost_dev *dev,
>> MemoryRegionSection *section,
>> uint64_t mfirst, uint64_t mlast,
>> uint64_t rfirst, uint64_t rlast)
>> {
>> + vhost_log_chunk_t *log = dev->log->log;
>> +
>> uint64_t start = MAX(mfirst, rfirst);
>> uint64_t end = MIN(mlast, rlast);
>> - vhost_log_chunk_t *from = dev->log + start / VHOST_LOG_CHUNK;
>> - vhost_log_chunk_t *to = dev->log + end / VHOST_LOG_CHUNK + 1;
>> + vhost_log_chunk_t *from = log + start / VHOST_LOG_CHUNK;
>> + vhost_log_chunk_t *to = log + end / VHOST_LOG_CHUNK + 1;
>> uint64_t addr = (start / VHOST_LOG_CHUNK) * VHOST_LOG_CHUNK;
>>
>> if (end < start) {
>> @@ -280,22 +284,55 @@ static uint64_t vhost_get_log_size(struct
>> vhost_dev *dev)
>> }
>> return log_size;
>> }
>> +static struct vhost_log *vhost_log_alloc(uint64_t size)
>> +{
>> + struct vhost_log *log = g_malloc0(sizeof *log + size *
>> sizeof(*(log->log)));
>> +
>> + log->size = size;
>> + log->refcnt = 1;
>> +
>> + return log;
>> +}
>> +
>> +static struct vhost_log *vhost_log_get(uint64_t size)
>> +{
>> + if (!vhost_log || vhost_log->size != size) {
>> + vhost_log = vhost_log_alloc(size);
>
> This just leaks the old log if size != size.
But old log is reference counted and will be freed during
vhost_log_put() if refcnt drops to zero.
>
>> + } else {
>> + ++vhost_log->refcnt;
>> + }
>> +
>> + return vhost_log;
>> +}
>> +
>> +static void vhost_log_put(struct vhost_log *log)
>> +{
>> + if (!log) {
>> + return;
>> + }
>> +
>> + --log->refcnt;
>> + if (log->refcnt == 0) {
>> + if (vhost_log == log) {
>> + vhost_log = NULL;
>> + }
>> + g_free(log);
>> + }
>> +}
>>
>> static inline void vhost_dev_log_resize(struct vhost_dev* dev,
>> uint64_t size)
>> {
>> - vhost_log_chunk_t *log;
>> - uint64_t log_base;
>> + struct vhost_log *log = vhost_log_get(size);
>
> At this point next device will try to use the
> new log, but there is nothing there.
vhost_log_get() will either allocate and return a new log if size is
change or just increase the refcnt and use current log. So it works in
fact?
>
>
>> + uint64_t log_base = (uint64_t)(unsigned long)log->log;
>> int r;
>>
>> - log = g_malloc0(size * sizeof *log);
>> - log_base = (uint64_t)(unsigned long)log;
>> r = dev->vhost_ops->vhost_call(dev, VHOST_SET_LOG_BASE,
>> &log_base);
>> assert(r >= 0);
>> /* Sync only the range covered by the old log */
>> if (dev->log_size) {
>> vhost_log_sync_range(dev, 0, dev->log_size *
>> VHOST_LOG_CHUNK - 1);
>> }
>> - g_free(dev->log);
>> + vhost_log_put(dev->log);
>> dev->log = log;
>> dev->log_size = size;
>> }
>> @@ -601,7 +638,7 @@ static int vhost_migration_log(MemoryListener
>> *listener, int enable)
>> if (r < 0) {
>> return r;
>> }
>> - g_free(dev->log);
>> + vhost_log_put(dev->log);
>> dev->log = NULL;
>> dev->log_size = 0;
>> } else {
>> @@ -1058,9 +1095,11 @@ int vhost_dev_start(struct vhost_dev *hdev,
>> VirtIODevice *vdev)
>>
>> if (hdev->log_enabled) {
>> hdev->log_size = vhost_get_log_size(hdev);
>> - hdev->log = hdev->log_size ?
>> - g_malloc0(hdev->log_size * sizeof *hdev->log) : NULL;
>> - r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE,
>> hdev->log);
>> + if (hdev->log_size) {
>> + hdev->log = vhost_log_get(hdev->log_size);
>> + }
>> + r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE,
>> + hdev->log_size ?
>> hdev->log->log : NULL);
>> if (r < 0) {
>> r = -errno;
>> goto fail_log;
>> @@ -1069,6 +1108,9 @@ int vhost_dev_start(struct vhost_dev *hdev,
>> VirtIODevice *vdev)
>>
>> return 0;
>> fail_log:
>> + if (hdev->log_size) {
>> + vhost_log_put(hdev->log);
>> + }
>> fail_vq:
>> while (--i >= 0) {
>> vhost_virtqueue_stop(hdev,
>> @@ -1098,7 +1140,7 @@ void vhost_dev_stop(struct vhost_dev *hdev,
>> VirtIODevice *vdev)
>> vhost_log_sync_range(hdev, 0, ~0x0ull);
>>
>> hdev->started = false;
>> - g_free(hdev->log);
>> + vhost_log_put(hdev->log);
>> hdev->log = NULL;
>> hdev->log_size = 0;
>> }
>> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
>> index d5593d1..3879778 100644
>> --- a/include/hw/virtio/vhost.h
>> +++ b/include/hw/virtio/vhost.h
>> @@ -28,6 +28,13 @@ typedef unsigned long vhost_log_chunk_t;
>> #define VHOST_LOG_CHUNK (VHOST_LOG_PAGE * VHOST_LOG_BITS)
>> #define VHOST_INVALID_FEATURE_BIT (0xff)
>>
>> +struct vhost_log {
>> + QLIST_ENTRY(vhost_log) node;
>> + unsigned long long size;
>> + int refcnt;
>> + vhost_log_chunk_t log[0];
>> +};
>> +
>
> Pls fix coding style here: typedef ... VhostLog.
Is this a new style requirement? (I'm asking since e.g the blow
vhost_dev structure does not have a typedef)
>
>
>> struct vhost_memory;
>> struct vhost_dev {
>> MemoryListener memory_listener;
>> @@ -43,7 +50,6 @@ struct vhost_dev {
>> unsigned long long backend_features;
>> bool started;
>> bool log_enabled;
>> - vhost_log_chunk_t *log;
>> unsigned long long log_size;
>> Error *migration_blocker;
>> bool force;
>> @@ -52,6 +58,7 @@ struct vhost_dev {
>> hwaddr mem_changed_end_addr;
>> const VhostOps *vhost_ops;
>> void *opaque;
>> + struct vhost_log *log;
>> };
>>
>> int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>> --
>> 2.1.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH V2] vhost: logs sharing
2015-04-28 9:58 ` Jason Wang
@ 2015-04-28 10:30 ` Michael S. Tsirkin
2015-04-30 8:05 ` Jason Wang
0 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2015-04-28 10:30 UTC (permalink / raw)
To: Jason Wang; +Cc: qemu-devel
On Tue, Apr 28, 2015 at 05:58:28PM +0800, Jason Wang wrote:
>
>
> On Tue, Apr 28, 2015 at 5:37 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >On Fri, Apr 10, 2015 at 05:33:35PM +0800, Jason Wang wrote:
> >> Currently we allocate one vhost log per vhost device. This is sub
> >> optimal when:
> >> - Guest has several device with vhost as backend
> >> - Guest has multiqueue devices
> >> In the above cases, we can avoid the memory allocation by sharing a
> >> single vhost log among all the vhost devices. This is done through:
> >> - Introducing a new vhost_log structure with refcnt inside.
> >> - Using a global pointer to vhost_log structure that will be used. And
> >> introduce helper to get the log with expected log size and helper to
> >> - drop the refcnt to the old log.
> >> - Each vhost device still keep track of a pointer to the log that was
> >> used.
> >> With above, if no resize happens, all vhost device will share a
> >>single
> >> vhost log. During resize, a new vhost_log structure will be allocated
> >> and made for the global pointer. And each vhost devices will drop the
> >> refcnt to the old log.
> >> Tested by doing scp during migration for a 2 queues virtio-net-pci.
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> ---
> >> Changes from V1:
> >> - Drop the list of vhost log, instead, using a global pointer instead
> >
> >I don't think it works like this. If you have a global pointer,
> >you also need a global listener, have that sync all devices.
>
> It doesn't conflict, see my comments below.
> >
> >
> >
> >> ---
> >> hw/virtio/vhost.c | 66
> >>++++++++++++++++++++++++++++++++++++++---------
> >> include/hw/virtio/vhost.h | 9 ++++++-
> >> 2 files changed, 62 insertions(+), 13 deletions(-)
> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >> index 5a12861..e16c2db 100644
> >> --- a/hw/virtio/vhost.c
> >> +++ b/hw/virtio/vhost.c
> >> @@ -22,15 +22,19 @@
> >> #include "hw/virtio/virtio-bus.h"
> >> #include "migration/migration.h"
> >> +static struct vhost_log *vhost_log;
> >> +
> >> static void vhost_dev_sync_region(struct vhost_dev *dev,
> >> MemoryRegionSection *section,
> >> uint64_t mfirst, uint64_t mlast,
> >> uint64_t rfirst, uint64_t rlast)
> >> {
> >> + vhost_log_chunk_t *log = dev->log->log;
> >> +
> >> uint64_t start = MAX(mfirst, rfirst);
> >> uint64_t end = MIN(mlast, rlast);
> >> - vhost_log_chunk_t *from = dev->log + start / VHOST_LOG_CHUNK;
> >> - vhost_log_chunk_t *to = dev->log + end / VHOST_LOG_CHUNK + 1;
> >> + vhost_log_chunk_t *from = log + start / VHOST_LOG_CHUNK;
> >> + vhost_log_chunk_t *to = log + end / VHOST_LOG_CHUNK + 1;
> >> uint64_t addr = (start / VHOST_LOG_CHUNK) * VHOST_LOG_CHUNK;
> >> if (end < start) {
> >> @@ -280,22 +284,55 @@ static uint64_t vhost_get_log_size(struct
> >>vhost_dev *dev)
> >> }
> >> return log_size;
> >> }
> >> +static struct vhost_log *vhost_log_alloc(uint64_t size)
> >> +{
> >> + struct vhost_log *log = g_malloc0(sizeof *log + size *
> >>sizeof(*(log->log)));
> >> +
> >> + log->size = size;
> >> + log->refcnt = 1;
> >> +
> >> + return log;
> >> +}
> >> +
> >> +static struct vhost_log *vhost_log_get(uint64_t size)
> >> +{
> >> + if (!vhost_log || vhost_log->size != size) {
> >> + vhost_log = vhost_log_alloc(size);
> >
> >This just leaks the old log if size != size.
>
> But old log is reference counted and will be freed during vhost_log_put() if
> refcnt drops to zero.
You need a pointer to reference-count it.
You return pointer to new object, no one references the old one.
> >
> >> + } else {
> >> + ++vhost_log->refcnt;
> >> + }
> >> +
> >> + return vhost_log;
> >> +}
> >> +
> >> +static void vhost_log_put(struct vhost_log *log)
> >> +{
> >> + if (!log) {
> >> + return;
> >> + }
> >> +
> >> + --log->refcnt;
> >> + if (log->refcnt == 0) {
> >> + if (vhost_log == log) {
> >> + vhost_log = NULL;
> >> + }
> >> + g_free(log);
> >> + }
> >> +}
> >> static inline void vhost_dev_log_resize(struct vhost_dev* dev,
> >>uint64_t size)
> >> {
> >> - vhost_log_chunk_t *log;
> >> - uint64_t log_base;
> >> + struct vhost_log *log = vhost_log_get(size);
> >
> >At this point next device will try to use the
> >new log, but there is nothing there.
>
> vhost_log_get() will either allocate and return a new log if size is change
> or just increase the refcnt and use current log. So it works in fact?
But old log is lost and is not synced.
> >
> >
> >> + uint64_t log_base = (uint64_t)(unsigned long)log->log;
> >> int r;
> >> - log = g_malloc0(size * sizeof *log);
> >> - log_base = (uint64_t)(unsigned long)log;
> >> r = dev->vhost_ops->vhost_call(dev, VHOST_SET_LOG_BASE,
> >>&log_base);
> >> assert(r >= 0);
> >> /* Sync only the range covered by the old log */
> >> if (dev->log_size) {
> >> vhost_log_sync_range(dev, 0, dev->log_size * VHOST_LOG_CHUNK -
> >>1);
> >> }
> >> - g_free(dev->log);
> >> + vhost_log_put(dev->log);
> >> dev->log = log;
> >> dev->log_size = size;
> >> }
> >> @@ -601,7 +638,7 @@ static int vhost_migration_log(MemoryListener
> >>*listener, int enable)
> >> if (r < 0) {
> >> return r;
> >> }
> >> - g_free(dev->log);
> >> + vhost_log_put(dev->log);
> >> dev->log = NULL;
> >> dev->log_size = 0;
> >> } else {
> >> @@ -1058,9 +1095,11 @@ int vhost_dev_start(struct vhost_dev *hdev,
> >>VirtIODevice *vdev)
> >> if (hdev->log_enabled) {
> >> hdev->log_size = vhost_get_log_size(hdev);
> >> - hdev->log = hdev->log_size ?
> >> - g_malloc0(hdev->log_size * sizeof *hdev->log) : NULL;
> >> - r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE,
> >>hdev->log);
> >> + if (hdev->log_size) {
> >> + hdev->log = vhost_log_get(hdev->log_size);
> >> + }
> >> + r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE,
> >> + hdev->log_size ?
> >>hdev->log->log : NULL);
> >> if (r < 0) {
> >> r = -errno;
> >> goto fail_log;
> >> @@ -1069,6 +1108,9 @@ int vhost_dev_start(struct vhost_dev *hdev,
> >>VirtIODevice *vdev)
> >> return 0;
> >> fail_log:
> >> + if (hdev->log_size) {
> >> + vhost_log_put(hdev->log);
> >> + }
> >> fail_vq:
> >> while (--i >= 0) {
> >> vhost_virtqueue_stop(hdev,
> >> @@ -1098,7 +1140,7 @@ void vhost_dev_stop(struct vhost_dev *hdev,
> >>VirtIODevice *vdev)
> >> vhost_log_sync_range(hdev, 0, ~0x0ull);
> >> hdev->started = false;
> >> - g_free(hdev->log);
> >> + vhost_log_put(hdev->log);
> >> hdev->log = NULL;
> >> hdev->log_size = 0;
> >> }
> >> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> >> index d5593d1..3879778 100644
> >> --- a/include/hw/virtio/vhost.h
> >> +++ b/include/hw/virtio/vhost.h
> >> @@ -28,6 +28,13 @@ typedef unsigned long vhost_log_chunk_t;
> >> #define VHOST_LOG_CHUNK (VHOST_LOG_PAGE * VHOST_LOG_BITS)
> >> #define VHOST_INVALID_FEATURE_BIT (0xff)
> >> +struct vhost_log {
> >> + QLIST_ENTRY(vhost_log) node;
> >> + unsigned long long size;
> >> + int refcnt;
> >> + vhost_log_chunk_t log[0];
> >> +};
> >> +
> >
> >Pls fix coding style here: typedef ... VhostLog.
>
> Is this a new style requirement? (I'm asking since e.g the blow vhost_dev
> structure does not have a typedef)
You are right here, you can keep it as is for now.
> >
> >
> >> struct vhost_memory;
> >> struct vhost_dev {
> >> MemoryListener memory_listener;
> >> @@ -43,7 +50,6 @@ struct vhost_dev {
> >> unsigned long long backend_features;
> >> bool started;
> >> bool log_enabled;
> >> - vhost_log_chunk_t *log;
> >> unsigned long long log_size;
> >> Error *migration_blocker;
> >> bool force;
> >> @@ -52,6 +58,7 @@ struct vhost_dev {
> >> hwaddr mem_changed_end_addr;
> >> const VhostOps *vhost_ops;
> >> void *opaque;
> >> + struct vhost_log *log;
> >> };
> >> int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> >> -- 2.1.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH V2] vhost: logs sharing
2015-04-28 10:30 ` Michael S. Tsirkin
@ 2015-04-30 8:05 ` Jason Wang
2015-04-30 8:09 ` Michael S. Tsirkin
0 siblings, 1 reply; 8+ messages in thread
From: Jason Wang @ 2015-04-30 8:05 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
On Tue, Apr 28, 2015 at 6:30 PM, Michael S. Tsirkin <mst@redhat.com>
wrote:
> On Tue, Apr 28, 2015 at 05:58:28PM +0800, Jason Wang wrote:
>>
>>
>> On Tue, Apr 28, 2015 at 5:37 PM, Michael S. Tsirkin
>> <mst@redhat.com> wrote:
>> >On Fri, Apr 10, 2015 at 05:33:35PM +0800, Jason Wang wrote:
>> >> Currently we allocate one vhost log per vhost device. This is sub
>> >> optimal when:
>> >> - Guest has several device with vhost as backend
>> >> - Guest has multiqueue devices
>> >> In the above cases, we can avoid the memory allocation by
>> sharing a
>> >> single vhost log among all the vhost devices. This is done
>> through:
>> >> - Introducing a new vhost_log structure with refcnt inside.
>> >> - Using a global pointer to vhost_log structure that will be
>> used. And
>> >> introduce helper to get the log with expected log size and
>> helper to
>> >> - drop the refcnt to the old log.
>> >> - Each vhost device still keep track of a pointer to the log
>> that was
>> >> used.
>> >> With above, if no resize happens, all vhost device will share a
>> >>single
>> >> vhost log. During resize, a new vhost_log structure will be
>> allocated
>> >> and made for the global pointer. And each vhost devices will
>> drop the
>> >> refcnt to the old log.
>> >> Tested by doing scp during migration for a 2 queues
>> virtio-net-pci.
>> >> Cc: Michael S. Tsirkin <mst@redhat.com>
>> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> >> ---
>> >> Changes from V1:
>> >> - Drop the list of vhost log, instead, using a global pointer
>> instead
>> >
>> >I don't think it works like this. If you have a global pointer,
>> >you also need a global listener, have that sync all devices.
>>
>> It doesn't conflict, see my comments below.
>> >
>> >
>> >
>> >> ---
>> >> hw/virtio/vhost.c | 66
>> >>++++++++++++++++++++++++++++++++++++++---------
>> >> include/hw/virtio/vhost.h | 9 ++++++-
>> >> 2 files changed, 62 insertions(+), 13 deletions(-)
>> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> >> index 5a12861..e16c2db 100644
>> >> --- a/hw/virtio/vhost.c
>> >> +++ b/hw/virtio/vhost.c
>> >> @@ -22,15 +22,19 @@
>> >> #include "hw/virtio/virtio-bus.h"
>> >> #include "migration/migration.h"
>> >> +static struct vhost_log *vhost_log;
>> >> +
>> >> static void vhost_dev_sync_region(struct vhost_dev *dev,
>> >> MemoryRegionSection *section,
>> >> uint64_t mfirst, uint64_t
>> mlast,
>> >> uint64_t rfirst, uint64_t
>> rlast)
>> >> {
>> >> + vhost_log_chunk_t *log = dev->log->log;
>> >> +
>> >> uint64_t start = MAX(mfirst, rfirst);
>> >> uint64_t end = MIN(mlast, rlast);
>> >> - vhost_log_chunk_t *from = dev->log + start /
>> VHOST_LOG_CHUNK;
>> >> - vhost_log_chunk_t *to = dev->log + end / VHOST_LOG_CHUNK +
>> 1;
>> >> + vhost_log_chunk_t *from = log + start / VHOST_LOG_CHUNK;
>> >> + vhost_log_chunk_t *to = log + end / VHOST_LOG_CHUNK + 1;
>> >> uint64_t addr = (start / VHOST_LOG_CHUNK) * VHOST_LOG_CHUNK;
>> >> if (end < start) {
>> >> @@ -280,22 +284,55 @@ static uint64_t vhost_get_log_size(struct
>> >>vhost_dev *dev)
>> >> }
>> >> return log_size;
>> >> }
>> >> +static struct vhost_log *vhost_log_alloc(uint64_t size)
>> >> +{
>> >> + struct vhost_log *log = g_malloc0(sizeof *log + size *
>> >>sizeof(*(log->log)));
>> >> +
>> >> + log->size = size;
>> >> + log->refcnt = 1;
>> >> +
>> >> + return log;
>> >> +}
>> >> +
>> >> +static struct vhost_log *vhost_log_get(uint64_t size)
>> >> +{
>> >> + if (!vhost_log || vhost_log->size != size) {
>> >> + vhost_log = vhost_log_alloc(size);
>> >
>> >This just leaks the old log if size != size.
>>
>> But old log is reference counted and will be freed during
>> vhost_log_put() if
>> refcnt drops to zero.
>
> You need a pointer to reference-count it.
> You return pointer to new object, no one references the old one.
The pointer is just vhost_log->log. The old pointer will be kept in
vhost_dev_log_resize() until 1) new log was set through ioctl and 2)
the old log was synced. Then vhost_log_put() will drop the reference
count to the old log.
>
>> >
>> >> + } else {
>> >> + ++vhost_log->refcnt;
>> >> + }
>> >> +
>> >> + return vhost_log;
>> >> +}
>> >> +
>> >> +static void vhost_log_put(struct vhost_log *log)
>> >> +{
>> >> + if (!log) {
>> >> + return;
>> >> + }
>> >> +
>> >> + --log->refcnt;
>> >> + if (log->refcnt == 0) {
>> >> + if (vhost_log == log) {
>> >> + vhost_log = NULL;
>> >> + }
>> >> + g_free(log);
>> >> + }
>> >> +}
>> >> static inline void vhost_dev_log_resize(struct vhost_dev*
>> dev,
>> >>uint64_t size)
>> >> {
>> >> - vhost_log_chunk_t *log;
>> >> - uint64_t log_base;
>> >> + struct vhost_log *log = vhost_log_get(size);
>> >
>> >At this point next device will try to use the
>> >new log, but there is nothing there.
>>
>> vhost_log_get() will either allocate and return a new log if size
>> is change
>> or just increase the refcnt and use current log. So it works in
>> fact?
>
> But old log is lost and is not synced.
As I replied above, it was not lost and kept in vhost_log->log.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH V2] vhost: logs sharing
2015-04-30 8:05 ` Jason Wang
@ 2015-04-30 8:09 ` Michael S. Tsirkin
2015-04-30 9:22 ` Jason Wang
0 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2015-04-30 8:09 UTC (permalink / raw)
To: Jason Wang; +Cc: qemu-devel
On Thu, Apr 30, 2015 at 04:05:09PM +0800, Jason Wang wrote:
>
>
> On Tue, Apr 28, 2015 at 6:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >On Tue, Apr 28, 2015 at 05:58:28PM +0800, Jason Wang wrote:
> >> On Tue, Apr 28, 2015 at 5:37 PM, Michael S. Tsirkin
> >><mst@redhat.com> wrote:
> >> >On Fri, Apr 10, 2015 at 05:33:35PM +0800, Jason Wang wrote:
> >> >> Currently we allocate one vhost log per vhost device. This is sub
> >> >> optimal when:
> >> >> - Guest has several device with vhost as backend
> >> >> - Guest has multiqueue devices
> >> >> In the above cases, we can avoid the memory allocation by sharing a
> >> >> single vhost log among all the vhost devices. This is done through:
> >> >> - Introducing a new vhost_log structure with refcnt inside.
> >> >> - Using a global pointer to vhost_log structure that will be used.
> >>And
> >> >> introduce helper to get the log with expected log size and helper
> >>to
> >> >> - drop the refcnt to the old log.
> >> >> - Each vhost device still keep track of a pointer to the log that
> >>was
> >> >> used.
> >> >> With above, if no resize happens, all vhost device will share a
> >> >>single
> >> >> vhost log. During resize, a new vhost_log structure will be
> >>allocated
> >> >> and made for the global pointer. And each vhost devices will drop
> >>the
> >> >> refcnt to the old log.
> >> >> Tested by doing scp during migration for a 2 queues virtio-net-pci.
> >> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> >> ---
> >> >> Changes from V1:
> >> >> - Drop the list of vhost log, instead, using a global pointer
> >>instead
> >> >
> >> >I don't think it works like this. If you have a global pointer,
> >> >you also need a global listener, have that sync all devices.
> >> It doesn't conflict, see my comments below.
> >> >
> >> >
> >> >
> >> >> ---
> >> >> hw/virtio/vhost.c | 66
> >> >>++++++++++++++++++++++++++++++++++++++---------
> >> >> include/hw/virtio/vhost.h | 9 ++++++-
> >> >> 2 files changed, 62 insertions(+), 13 deletions(-)
> >> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >> >> index 5a12861..e16c2db 100644
> >> >> --- a/hw/virtio/vhost.c
> >> >> +++ b/hw/virtio/vhost.c
> >> >> @@ -22,15 +22,19 @@
> >> >> #include "hw/virtio/virtio-bus.h"
> >> >> #include "migration/migration.h"
> >> >> +static struct vhost_log *vhost_log;
> >> >> +
> >> >> static void vhost_dev_sync_region(struct vhost_dev *dev,
> >> >> MemoryRegionSection *section,
> >> >> uint64_t mfirst, uint64_t mlast,
> >> >> uint64_t rfirst, uint64_t rlast)
> >> >> {
> >> >> + vhost_log_chunk_t *log = dev->log->log;
> >> >> +
> >> >> uint64_t start = MAX(mfirst, rfirst);
> >> >> uint64_t end = MIN(mlast, rlast);
> >> >> - vhost_log_chunk_t *from = dev->log + start / VHOST_LOG_CHUNK;
> >> >> - vhost_log_chunk_t *to = dev->log + end / VHOST_LOG_CHUNK + 1;
> >> >> + vhost_log_chunk_t *from = log + start / VHOST_LOG_CHUNK;
> >> >> + vhost_log_chunk_t *to = log + end / VHOST_LOG_CHUNK + 1;
> >> >> uint64_t addr = (start / VHOST_LOG_CHUNK) * VHOST_LOG_CHUNK;
> >> >> if (end < start) {
> >> >> @@ -280,22 +284,55 @@ static uint64_t vhost_get_log_size(struct
> >> >>vhost_dev *dev)
> >> >> }
> >> >> return log_size;
> >> >> }
> >> >> +static struct vhost_log *vhost_log_alloc(uint64_t size)
> >> >> +{
> >> >> + struct vhost_log *log = g_malloc0(sizeof *log + size *
> >> >>sizeof(*(log->log)));
> >> >> +
> >> >> + log->size = size;
> >> >> + log->refcnt = 1;
> >> >> +
> >> >> + return log;
> >> >> +}
> >> >> +
> >> >> +static struct vhost_log *vhost_log_get(uint64_t size)
> >> >> +{
> >> >> + if (!vhost_log || vhost_log->size != size) {
> >> >> + vhost_log = vhost_log_alloc(size);
> >> >
> >> >This just leaks the old log if size != size.
> >> But old log is reference counted and will be freed during
> >>vhost_log_put() if
> >> refcnt drops to zero.
> >
> >You need a pointer to reference-count it.
> >You return pointer to new object, no one references the old one.
>
> The pointer is just vhost_log->log. The old pointer will be kept in
> vhost_dev_log_resize() until 1) new log was set through ioctl and 2) the old
> log was synced.
vhost_dev_log_resize is per device, isn't it?
So the log is synced in device 1, but not in device 2.
> Then vhost_log_put() will drop the reference count to the
> old log.
>
> >
> >> >
> >> >> + } else {
> >> >> + ++vhost_log->refcnt;
> >> >> + }
> >> >> +
> >> >> + return vhost_log;
> >> >> +}
> >> >> +
> >> >> +static void vhost_log_put(struct vhost_log *log)
> >> >> +{
> >> >> + if (!log) {
> >> >> + return;
> >> >> + }
> >> >> +
> >> >> + --log->refcnt;
> >> >> + if (log->refcnt == 0) {
> >> >> + if (vhost_log == log) {
> >> >> + vhost_log = NULL;
> >> >> + }
> >> >> + g_free(log);
> >> >> + }
> >> >> +}
> >> >> static inline void vhost_dev_log_resize(struct vhost_dev* dev,
> >> >>uint64_t size)
> >> >> {
> >> >> - vhost_log_chunk_t *log;
> >> >> - uint64_t log_base;
> >> >> + struct vhost_log *log = vhost_log_get(size);
> >> >
> >> >At this point next device will try to use the
> >> >new log, but there is nothing there.
> >> vhost_log_get() will either allocate and return a new log if size is
> >>change
> >> or just increase the refcnt and use current log. So it works in fact?
> >
> >But old log is lost and is not synced.
>
> As I replied above, it was not lost and kept in vhost_log->log.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH V2] vhost: logs sharing
2015-04-30 8:09 ` Michael S. Tsirkin
@ 2015-04-30 9:22 ` Jason Wang
2015-04-30 9:38 ` Michael S. Tsirkin
0 siblings, 1 reply; 8+ messages in thread
From: Jason Wang @ 2015-04-30 9:22 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
On Thu, Apr 30, 2015 at 4:09 PM, Michael S. Tsirkin <mst@redhat.com>
wrote:
> On Thu, Apr 30, 2015 at 04:05:09PM +0800, Jason Wang wrote:
>>
>>
>> On Tue, Apr 28, 2015 at 6:30 PM, Michael S. Tsirkin
>> <mst@redhat.com> wrote:
>> >On Tue, Apr 28, 2015 at 05:58:28PM +0800, Jason Wang wrote:
>> >> On Tue, Apr 28, 2015 at 5:37 PM, Michael S. Tsirkin
>> >><mst@redhat.com> wrote:
>> >> >On Fri, Apr 10, 2015 at 05:33:35PM +0800, Jason Wang wrote:
>> >> >> Currently we allocate one vhost log per vhost device. This is
>> sub
>> >> >> optimal when:
>> >> >> - Guest has several device with vhost as backend
>> >> >> - Guest has multiqueue devices
>> >> >> In the above cases, we can avoid the memory allocation by
>> sharing a
>> >> >> single vhost log among all the vhost devices. This is done
>> through:
>> >> >> - Introducing a new vhost_log structure with refcnt inside.
>> >> >> - Using a global pointer to vhost_log structure that will be
>> used.
>> >>And
>> >> >> introduce helper to get the log with expected log size and
>> helper
>> >>to
>> >> >> - drop the refcnt to the old log.
>> >> >> - Each vhost device still keep track of a pointer to the log
>> that
>> >>was
>> >> >> used.
>> >> >> With above, if no resize happens, all vhost device will
>> share a
>> >> >>single
>> >> >> vhost log. During resize, a new vhost_log structure will be
>> >>allocated
>> >> >> and made for the global pointer. And each vhost devices will
>> drop
>> >>the
>> >> >> refcnt to the old log.
>> >> >> Tested by doing scp during migration for a 2 queues
>> virtio-net-pci.
>> >> >> Cc: Michael S. Tsirkin <mst@redhat.com>
>> >> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> >> >> ---
>> >> >> Changes from V1:
>> >> >> - Drop the list of vhost log, instead, using a global pointer
>> >>instead
>> >> >
>> >> >I don't think it works like this. If you have a global pointer,
>> >> >you also need a global listener, have that sync all devices.
>> >> It doesn't conflict, see my comments below.
>> >> >
>> >> >
>> >> >
>> >> >> ---
>> >> >> hw/virtio/vhost.c | 66
>> >> >>++++++++++++++++++++++++++++++++++++++---------
>> >> >> include/hw/virtio/vhost.h | 9 ++++++-
>> >> >> 2 files changed, 62 insertions(+), 13 deletions(-)
>> >> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> >> >> index 5a12861..e16c2db 100644
>> >> >> --- a/hw/virtio/vhost.c
>> >> >> +++ b/hw/virtio/vhost.c
>> >> >> @@ -22,15 +22,19 @@
>> >> >> #include "hw/virtio/virtio-bus.h"
>> >> >> #include "migration/migration.h"
>> >> >> +static struct vhost_log *vhost_log;
>> >> >> +
>> >> >> static void vhost_dev_sync_region(struct vhost_dev *dev,
>> >> >> MemoryRegionSection
>> *section,
>> >> >> uint64_t mfirst, uint64_t
>> mlast,
>> >> >> uint64_t rfirst, uint64_t
>> rlast)
>> >> >> {
>> >> >> + vhost_log_chunk_t *log = dev->log->log;
>> >> >> +
>> >> >> uint64_t start = MAX(mfirst, rfirst);
>> >> >> uint64_t end = MIN(mlast, rlast);
>> >> >> - vhost_log_chunk_t *from = dev->log + start /
>> VHOST_LOG_CHUNK;
>> >> >> - vhost_log_chunk_t *to = dev->log + end / VHOST_LOG_CHUNK
>> + 1;
>> >> >> + vhost_log_chunk_t *from = log + start / VHOST_LOG_CHUNK;
>> >> >> + vhost_log_chunk_t *to = log + end / VHOST_LOG_CHUNK + 1;
>> >> >> uint64_t addr = (start / VHOST_LOG_CHUNK) *
>> VHOST_LOG_CHUNK;
>> >> >> if (end < start) {
>> >> >> @@ -280,22 +284,55 @@ static uint64_t
>> vhost_get_log_size(struct
>> >> >>vhost_dev *dev)
>> >> >> }
>> >> >> return log_size;
>> >> >> }
>> >> >> +static struct vhost_log *vhost_log_alloc(uint64_t size)
>> >> >> +{
>> >> >> + struct vhost_log *log = g_malloc0(sizeof *log + size *
>> >> >>sizeof(*(log->log)));
>> >> >> +
>> >> >> + log->size = size;
>> >> >> + log->refcnt = 1;
>> >> >> +
>> >> >> + return log;
>> >> >> +}
>> >> >> +
>> >> >> +static struct vhost_log *vhost_log_get(uint64_t size)
>> >> >> +{
>> >> >> + if (!vhost_log || vhost_log->size != size) {
>> >> >> + vhost_log = vhost_log_alloc(size);
>> >> >
>> >> >This just leaks the old log if size != size.
>> >> But old log is reference counted and will be freed during
>> >>vhost_log_put() if
>> >> refcnt drops to zero.
>> >
>> >You need a pointer to reference-count it.
>> >You return pointer to new object, no one references the old one.
>>
>> The pointer is just vhost_log->log. The old pointer will be kept in
>> vhost_dev_log_resize() until 1) new log was set through ioctl and
>> 2) the old
>> log was synced.
>
> vhost_dev_log_resize is per device, isn't it?
Yes.
>
> So the log is synced in device 1, but not in device 2.
But we will do resizing one by one for all listeners. So the sync of
device 2 will happen soon afterwards.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH V2] vhost: logs sharing
2015-04-30 9:22 ` Jason Wang
@ 2015-04-30 9:38 ` Michael S. Tsirkin
0 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2015-04-30 9:38 UTC (permalink / raw)
To: Jason Wang; +Cc: qemu-devel
On Thu, Apr 30, 2015 at 05:22:33PM +0800, Jason Wang wrote:
>
>
> On Thu, Apr 30, 2015 at 4:09 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >On Thu, Apr 30, 2015 at 04:05:09PM +0800, Jason Wang wrote:
> >> On Tue, Apr 28, 2015 at 6:30 PM, Michael S. Tsirkin
> >><mst@redhat.com> wrote:
> >> >On Tue, Apr 28, 2015 at 05:58:28PM +0800, Jason Wang wrote:
> >> >> On Tue, Apr 28, 2015 at 5:37 PM, Michael S. Tsirkin
> >> >><mst@redhat.com> wrote:
> >> >> >On Fri, Apr 10, 2015 at 05:33:35PM +0800, Jason Wang wrote:
> >> >> >> Currently we allocate one vhost log per vhost device. This is sub
> >> >> >> optimal when:
> >> >> >> - Guest has several device with vhost as backend
> >> >> >> - Guest has multiqueue devices
> >> >> >> In the above cases, we can avoid the memory allocation by sharing
> >>a
> >> >> >> single vhost log among all the vhost devices. This is done
> >>through:
> >> >> >> - Introducing a new vhost_log structure with refcnt inside.
> >> >> >> - Using a global pointer to vhost_log structure that will be
> >>used.
> >> >>And
> >> >> >> introduce helper to get the log with expected log size and
> >>helper
> >> >>to
> >> >> >> - drop the refcnt to the old log.
> >> >> >> - Each vhost device still keep track of a pointer to the log that
> >> >>was
> >> >> >> used.
> >> >> >> With above, if no resize happens, all vhost device will share a
> >> >> >>single
> >> >> >> vhost log. During resize, a new vhost_log structure will be
> >> >>allocated
> >> >> >> and made for the global pointer. And each vhost devices will drop
> >> >>the
> >> >> >> refcnt to the old log.
> >> >> >> Tested by doing scp during migration for a 2 queues
> >>virtio-net-pci.
> >> >> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> >> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> >> >> ---
> >> >> >> Changes from V1:
> >> >> >> - Drop the list of vhost log, instead, using a global pointer
> >> >>instead
> >> >> >
> >> >> >I don't think it works like this. If you have a global pointer,
> >> >> >you also need a global listener, have that sync all devices.
> >> >> It doesn't conflict, see my comments below.
> >> >> >
> >> >> >
> >> >> >
> >> >> >> ---
> >> >> >> hw/virtio/vhost.c | 66
> >> >> >>++++++++++++++++++++++++++++++++++++++---------
> >> >> >> include/hw/virtio/vhost.h | 9 ++++++-
> >> >> >> 2 files changed, 62 insertions(+), 13 deletions(-)
> >> >> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >> >> >> index 5a12861..e16c2db 100644
> >> >> >> --- a/hw/virtio/vhost.c
> >> >> >> +++ b/hw/virtio/vhost.c
> >> >> >> @@ -22,15 +22,19 @@
> >> >> >> #include "hw/virtio/virtio-bus.h"
> >> >> >> #include "migration/migration.h"
> >> >> >> +static struct vhost_log *vhost_log;
> >> >> >> +
> >> >> >> static void vhost_dev_sync_region(struct vhost_dev *dev,
> >> >> >> MemoryRegionSection *section,
> >> >> >> uint64_t mfirst, uint64_t
> >>mlast,
> >> >> >> uint64_t rfirst, uint64_t
> >>rlast)
> >> >> >> {
> >> >> >> + vhost_log_chunk_t *log = dev->log->log;
> >> >> >> +
> >> >> >> uint64_t start = MAX(mfirst, rfirst);
> >> >> >> uint64_t end = MIN(mlast, rlast);
> >> >> >> - vhost_log_chunk_t *from = dev->log + start /
> >>VHOST_LOG_CHUNK;
> >> >> >> - vhost_log_chunk_t *to = dev->log + end / VHOST_LOG_CHUNK +
> >>1;
> >> >> >> + vhost_log_chunk_t *from = log + start / VHOST_LOG_CHUNK;
> >> >> >> + vhost_log_chunk_t *to = log + end / VHOST_LOG_CHUNK + 1;
> >> >> >> uint64_t addr = (start / VHOST_LOG_CHUNK) * VHOST_LOG_CHUNK;
> >> >> >> if (end < start) {
> >> >> >> @@ -280,22 +284,55 @@ static uint64_t vhost_get_log_size(struct
> >> >> >>vhost_dev *dev)
> >> >> >> }
> >> >> >> return log_size;
> >> >> >> }
> >> >> >> +static struct vhost_log *vhost_log_alloc(uint64_t size)
> >> >> >> +{
> >> >> >> + struct vhost_log *log = g_malloc0(sizeof *log + size *
> >> >> >>sizeof(*(log->log)));
> >> >> >> +
> >> >> >> + log->size = size;
> >> >> >> + log->refcnt = 1;
> >> >> >> +
> >> >> >> + return log;
> >> >> >> +}
> >> >> >> +
> >> >> >> +static struct vhost_log *vhost_log_get(uint64_t size)
> >> >> >> +{
> >> >> >> + if (!vhost_log || vhost_log->size != size) {
> >> >> >> + vhost_log = vhost_log_alloc(size);
> >> >> >
> >> >> >This just leaks the old log if size != size.
> >> >> But old log is reference counted and will be freed during
> >> >>vhost_log_put() if
> >> >> refcnt drops to zero.
> >> >
> >> >You need a pointer to reference-count it.
> >> >You return pointer to new object, no one references the old one.
> >> The pointer is just vhost_log->log. The old pointer will be kept in
> >> vhost_dev_log_resize() until 1) new log was set through ioctl and 2)
> >>the old
> >> log was synced.
> >
> >vhost_dev_log_resize is per device, isn't it?
>
> Yes.
>
> >
> >So the log is synced in device 1, but not in device 2.
>
> But we will do resizing one by one for all listeners. So the sync of device
> 2 will happen soon afterwards.
yes but it will get the new log from vhost_log_get, not the old one,
and sync the new buffer not the old one.
--
MSt
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-04-30 9:38 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-10 9:33 [Qemu-devel] [PATCH V2] vhost: logs sharing Jason Wang
2015-04-28 9:37 ` Michael S. Tsirkin
2015-04-28 9:58 ` Jason Wang
2015-04-28 10:30 ` Michael S. Tsirkin
2015-04-30 8:05 ` Jason Wang
2015-04-30 8:09 ` Michael S. Tsirkin
2015-04-30 9:22 ` Jason Wang
2015-04-30 9:38 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.