* [PATCH v3 1/2] vhost: dirty log should be per backend type @ 2024-03-14 7:26 Si-Wei Liu 2024-03-14 7:26 ` [PATCH v3 2/2] vhost: Perform memory section dirty scans once per iteration Si-Wei Liu 2024-03-14 15:25 ` [PATCH v3 1/2] vhost: dirty log should be per backend type Eugenio Perez Martin 0 siblings, 2 replies; 7+ messages in thread From: Si-Wei Liu @ 2024-03-14 7:26 UTC (permalink / raw) To: qemu-devel; +Cc: mst, jasowang, eperezma, joao.m.martins, si-wei.liu There could be a mix of both vhost-user and vhost-kernel clients in the same QEMU process, where separate vhost loggers for the specific vhost type have to be used. Make the vhost logger per backend type, and have them properly reference counted. Suggested-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> --- v2->v3: - remove non-effective assertion that never be reached - do not return NULL from vhost_log_get() - add neccessary assertions to vhost_log_get() --- hw/virtio/vhost.c | 50 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 12 deletions(-) diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 2c9ac79..efe2f74 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -43,8 +43,8 @@ do { } while (0) #endif -static struct vhost_log *vhost_log; -static struct vhost_log *vhost_log_shm; +static struct vhost_log *vhost_log[VHOST_BACKEND_TYPE_MAX]; +static struct vhost_log *vhost_log_shm[VHOST_BACKEND_TYPE_MAX]; /* Memslots used by backends that support private memslots (without an fd). */ static unsigned int used_memslots; @@ -287,6 +287,10 @@ static int vhost_set_backend_type(struct vhost_dev *dev, r = -1; } + if (r == 0) { + assert(dev->vhost_ops->backend_type == backend_type); + } + return r; } @@ -319,16 +323,22 @@ static struct vhost_log *vhost_log_alloc(uint64_t size, bool share) return log; } -static struct vhost_log *vhost_log_get(uint64_t size, bool share) +static struct vhost_log *vhost_log_get(VhostBackendType backend_type, + uint64_t size, bool share) { - struct vhost_log *log = share ? vhost_log_shm : vhost_log; + struct vhost_log *log; + + assert(backend_type > VHOST_BACKEND_TYPE_NONE); + assert(backend_type < VHOST_BACKEND_TYPE_MAX); + + log = share ? vhost_log_shm[backend_type] : vhost_log[backend_type]; if (!log || log->size != size) { log = vhost_log_alloc(size, share); if (share) { - vhost_log_shm = log; + vhost_log_shm[backend_type] = log; } else { - vhost_log = log; + vhost_log[backend_type] = log; } } else { ++log->refcnt; @@ -340,11 +350,20 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share) static void vhost_log_put(struct vhost_dev *dev, bool sync) { struct vhost_log *log = dev->log; + VhostBackendType backend_type; if (!log) { return; } + assert(dev->vhost_ops); + backend_type = dev->vhost_ops->backend_type; + + if (backend_type == VHOST_BACKEND_TYPE_NONE || + backend_type >= VHOST_BACKEND_TYPE_MAX) { + return; + } + --log->refcnt; if (log->refcnt == 0) { /* Sync only the range covered by the old log */ @@ -352,13 +371,13 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync) vhost_log_sync_range(dev, 0, dev->log_size * VHOST_LOG_CHUNK - 1); } - if (vhost_log == log) { + if (vhost_log[backend_type] == log) { g_free(log->log); - vhost_log = NULL; - } else if (vhost_log_shm == log) { + vhost_log[backend_type] = NULL; + } else if (vhost_log_shm[backend_type] == log) { qemu_memfd_free(log->log, log->size * sizeof(*(log->log)), log->fd); - vhost_log_shm = NULL; + vhost_log_shm[backend_type] = NULL; } g_free(log); @@ -376,7 +395,8 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev) static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size) { - struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); + struct vhost_log *log = vhost_log_get(dev->vhost_ops->backend_type, + size, vhost_dev_log_is_shared(dev)); uint64_t log_base = (uintptr_t)log->log; int r; @@ -2037,8 +2057,14 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings) uint64_t log_base; hdev->log_size = vhost_get_log_size(hdev); - hdev->log = vhost_log_get(hdev->log_size, + hdev->log = vhost_log_get(hdev->vhost_ops->backend_type, + hdev->log_size, vhost_dev_log_is_shared(hdev)); + if (!hdev->log) { + VHOST_OPS_DEBUG(r, "vhost_log_get failed"); + goto fail_vq; + } + log_base = (uintptr_t)hdev->log->log; r = hdev->vhost_ops->vhost_set_log_base(hdev, hdev->log_size ? log_base : 0, -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v3 2/2] vhost: Perform memory section dirty scans once per iteration 2024-03-14 7:26 [PATCH v3 1/2] vhost: dirty log should be per backend type Si-Wei Liu @ 2024-03-14 7:26 ` Si-Wei Liu 2024-03-14 15:34 ` Eugenio Perez Martin 2024-03-14 15:25 ` [PATCH v3 1/2] vhost: dirty log should be per backend type Eugenio Perez Martin 1 sibling, 1 reply; 7+ messages in thread From: Si-Wei Liu @ 2024-03-14 7:26 UTC (permalink / raw) To: qemu-devel; +Cc: mst, jasowang, eperezma, joao.m.martins, si-wei.liu On setups with one or more virtio-net devices with vhost on, dirty tracking iteration increases cost the bigger the number amount of queues are set up e.g. on idle guests migration the following is observed with virtio-net with vhost=on: 48 queues -> 78.11% [.] vhost_dev_sync_region.isra.13 8 queues -> 40.50% [.] vhost_dev_sync_region.isra.13 1 queue -> 6.89% [.] vhost_dev_sync_region.isra.13 2 devices, 1 queue -> 18.60% [.] vhost_dev_sync_region.isra.14 With high memory rates the symptom is lack of convergence as soon as it has a vhost device with a sufficiently high number of queues, the sufficient number of vhost devices. On every migration iteration (every 100msecs) it will redundantly query the *shared log* the number of queues configured with vhost that exist in the guest. For the virtqueue data, this is necessary, but not for the memory sections which are the same. So essentially we end up scanning the dirty log too often. To fix that, select a vhost device responsible for scanning the log with regards to memory sections dirty tracking. It is selected when we enable the logger (during migration) and cleared when we disable the logger. If the vhost logger device goes away for some reason, the logger will be re-selected from the rest of vhost devices. After making mem-section logger a singleton instance, constant cost of 7%-9% (like the 1 queue report) will be seen, no matter how many queues or how many vhost devices are configured: 48 queues -> 8.71% [.] vhost_dev_sync_region.isra.13 2 devices, 8 queues -> 7.97% [.] vhost_dev_sync_region.isra.14 Co-developed-by: Joao Martins <joao.m.martins@oracle.com> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> --- v2 -> v3: - add after-fix benchmark to commit log - rename vhost_log_dev_enabled to vhost_dev_should_log - remove unneeded comparisons for backend_type - use QLIST array instead of single flat list to store vhost logger devices - simplify logger election logic --- hw/virtio/vhost.c | 63 ++++++++++++++++++++++++++++++++++++++++++----- include/hw/virtio/vhost.h | 1 + 2 files changed, 58 insertions(+), 6 deletions(-) diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index efe2f74..d91858b 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -45,6 +45,7 @@ static struct vhost_log *vhost_log[VHOST_BACKEND_TYPE_MAX]; static struct vhost_log *vhost_log_shm[VHOST_BACKEND_TYPE_MAX]; +static QLIST_HEAD(, vhost_dev) vhost_log_devs[VHOST_BACKEND_TYPE_MAX]; /* Memslots used by backends that support private memslots (without an fd). */ static unsigned int used_memslots; @@ -149,6 +150,43 @@ bool vhost_dev_has_iommu(struct vhost_dev *dev) } } +static inline bool vhost_dev_should_log(struct vhost_dev *dev) +{ + assert(dev->vhost_ops); + assert(dev->vhost_ops->backend_type > VHOST_BACKEND_TYPE_NONE); + assert(dev->vhost_ops->backend_type < VHOST_BACKEND_TYPE_MAX); + + return dev == QLIST_FIRST(&vhost_log_devs[dev->vhost_ops->backend_type]); +} + +static inline void vhost_dev_elect_mem_logger(struct vhost_dev *hdev, bool add) +{ + VhostBackendType backend_type; + + assert(hdev->vhost_ops); + + backend_type = hdev->vhost_ops->backend_type; + assert(backend_type > VHOST_BACKEND_TYPE_NONE); + assert(backend_type < VHOST_BACKEND_TYPE_MAX); + + if (add && !QLIST_IS_INSERTED(hdev, logdev_entry)) { + if (QLIST_EMPTY(&vhost_log_devs[backend_type])) { + QLIST_INSERT_HEAD(&vhost_log_devs[backend_type], + hdev, logdev_entry); + } else { + /* + * The first vhost_device in the list is selected as the shared + * logger to scan memory sections. Put new entry next to the head + * to avoid inadvertent change to the underlying logger device. + */ + QLIST_INSERT_AFTER(QLIST_FIRST(&vhost_log_devs[backend_type]), + hdev, logdev_entry); + } + } else if (!add && QLIST_IS_INSERTED(hdev, logdev_entry)) { + QLIST_REMOVE(hdev, logdev_entry); + } +} + static int vhost_sync_dirty_bitmap(struct vhost_dev *dev, MemoryRegionSection *section, hwaddr first, @@ -166,12 +204,14 @@ static int vhost_sync_dirty_bitmap(struct vhost_dev *dev, start_addr = MAX(first, start_addr); end_addr = MIN(last, end_addr); - for (i = 0; i < dev->mem->nregions; ++i) { - struct vhost_memory_region *reg = dev->mem->regions + i; - vhost_dev_sync_region(dev, section, start_addr, end_addr, - reg->guest_phys_addr, - range_get_last(reg->guest_phys_addr, - reg->memory_size)); + if (vhost_dev_should_log(dev)) { + for (i = 0; i < dev->mem->nregions; ++i) { + struct vhost_memory_region *reg = dev->mem->regions + i; + vhost_dev_sync_region(dev, section, start_addr, end_addr, + reg->guest_phys_addr, + range_get_last(reg->guest_phys_addr, + reg->memory_size)); + } } for (i = 0; i < dev->nvqs; ++i) { struct vhost_virtqueue *vq = dev->vqs + i; @@ -383,6 +423,7 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync) g_free(log); } + vhost_dev_elect_mem_logger(dev, false); dev->log = NULL; dev->log_size = 0; } @@ -998,6 +1039,15 @@ static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log) goto err_vq; } } + + /* + * At log start we select our vhost_device logger that will scan the + * memory sections and skip for the others. This is possible because + * the log is shared amongst all vhost devices for a given type of + * backend. + */ + vhost_dev_elect_mem_logger(dev, enable_log); + return 0; err_vq: for (; i >= 0; --i) { @@ -2073,6 +2123,7 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings) VHOST_OPS_DEBUG(r, "vhost_set_log_base failed"); goto fail_log; } + vhost_dev_elect_mem_logger(hdev, true); } if (vrings) { r = vhost_dev_set_vring_enable(hdev, true); diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h index 0247778..d75faf4 100644 --- a/include/hw/virtio/vhost.h +++ b/include/hw/virtio/vhost.h @@ -129,6 +129,7 @@ struct vhost_dev { void *opaque; struct vhost_log *log; QLIST_ENTRY(vhost_dev) entry; + QLIST_ENTRY(vhost_dev) logdev_entry; QLIST_HEAD(, vhost_iommu) iommu_list; IOMMUNotifier n; const VhostDevConfigOps *config_ops; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] vhost: Perform memory section dirty scans once per iteration 2024-03-14 7:26 ` [PATCH v3 2/2] vhost: Perform memory section dirty scans once per iteration Si-Wei Liu @ 2024-03-14 15:34 ` Eugenio Perez Martin 2024-03-14 18:34 ` Si-Wei Liu 0 siblings, 1 reply; 7+ messages in thread From: Eugenio Perez Martin @ 2024-03-14 15:34 UTC (permalink / raw) To: Si-Wei Liu; +Cc: qemu-devel, mst, jasowang, joao.m.martins On Thu, Mar 14, 2024 at 9:38 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote: > > On setups with one or more virtio-net devices with vhost on, > dirty tracking iteration increases cost the bigger the number > amount of queues are set up e.g. on idle guests migration the > following is observed with virtio-net with vhost=on: > > 48 queues -> 78.11% [.] vhost_dev_sync_region.isra.13 > 8 queues -> 40.50% [.] vhost_dev_sync_region.isra.13 > 1 queue -> 6.89% [.] vhost_dev_sync_region.isra.13 > 2 devices, 1 queue -> 18.60% [.] vhost_dev_sync_region.isra.14 > > With high memory rates the symptom is lack of convergence as soon > as it has a vhost device with a sufficiently high number of queues, > the sufficient number of vhost devices. > > On every migration iteration (every 100msecs) it will redundantly > query the *shared log* the number of queues configured with vhost > that exist in the guest. For the virtqueue data, this is necessary, > but not for the memory sections which are the same. So essentially > we end up scanning the dirty log too often. > > To fix that, select a vhost device responsible for scanning the > log with regards to memory sections dirty tracking. It is selected > when we enable the logger (during migration) and cleared when we > disable the logger. If the vhost logger device goes away for some > reason, the logger will be re-selected from the rest of vhost > devices. > > After making mem-section logger a singleton instance, constant cost > of 7%-9% (like the 1 queue report) will be seen, no matter how many > queues or how many vhost devices are configured: > > 48 queues -> 8.71% [.] vhost_dev_sync_region.isra.13 > 2 devices, 8 queues -> 7.97% [.] vhost_dev_sync_region.isra.14 > > Co-developed-by: Joao Martins <joao.m.martins@oracle.com> > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> > --- > v2 -> v3: > - add after-fix benchmark to commit log > - rename vhost_log_dev_enabled to vhost_dev_should_log > - remove unneeded comparisons for backend_type > - use QLIST array instead of single flat list to store vhost > logger devices > - simplify logger election logic > > --- > hw/virtio/vhost.c | 63 ++++++++++++++++++++++++++++++++++++++++++----- > include/hw/virtio/vhost.h | 1 + > 2 files changed, 58 insertions(+), 6 deletions(-) > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index efe2f74..d91858b 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -45,6 +45,7 @@ > > static struct vhost_log *vhost_log[VHOST_BACKEND_TYPE_MAX]; > static struct vhost_log *vhost_log_shm[VHOST_BACKEND_TYPE_MAX]; > +static QLIST_HEAD(, vhost_dev) vhost_log_devs[VHOST_BACKEND_TYPE_MAX]; > > /* Memslots used by backends that support private memslots (without an fd). */ > static unsigned int used_memslots; > @@ -149,6 +150,43 @@ bool vhost_dev_has_iommu(struct vhost_dev *dev) > } > } > > +static inline bool vhost_dev_should_log(struct vhost_dev *dev) > +{ > + assert(dev->vhost_ops); > + assert(dev->vhost_ops->backend_type > VHOST_BACKEND_TYPE_NONE); > + assert(dev->vhost_ops->backend_type < VHOST_BACKEND_TYPE_MAX); > + > + return dev == QLIST_FIRST(&vhost_log_devs[dev->vhost_ops->backend_type]); > +} > + > +static inline void vhost_dev_elect_mem_logger(struct vhost_dev *hdev, bool add) > +{ > + VhostBackendType backend_type; > + > + assert(hdev->vhost_ops); > + > + backend_type = hdev->vhost_ops->backend_type; > + assert(backend_type > VHOST_BACKEND_TYPE_NONE); > + assert(backend_type < VHOST_BACKEND_TYPE_MAX); > + > + if (add && !QLIST_IS_INSERTED(hdev, logdev_entry)) { > + if (QLIST_EMPTY(&vhost_log_devs[backend_type])) { > + QLIST_INSERT_HEAD(&vhost_log_devs[backend_type], > + hdev, logdev_entry); > + } else { > + /* > + * The first vhost_device in the list is selected as the shared > + * logger to scan memory sections. Put new entry next to the head > + * to avoid inadvertent change to the underlying logger device. > + */ Why is changing the logger device a problem? All the code paths are either changing the QLIST or logging, isn't it? > + QLIST_INSERT_AFTER(QLIST_FIRST(&vhost_log_devs[backend_type]), > + hdev, logdev_entry); > + } > + } else if (!add && QLIST_IS_INSERTED(hdev, logdev_entry)) { > + QLIST_REMOVE(hdev, logdev_entry); > + } > +} > + > static int vhost_sync_dirty_bitmap(struct vhost_dev *dev, > MemoryRegionSection *section, > hwaddr first, > @@ -166,12 +204,14 @@ static int vhost_sync_dirty_bitmap(struct vhost_dev *dev, > start_addr = MAX(first, start_addr); > end_addr = MIN(last, end_addr); > > - for (i = 0; i < dev->mem->nregions; ++i) { > - struct vhost_memory_region *reg = dev->mem->regions + i; > - vhost_dev_sync_region(dev, section, start_addr, end_addr, > - reg->guest_phys_addr, > - range_get_last(reg->guest_phys_addr, > - reg->memory_size)); > + if (vhost_dev_should_log(dev)) { > + for (i = 0; i < dev->mem->nregions; ++i) { > + struct vhost_memory_region *reg = dev->mem->regions + i; > + vhost_dev_sync_region(dev, section, start_addr, end_addr, > + reg->guest_phys_addr, > + range_get_last(reg->guest_phys_addr, > + reg->memory_size)); > + } > } > for (i = 0; i < dev->nvqs; ++i) { > struct vhost_virtqueue *vq = dev->vqs + i; > @@ -383,6 +423,7 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync) > g_free(log); > } > > + vhost_dev_elect_mem_logger(dev, false); > dev->log = NULL; > dev->log_size = 0; > } > @@ -998,6 +1039,15 @@ static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log) > goto err_vq; > } > } > + > + /* > + * At log start we select our vhost_device logger that will scan the > + * memory sections and skip for the others. This is possible because > + * the log is shared amongst all vhost devices for a given type of > + * backend. > + */ > + vhost_dev_elect_mem_logger(dev, enable_log); > + > return 0; > err_vq: > for (; i >= 0; --i) { > @@ -2073,6 +2123,7 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings) > VHOST_OPS_DEBUG(r, "vhost_set_log_base failed"); > goto fail_log; > } > + vhost_dev_elect_mem_logger(hdev, true); > } > if (vrings) { > r = vhost_dev_set_vring_enable(hdev, true); > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h > index 0247778..d75faf4 100644 > --- a/include/hw/virtio/vhost.h > +++ b/include/hw/virtio/vhost.h > @@ -129,6 +129,7 @@ struct vhost_dev { > void *opaque; > struct vhost_log *log; > QLIST_ENTRY(vhost_dev) entry; > + QLIST_ENTRY(vhost_dev) logdev_entry; > QLIST_HEAD(, vhost_iommu) iommu_list; > IOMMUNotifier n; > const VhostDevConfigOps *config_ops; > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] vhost: Perform memory section dirty scans once per iteration 2024-03-14 15:34 ` Eugenio Perez Martin @ 2024-03-14 18:34 ` Si-Wei Liu 2024-03-14 19:07 ` Eugenio Perez Martin 0 siblings, 1 reply; 7+ messages in thread From: Si-Wei Liu @ 2024-03-14 18:34 UTC (permalink / raw) To: Eugenio Perez Martin; +Cc: qemu-devel, mst, jasowang, joao.m.martins On 3/14/2024 8:34 AM, Eugenio Perez Martin wrote: > On Thu, Mar 14, 2024 at 9:38 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote: >> On setups with one or more virtio-net devices with vhost on, >> dirty tracking iteration increases cost the bigger the number >> amount of queues are set up e.g. on idle guests migration the >> following is observed with virtio-net with vhost=on: >> >> 48 queues -> 78.11% [.] vhost_dev_sync_region.isra.13 >> 8 queues -> 40.50% [.] vhost_dev_sync_region.isra.13 >> 1 queue -> 6.89% [.] vhost_dev_sync_region.isra.13 >> 2 devices, 1 queue -> 18.60% [.] vhost_dev_sync_region.isra.14 >> >> With high memory rates the symptom is lack of convergence as soon >> as it has a vhost device with a sufficiently high number of queues, >> the sufficient number of vhost devices. >> >> On every migration iteration (every 100msecs) it will redundantly >> query the *shared log* the number of queues configured with vhost >> that exist in the guest. For the virtqueue data, this is necessary, >> but not for the memory sections which are the same. So essentially >> we end up scanning the dirty log too often. >> >> To fix that, select a vhost device responsible for scanning the >> log with regards to memory sections dirty tracking. It is selected >> when we enable the logger (during migration) and cleared when we >> disable the logger. If the vhost logger device goes away for some >> reason, the logger will be re-selected from the rest of vhost >> devices. >> >> After making mem-section logger a singleton instance, constant cost >> of 7%-9% (like the 1 queue report) will be seen, no matter how many >> queues or how many vhost devices are configured: >> >> 48 queues -> 8.71% [.] vhost_dev_sync_region.isra.13 >> 2 devices, 8 queues -> 7.97% [.] vhost_dev_sync_region.isra.14 >> >> Co-developed-by: Joao Martins <joao.m.martins@oracle.com> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> >> --- >> v2 -> v3: >> - add after-fix benchmark to commit log >> - rename vhost_log_dev_enabled to vhost_dev_should_log >> - remove unneeded comparisons for backend_type >> - use QLIST array instead of single flat list to store vhost >> logger devices >> - simplify logger election logic >> >> --- >> hw/virtio/vhost.c | 63 ++++++++++++++++++++++++++++++++++++++++++----- >> include/hw/virtio/vhost.h | 1 + >> 2 files changed, 58 insertions(+), 6 deletions(-) >> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >> index efe2f74..d91858b 100644 >> --- a/hw/virtio/vhost.c >> +++ b/hw/virtio/vhost.c >> @@ -45,6 +45,7 @@ >> >> static struct vhost_log *vhost_log[VHOST_BACKEND_TYPE_MAX]; >> static struct vhost_log *vhost_log_shm[VHOST_BACKEND_TYPE_MAX]; >> +static QLIST_HEAD(, vhost_dev) vhost_log_devs[VHOST_BACKEND_TYPE_MAX]; >> >> /* Memslots used by backends that support private memslots (without an fd). */ >> static unsigned int used_memslots; >> @@ -149,6 +150,43 @@ bool vhost_dev_has_iommu(struct vhost_dev *dev) >> } >> } >> >> +static inline bool vhost_dev_should_log(struct vhost_dev *dev) >> +{ >> + assert(dev->vhost_ops); >> + assert(dev->vhost_ops->backend_type > VHOST_BACKEND_TYPE_NONE); >> + assert(dev->vhost_ops->backend_type < VHOST_BACKEND_TYPE_MAX); >> + >> + return dev == QLIST_FIRST(&vhost_log_devs[dev->vhost_ops->backend_type]); >> +} >> + >> +static inline void vhost_dev_elect_mem_logger(struct vhost_dev *hdev, bool add) >> +{ >> + VhostBackendType backend_type; >> + >> + assert(hdev->vhost_ops); >> + >> + backend_type = hdev->vhost_ops->backend_type; >> + assert(backend_type > VHOST_BACKEND_TYPE_NONE); >> + assert(backend_type < VHOST_BACKEND_TYPE_MAX); >> + >> + if (add && !QLIST_IS_INSERTED(hdev, logdev_entry)) { >> + if (QLIST_EMPTY(&vhost_log_devs[backend_type])) { >> + QLIST_INSERT_HEAD(&vhost_log_devs[backend_type], >> + hdev, logdev_entry); >> + } else { >> + /* >> + * The first vhost_device in the list is selected as the shared >> + * logger to scan memory sections. Put new entry next to the head >> + * to avoid inadvertent change to the underlying logger device. >> + */ > Why is changing the logger device a problem? All the code paths are > either changing the QLIST or logging, isn't it? Changing logger device doesn't affect functionality for sure, but may have inadvertent effect on cache locality, particularly it's relevant to the log scanning process in the hot path. The code makes sure there's no churn on the leading logger selection as a result of adding new vhost device, unless the selected logger device will be gone and a re-election of another logger is needed. -Siwei > >> + QLIST_INSERT_AFTER(QLIST_FIRST(&vhost_log_devs[backend_type]), >> + hdev, logdev_entry); >> + } >> + } else if (!add && QLIST_IS_INSERTED(hdev, logdev_entry)) { >> + QLIST_REMOVE(hdev, logdev_entry); >> + } >> +} >> + >> static int vhost_sync_dirty_bitmap(struct vhost_dev *dev, >> MemoryRegionSection *section, >> hwaddr first, >> @@ -166,12 +204,14 @@ static int vhost_sync_dirty_bitmap(struct vhost_dev *dev, >> start_addr = MAX(first, start_addr); >> end_addr = MIN(last, end_addr); >> >> - for (i = 0; i < dev->mem->nregions; ++i) { >> - struct vhost_memory_region *reg = dev->mem->regions + i; >> - vhost_dev_sync_region(dev, section, start_addr, end_addr, >> - reg->guest_phys_addr, >> - range_get_last(reg->guest_phys_addr, >> - reg->memory_size)); >> + if (vhost_dev_should_log(dev)) { >> + for (i = 0; i < dev->mem->nregions; ++i) { >> + struct vhost_memory_region *reg = dev->mem->regions + i; >> + vhost_dev_sync_region(dev, section, start_addr, end_addr, >> + reg->guest_phys_addr, >> + range_get_last(reg->guest_phys_addr, >> + reg->memory_size)); >> + } >> } >> for (i = 0; i < dev->nvqs; ++i) { >> struct vhost_virtqueue *vq = dev->vqs + i; >> @@ -383,6 +423,7 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync) >> g_free(log); >> } >> >> + vhost_dev_elect_mem_logger(dev, false); >> dev->log = NULL; >> dev->log_size = 0; >> } >> @@ -998,6 +1039,15 @@ static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log) >> goto err_vq; >> } >> } >> + >> + /* >> + * At log start we select our vhost_device logger that will scan the >> + * memory sections and skip for the others. This is possible because >> + * the log is shared amongst all vhost devices for a given type of >> + * backend. >> + */ >> + vhost_dev_elect_mem_logger(dev, enable_log); >> + >> return 0; >> err_vq: >> for (; i >= 0; --i) { >> @@ -2073,6 +2123,7 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings) >> VHOST_OPS_DEBUG(r, "vhost_set_log_base failed"); >> goto fail_log; >> } >> + vhost_dev_elect_mem_logger(hdev, true); >> } >> if (vrings) { >> r = vhost_dev_set_vring_enable(hdev, true); >> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h >> index 0247778..d75faf4 100644 >> --- a/include/hw/virtio/vhost.h >> +++ b/include/hw/virtio/vhost.h >> @@ -129,6 +129,7 @@ struct vhost_dev { >> void *opaque; >> struct vhost_log *log; >> QLIST_ENTRY(vhost_dev) entry; >> + QLIST_ENTRY(vhost_dev) logdev_entry; >> QLIST_HEAD(, vhost_iommu) iommu_list; >> IOMMUNotifier n; >> const VhostDevConfigOps *config_ops; >> -- >> 1.8.3.1 >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] vhost: Perform memory section dirty scans once per iteration 2024-03-14 18:34 ` Si-Wei Liu @ 2024-03-14 19:07 ` Eugenio Perez Martin 0 siblings, 0 replies; 7+ messages in thread From: Eugenio Perez Martin @ 2024-03-14 19:07 UTC (permalink / raw) To: Si-Wei Liu; +Cc: qemu-devel, mst, jasowang, joao.m.martins On Thu, Mar 14, 2024 at 7:35 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote: > > > > On 3/14/2024 8:34 AM, Eugenio Perez Martin wrote: > > On Thu, Mar 14, 2024 at 9:38 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote: > >> On setups with one or more virtio-net devices with vhost on, > >> dirty tracking iteration increases cost the bigger the number > >> amount of queues are set up e.g. on idle guests migration the > >> following is observed with virtio-net with vhost=on: > >> > >> 48 queues -> 78.11% [.] vhost_dev_sync_region.isra.13 > >> 8 queues -> 40.50% [.] vhost_dev_sync_region.isra.13 > >> 1 queue -> 6.89% [.] vhost_dev_sync_region.isra.13 > >> 2 devices, 1 queue -> 18.60% [.] vhost_dev_sync_region.isra.14 > >> > >> With high memory rates the symptom is lack of convergence as soon > >> as it has a vhost device with a sufficiently high number of queues, > >> the sufficient number of vhost devices. > >> > >> On every migration iteration (every 100msecs) it will redundantly > >> query the *shared log* the number of queues configured with vhost > >> that exist in the guest. For the virtqueue data, this is necessary, > >> but not for the memory sections which are the same. So essentially > >> we end up scanning the dirty log too often. > >> > >> To fix that, select a vhost device responsible for scanning the > >> log with regards to memory sections dirty tracking. It is selected > >> when we enable the logger (during migration) and cleared when we > >> disable the logger. If the vhost logger device goes away for some > >> reason, the logger will be re-selected from the rest of vhost > >> devices. > >> > >> After making mem-section logger a singleton instance, constant cost > >> of 7%-9% (like the 1 queue report) will be seen, no matter how many > >> queues or how many vhost devices are configured: > >> > >> 48 queues -> 8.71% [.] vhost_dev_sync_region.isra.13 > >> 2 devices, 8 queues -> 7.97% [.] vhost_dev_sync_region.isra.14 > >> > >> Co-developed-by: Joao Martins <joao.m.martins@oracle.com> > >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > >> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> > >> --- > >> v2 -> v3: > >> - add after-fix benchmark to commit log > >> - rename vhost_log_dev_enabled to vhost_dev_should_log > >> - remove unneeded comparisons for backend_type > >> - use QLIST array instead of single flat list to store vhost > >> logger devices > >> - simplify logger election logic > >> > >> --- > >> hw/virtio/vhost.c | 63 ++++++++++++++++++++++++++++++++++++++++++----- > >> include/hw/virtio/vhost.h | 1 + > >> 2 files changed, 58 insertions(+), 6 deletions(-) > >> > >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > >> index efe2f74..d91858b 100644 > >> --- a/hw/virtio/vhost.c > >> +++ b/hw/virtio/vhost.c > >> @@ -45,6 +45,7 @@ > >> > >> static struct vhost_log *vhost_log[VHOST_BACKEND_TYPE_MAX]; > >> static struct vhost_log *vhost_log_shm[VHOST_BACKEND_TYPE_MAX]; > >> +static QLIST_HEAD(, vhost_dev) vhost_log_devs[VHOST_BACKEND_TYPE_MAX]; > >> > >> /* Memslots used by backends that support private memslots (without an fd). */ > >> static unsigned int used_memslots; > >> @@ -149,6 +150,43 @@ bool vhost_dev_has_iommu(struct vhost_dev *dev) > >> } > >> } > >> > >> +static inline bool vhost_dev_should_log(struct vhost_dev *dev) > >> +{ > >> + assert(dev->vhost_ops); > >> + assert(dev->vhost_ops->backend_type > VHOST_BACKEND_TYPE_NONE); > >> + assert(dev->vhost_ops->backend_type < VHOST_BACKEND_TYPE_MAX); > >> + > >> + return dev == QLIST_FIRST(&vhost_log_devs[dev->vhost_ops->backend_type]); > >> +} > >> + > >> +static inline void vhost_dev_elect_mem_logger(struct vhost_dev *hdev, bool add) > >> +{ > >> + VhostBackendType backend_type; > >> + > >> + assert(hdev->vhost_ops); > >> + > >> + backend_type = hdev->vhost_ops->backend_type; > >> + assert(backend_type > VHOST_BACKEND_TYPE_NONE); > >> + assert(backend_type < VHOST_BACKEND_TYPE_MAX); > >> + > >> + if (add && !QLIST_IS_INSERTED(hdev, logdev_entry)) { > >> + if (QLIST_EMPTY(&vhost_log_devs[backend_type])) { > >> + QLIST_INSERT_HEAD(&vhost_log_devs[backend_type], > >> + hdev, logdev_entry); > >> + } else { > >> + /* > >> + * The first vhost_device in the list is selected as the shared > >> + * logger to scan memory sections. Put new entry next to the head > >> + * to avoid inadvertent change to the underlying logger device. > >> + */ > > Why is changing the logger device a problem? All the code paths are > > either changing the QLIST or logging, isn't it? > Changing logger device doesn't affect functionality for sure, but may > have inadvertent effect on cache locality, particularly it's relevant to > the log scanning process in the hot path. The code makes sure there's no > churn on the leading logger selection as a result of adding new vhost > device, unless the selected logger device will be gone and a re-election > of another logger is needed. > Understood, thanks for the explanation! If you're going to send a new version I suggest adding that to the comment. Acked-by: Eugenio Pérez <eperezma@redhat.com> Thanks! > -Siwei > > > > >> + QLIST_INSERT_AFTER(QLIST_FIRST(&vhost_log_devs[backend_type]), > >> + hdev, logdev_entry); > >> + } > >> + } else if (!add && QLIST_IS_INSERTED(hdev, logdev_entry)) { > >> + QLIST_REMOVE(hdev, logdev_entry); > >> + } > >> +} > >> + > >> static int vhost_sync_dirty_bitmap(struct vhost_dev *dev, > >> MemoryRegionSection *section, > >> hwaddr first, > >> @@ -166,12 +204,14 @@ static int vhost_sync_dirty_bitmap(struct vhost_dev *dev, > >> start_addr = MAX(first, start_addr); > >> end_addr = MIN(last, end_addr); > >> > >> - for (i = 0; i < dev->mem->nregions; ++i) { > >> - struct vhost_memory_region *reg = dev->mem->regions + i; > >> - vhost_dev_sync_region(dev, section, start_addr, end_addr, > >> - reg->guest_phys_addr, > >> - range_get_last(reg->guest_phys_addr, > >> - reg->memory_size)); > >> + if (vhost_dev_should_log(dev)) { > >> + for (i = 0; i < dev->mem->nregions; ++i) { > >> + struct vhost_memory_region *reg = dev->mem->regions + i; > >> + vhost_dev_sync_region(dev, section, start_addr, end_addr, > >> + reg->guest_phys_addr, > >> + range_get_last(reg->guest_phys_addr, > >> + reg->memory_size)); > >> + } > >> } > >> for (i = 0; i < dev->nvqs; ++i) { > >> struct vhost_virtqueue *vq = dev->vqs + i; > >> @@ -383,6 +423,7 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync) > >> g_free(log); > >> } > >> > >> + vhost_dev_elect_mem_logger(dev, false); > >> dev->log = NULL; > >> dev->log_size = 0; > >> } > >> @@ -998,6 +1039,15 @@ static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log) > >> goto err_vq; > >> } > >> } > >> + > >> + /* > >> + * At log start we select our vhost_device logger that will scan the > >> + * memory sections and skip for the others. This is possible because > >> + * the log is shared amongst all vhost devices for a given type of > >> + * backend. > >> + */ > >> + vhost_dev_elect_mem_logger(dev, enable_log); > >> + > >> return 0; > >> err_vq: > >> for (; i >= 0; --i) { > >> @@ -2073,6 +2123,7 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings) > >> VHOST_OPS_DEBUG(r, "vhost_set_log_base failed"); > >> goto fail_log; > >> } > >> + vhost_dev_elect_mem_logger(hdev, true); > >> } > >> if (vrings) { > >> r = vhost_dev_set_vring_enable(hdev, true); > >> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h > >> index 0247778..d75faf4 100644 > >> --- a/include/hw/virtio/vhost.h > >> +++ b/include/hw/virtio/vhost.h > >> @@ -129,6 +129,7 @@ struct vhost_dev { > >> void *opaque; > >> struct vhost_log *log; > >> QLIST_ENTRY(vhost_dev) entry; > >> + QLIST_ENTRY(vhost_dev) logdev_entry; > >> QLIST_HEAD(, vhost_iommu) iommu_list; > >> IOMMUNotifier n; > >> const VhostDevConfigOps *config_ops; > >> -- > >> 1.8.3.1 > >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] vhost: dirty log should be per backend type 2024-03-14 7:26 [PATCH v3 1/2] vhost: dirty log should be per backend type Si-Wei Liu 2024-03-14 7:26 ` [PATCH v3 2/2] vhost: Perform memory section dirty scans once per iteration Si-Wei Liu @ 2024-03-14 15:25 ` Eugenio Perez Martin 2024-03-14 18:35 ` Si-Wei Liu 1 sibling, 1 reply; 7+ messages in thread From: Eugenio Perez Martin @ 2024-03-14 15:25 UTC (permalink / raw) To: Si-Wei Liu; +Cc: qemu-devel, mst, jasowang, joao.m.martins On Thu, Mar 14, 2024 at 9:38 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote: > > There could be a mix of both vhost-user and vhost-kernel clients > in the same QEMU process, where separate vhost loggers for the > specific vhost type have to be used. Make the vhost logger per > backend type, and have them properly reference counted. > > Suggested-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> > --- > v2->v3: > - remove non-effective assertion that never be reached > - do not return NULL from vhost_log_get() > - add neccessary assertions to vhost_log_get() > > --- > hw/virtio/vhost.c | 50 ++++++++++++++++++++++++++++++++++++++------------ > 1 file changed, 38 insertions(+), 12 deletions(-) > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 2c9ac79..efe2f74 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -43,8 +43,8 @@ > do { } while (0) > #endif > > -static struct vhost_log *vhost_log; > -static struct vhost_log *vhost_log_shm; > +static struct vhost_log *vhost_log[VHOST_BACKEND_TYPE_MAX]; > +static struct vhost_log *vhost_log_shm[VHOST_BACKEND_TYPE_MAX]; > > /* Memslots used by backends that support private memslots (without an fd). */ > static unsigned int used_memslots; > @@ -287,6 +287,10 @@ static int vhost_set_backend_type(struct vhost_dev *dev, > r = -1; > } > > + if (r == 0) { > + assert(dev->vhost_ops->backend_type == backend_type); > + } > + > return r; > } > > @@ -319,16 +323,22 @@ static struct vhost_log *vhost_log_alloc(uint64_t size, bool share) > return log; > } > > -static struct vhost_log *vhost_log_get(uint64_t size, bool share) > +static struct vhost_log *vhost_log_get(VhostBackendType backend_type, > + uint64_t size, bool share) > { > - struct vhost_log *log = share ? vhost_log_shm : vhost_log; > + struct vhost_log *log; > + > + assert(backend_type > VHOST_BACKEND_TYPE_NONE); > + assert(backend_type < VHOST_BACKEND_TYPE_MAX); > + > + log = share ? vhost_log_shm[backend_type] : vhost_log[backend_type]; > > if (!log || log->size != size) { > log = vhost_log_alloc(size, share); > if (share) { > - vhost_log_shm = log; > + vhost_log_shm[backend_type] = log; > } else { > - vhost_log = log; > + vhost_log[backend_type] = log; > } > } else { > ++log->refcnt; > @@ -340,11 +350,20 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share) > static void vhost_log_put(struct vhost_dev *dev, bool sync) > { > struct vhost_log *log = dev->log; > + VhostBackendType backend_type; > > if (!log) { > return; > } > > + assert(dev->vhost_ops); > + backend_type = dev->vhost_ops->backend_type; > + > + if (backend_type == VHOST_BACKEND_TYPE_NONE || > + backend_type >= VHOST_BACKEND_TYPE_MAX) { > + return; > + } > + > --log->refcnt; > if (log->refcnt == 0) { > /* Sync only the range covered by the old log */ > @@ -352,13 +371,13 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync) > vhost_log_sync_range(dev, 0, dev->log_size * VHOST_LOG_CHUNK - 1); > } > > - if (vhost_log == log) { > + if (vhost_log[backend_type] == log) { > g_free(log->log); > - vhost_log = NULL; > - } else if (vhost_log_shm == log) { > + vhost_log[backend_type] = NULL; > + } else if (vhost_log_shm[backend_type] == log) { > qemu_memfd_free(log->log, log->size * sizeof(*(log->log)), > log->fd); > - vhost_log_shm = NULL; > + vhost_log_shm[backend_type] = NULL; > } > > g_free(log); > @@ -376,7 +395,8 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev) > > static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size) > { > - struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); > + struct vhost_log *log = vhost_log_get(dev->vhost_ops->backend_type, > + size, vhost_dev_log_is_shared(dev)); > uint64_t log_base = (uintptr_t)log->log; > int r; > > @@ -2037,8 +2057,14 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings) > uint64_t log_base; > > hdev->log_size = vhost_get_log_size(hdev); > - hdev->log = vhost_log_get(hdev->log_size, > + hdev->log = vhost_log_get(hdev->vhost_ops->backend_type, > + hdev->log_size, > vhost_dev_log_is_shared(hdev)); > + if (!hdev->log) { I thought vhost_log_get couldn't return NULL :). Other than that, Acked-by: Eugenio Pérez <eperezma@redhat.com> > + VHOST_OPS_DEBUG(r, "vhost_log_get failed"); > + goto fail_vq; > + } > + > log_base = (uintptr_t)hdev->log->log; > r = hdev->vhost_ops->vhost_set_log_base(hdev, > hdev->log_size ? log_base : 0, > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] vhost: dirty log should be per backend type 2024-03-14 15:25 ` [PATCH v3 1/2] vhost: dirty log should be per backend type Eugenio Perez Martin @ 2024-03-14 18:35 ` Si-Wei Liu 0 siblings, 0 replies; 7+ messages in thread From: Si-Wei Liu @ 2024-03-14 18:35 UTC (permalink / raw) To: Eugenio Perez Martin; +Cc: qemu-devel, mst, jasowang, joao.m.martins On 3/14/2024 8:25 AM, Eugenio Perez Martin wrote: > On Thu, Mar 14, 2024 at 9:38 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote: >> There could be a mix of both vhost-user and vhost-kernel clients >> in the same QEMU process, where separate vhost loggers for the >> specific vhost type have to be used. Make the vhost logger per >> backend type, and have them properly reference counted. >> >> Suggested-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com> >> --- >> v2->v3: >> - remove non-effective assertion that never be reached >> - do not return NULL from vhost_log_get() >> - add neccessary assertions to vhost_log_get() >> >> --- >> hw/virtio/vhost.c | 50 ++++++++++++++++++++++++++++++++++++++------------ >> 1 file changed, 38 insertions(+), 12 deletions(-) >> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >> index 2c9ac79..efe2f74 100644 >> --- a/hw/virtio/vhost.c >> +++ b/hw/virtio/vhost.c >> @@ -43,8 +43,8 @@ >> do { } while (0) >> #endif >> >> -static struct vhost_log *vhost_log; >> -static struct vhost_log *vhost_log_shm; >> +static struct vhost_log *vhost_log[VHOST_BACKEND_TYPE_MAX]; >> +static struct vhost_log *vhost_log_shm[VHOST_BACKEND_TYPE_MAX]; >> >> /* Memslots used by backends that support private memslots (without an fd). */ >> static unsigned int used_memslots; >> @@ -287,6 +287,10 @@ static int vhost_set_backend_type(struct vhost_dev *dev, >> r = -1; >> } >> >> + if (r == 0) { >> + assert(dev->vhost_ops->backend_type == backend_type); >> + } >> + >> return r; >> } >> >> @@ -319,16 +323,22 @@ static struct vhost_log *vhost_log_alloc(uint64_t size, bool share) >> return log; >> } >> >> -static struct vhost_log *vhost_log_get(uint64_t size, bool share) >> +static struct vhost_log *vhost_log_get(VhostBackendType backend_type, >> + uint64_t size, bool share) >> { >> - struct vhost_log *log = share ? vhost_log_shm : vhost_log; >> + struct vhost_log *log; >> + >> + assert(backend_type > VHOST_BACKEND_TYPE_NONE); >> + assert(backend_type < VHOST_BACKEND_TYPE_MAX); >> + >> + log = share ? vhost_log_shm[backend_type] : vhost_log[backend_type]; >> >> if (!log || log->size != size) { >> log = vhost_log_alloc(size, share); >> if (share) { >> - vhost_log_shm = log; >> + vhost_log_shm[backend_type] = log; >> } else { >> - vhost_log = log; >> + vhost_log[backend_type] = log; >> } >> } else { >> ++log->refcnt; >> @@ -340,11 +350,20 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share) >> static void vhost_log_put(struct vhost_dev *dev, bool sync) >> { >> struct vhost_log *log = dev->log; >> + VhostBackendType backend_type; >> >> if (!log) { >> return; >> } >> >> + assert(dev->vhost_ops); >> + backend_type = dev->vhost_ops->backend_type; >> + >> + if (backend_type == VHOST_BACKEND_TYPE_NONE || >> + backend_type >= VHOST_BACKEND_TYPE_MAX) { >> + return; >> + } >> + >> --log->refcnt; >> if (log->refcnt == 0) { >> /* Sync only the range covered by the old log */ >> @@ -352,13 +371,13 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync) >> vhost_log_sync_range(dev, 0, dev->log_size * VHOST_LOG_CHUNK - 1); >> } >> >> - if (vhost_log == log) { >> + if (vhost_log[backend_type] == log) { >> g_free(log->log); >> - vhost_log = NULL; >> - } else if (vhost_log_shm == log) { >> + vhost_log[backend_type] = NULL; >> + } else if (vhost_log_shm[backend_type] == log) { >> qemu_memfd_free(log->log, log->size * sizeof(*(log->log)), >> log->fd); >> - vhost_log_shm = NULL; >> + vhost_log_shm[backend_type] = NULL; >> } >> >> g_free(log); >> @@ -376,7 +395,8 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev) >> >> static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size) >> { >> - struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev)); >> + struct vhost_log *log = vhost_log_get(dev->vhost_ops->backend_type, >> + size, vhost_dev_log_is_shared(dev)); >> uint64_t log_base = (uintptr_t)log->log; >> int r; >> >> @@ -2037,8 +2057,14 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings) >> uint64_t log_base; >> >> hdev->log_size = vhost_get_log_size(hdev); >> - hdev->log = vhost_log_get(hdev->log_size, >> + hdev->log = vhost_log_get(hdev->vhost_ops->backend_type, >> + hdev->log_size, >> vhost_dev_log_is_shared(hdev)); >> + if (!hdev->log) { > I thought vhost_log_get couldn't return NULL :). Sure, missed that. Will post a revised v4. -Siwei > > Other than that, > > Acked-by: Eugenio Pérez <eperezma@redhat.com> > >> + VHOST_OPS_DEBUG(r, "vhost_log_get failed"); >> + goto fail_vq; >> + } >> + >> log_base = (uintptr_t)hdev->log->log; >> r = hdev->vhost_ops->vhost_set_log_base(hdev, >> hdev->log_size ? log_base : 0, >> -- >> 1.8.3.1 >> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-03-14 19:08 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-03-14 7:26 [PATCH v3 1/2] vhost: dirty log should be per backend type Si-Wei Liu 2024-03-14 7:26 ` [PATCH v3 2/2] vhost: Perform memory section dirty scans once per iteration Si-Wei Liu 2024-03-14 15:34 ` Eugenio Perez Martin 2024-03-14 18:34 ` Si-Wei Liu 2024-03-14 19:07 ` Eugenio Perez Martin 2024-03-14 15:25 ` [PATCH v3 1/2] vhost: dirty log should be per backend type Eugenio Perez Martin 2024-03-14 18:35 ` Si-Wei Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).