Message ID | 1428658415-5049-1-git-send-email-jasowang@redhat.com |
---|---|
State | New |
Headers | show |
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
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
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
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.
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.
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.
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.
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,
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(-)