| Message ID | 20230216114752.198627-3-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | vhost: memslot handling improvements | expand |
On Thu, 16 Feb 2023 12:47:52 +0100 David Hildenbrand <david@redhat.com> wrote: > Checking whether the memory regions are equal is sufficient: if they are > equal, then most certainly the contained fd is equal. sounds reasonable to me. > > The whole vhost-user memslot handling is suboptimal and overly > complicated. We shouldn't have to lookup a RAM memory regions we got > notified about in vhost_user_get_mr_data() using a host pointer. But that While on janitor duty can you fixup following? vhost_user_get_mr_data() -> memory_region_from_host -> -> qemu_ram_block_from_host() for qemu_ram_block_from_host doc comment seems to out of sync (ram_addr not longer exists) > requires a bigger rework -- especially an alternative vhost_set_mem_table() > backend call that simply consumes MemoryRegionSections. just skimming through usage of vhost_user_get_mr_data() it looks like we are first collecting MemoryRegionSection-s into tmp_sections then we do vhost_commit we convert then into vhost_memory_region list and the we are trying hard to convert addresses from the later to back to MemoryRegions we've lost during tmp_sections conversion all over the place. To me it looks like we should drop conversion to vhost_dev::mem and replace its usage with vhost_dev::mem_sections directly to get rid of data duplication and back and forth addr<->mr conversion. > For now, let's just drop vhost_backend_can_merge(). > > Signed-off-by: David Hildenbrand <david@redhat.com> Reviewed-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/virtio/vhost-user.c | 14 -------------- > hw/virtio/vhost-vdpa.c | 1 - > hw/virtio/vhost.c | 6 +----- > include/hw/virtio/vhost-backend.h | 4 ---- > 4 files changed, 1 insertion(+), 24 deletions(-) > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index e68daa35d4..4bfaf559a7 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -2195,19 +2195,6 @@ static int vhost_user_migration_done(struct vhost_dev *dev, char* mac_addr) > return -ENOTSUP; > } > > -static bool vhost_user_can_merge(struct vhost_dev *dev, > - uint64_t start1, uint64_t size1, > - uint64_t start2, uint64_t size2) > -{ > - ram_addr_t offset; > - int mfd, rfd; > - > - (void)vhost_user_get_mr_data(start1, &offset, &mfd); > - (void)vhost_user_get_mr_data(start2, &offset, &rfd); > - > - return mfd == rfd; > -} > - > static int vhost_user_net_set_mtu(struct vhost_dev *dev, uint16_t mtu) > { > VhostUserMsg msg; > @@ -2704,7 +2691,6 @@ const VhostOps user_ops = { > .vhost_set_vring_enable = vhost_user_set_vring_enable, > .vhost_requires_shm_log = vhost_user_requires_shm_log, > .vhost_migration_done = vhost_user_migration_done, > - .vhost_backend_can_merge = vhost_user_can_merge, > .vhost_net_set_mtu = vhost_user_net_set_mtu, > .vhost_set_iotlb_callback = vhost_user_set_iotlb_callback, > .vhost_send_device_iotlb_msg = vhost_user_send_device_iotlb_msg, > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > index 542e003101..9ab7bc8718 100644 > --- a/hw/virtio/vhost-vdpa.c > +++ b/hw/virtio/vhost-vdpa.c > @@ -1317,7 +1317,6 @@ const VhostOps vdpa_ops = { > .vhost_set_config = vhost_vdpa_set_config, > .vhost_requires_shm_log = NULL, > .vhost_migration_done = NULL, > - .vhost_backend_can_merge = NULL, > .vhost_net_set_mtu = NULL, > .vhost_set_iotlb_callback = NULL, > .vhost_send_device_iotlb_msg = NULL, > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index b7fb960fa9..9d8662aa98 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -733,11 +733,7 @@ static void vhost_region_add_section(struct vhost_dev *dev, > size_t offset = mrs_gpa - prev_gpa_start; > > if (prev_host_start + offset == mrs_host && > - section->mr == prev_sec->mr && > - (!dev->vhost_ops->vhost_backend_can_merge || > - dev->vhost_ops->vhost_backend_can_merge(dev, > - mrs_host, mrs_size, > - prev_host_start, prev_size))) { > + section->mr == prev_sec->mr) { > uint64_t max_end = MAX(prev_host_end, mrs_host + mrs_size); > need_add = false; > prev_sec->offset_within_address_space = > diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h > index c5ab49051e..abf1601ba2 100644 > --- a/include/hw/virtio/vhost-backend.h > +++ b/include/hw/virtio/vhost-backend.h > @@ -86,9 +86,6 @@ typedef int (*vhost_set_vring_enable_op)(struct vhost_dev *dev, > typedef bool (*vhost_requires_shm_log_op)(struct vhost_dev *dev); > typedef int (*vhost_migration_done_op)(struct vhost_dev *dev, > char *mac_addr); > -typedef bool (*vhost_backend_can_merge_op)(struct vhost_dev *dev, > - uint64_t start1, uint64_t size1, > - uint64_t start2, uint64_t size2); > typedef int (*vhost_vsock_set_guest_cid_op)(struct vhost_dev *dev, > uint64_t guest_cid); > typedef int (*vhost_vsock_set_running_op)(struct vhost_dev *dev, int start); > @@ -160,7 +157,6 @@ typedef struct VhostOps { > vhost_set_vring_enable_op vhost_set_vring_enable; > vhost_requires_shm_log_op vhost_requires_shm_log; > vhost_migration_done_op vhost_migration_done; > - vhost_backend_can_merge_op vhost_backend_can_merge; > vhost_vsock_set_guest_cid_op vhost_vsock_set_guest_cid; > vhost_vsock_set_running_op vhost_vsock_set_running; > vhost_set_iotlb_callback_op vhost_set_iotlb_callback;
On Tue, 7 Mar 2023 11:25:48 +0100 Igor Mammedov <imammedo@redhat.com> wrote: > On Thu, 16 Feb 2023 12:47:52 +0100 > David Hildenbrand <david@redhat.com> wrote: > > > Checking whether the memory regions are equal is sufficient: if they are > > equal, then most certainly the contained fd is equal. > sounds reasonable to me. > > > > > The whole vhost-user memslot handling is suboptimal and overly > > complicated. We shouldn't have to lookup a RAM memory regions we got > > notified about in vhost_user_get_mr_data() using a host pointer. But that > > While on janitor duty can you fixup following? > > vhost_user_get_mr_data() -> memory_region_from_host -> > -> qemu_ram_block_from_host() > for qemu_ram_block_from_host doc comment seems to out of > sync (ram_addr not longer exists) > > > requires a bigger rework -- especially an alternative vhost_set_mem_table() > > backend call that simply consumes MemoryRegionSections. > > just skimming through usage of vhost_user_get_mr_data() it looks like > we are first collecting MemoryRegionSection-s into tmp_sections > then we do vhost_commit we convert then into vhost_memory_region list > and the we are trying hard to convert addresses from the later > to back to MemoryRegions we've lost during tmp_sections conversion > all over the place. > > To me it looks like we should drop conversion to vhost_dev::mem > and replace its usage with vhost_dev::mem_sections directly > to get rid of data duplication and back and forth addr<->mr conversion. > > > For now, let's just drop vhost_backend_can_merge(). > > > > Signed-off-by: David Hildenbrand <david@redhat.com> > > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > hw/virtio/vhost-user.c | 14 -------------- > > hw/virtio/vhost-vdpa.c | 1 - > > hw/virtio/vhost.c | 6 +----- > > include/hw/virtio/vhost-backend.h | 4 ---- > > 4 files changed, 1 insertion(+), 24 deletions(-) > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > > index e68daa35d4..4bfaf559a7 100644 > > --- a/hw/virtio/vhost-user.c > > +++ b/hw/virtio/vhost-user.c > > @@ -2195,19 +2195,6 @@ static int vhost_user_migration_done(struct vhost_dev *dev, char* mac_addr) > > return -ENOTSUP; > > } > > > > -static bool vhost_user_can_merge(struct vhost_dev *dev, > > - uint64_t start1, uint64_t size1, > > - uint64_t start2, uint64_t size2) > > -{ > > - ram_addr_t offset; > > - int mfd, rfd; > > - > > - (void)vhost_user_get_mr_data(start1, &offset, &mfd); > > - (void)vhost_user_get_mr_data(start2, &offset, &rfd); > > - > > - return mfd == rfd; > > -} > > - > > static int vhost_user_net_set_mtu(struct vhost_dev *dev, uint16_t mtu) > > { > > VhostUserMsg msg; > > @@ -2704,7 +2691,6 @@ const VhostOps user_ops = { > > .vhost_set_vring_enable = vhost_user_set_vring_enable, > > .vhost_requires_shm_log = vhost_user_requires_shm_log, > > .vhost_migration_done = vhost_user_migration_done, > > - .vhost_backend_can_merge = vhost_user_can_merge, > > .vhost_net_set_mtu = vhost_user_net_set_mtu, > > .vhost_set_iotlb_callback = vhost_user_set_iotlb_callback, > > .vhost_send_device_iotlb_msg = vhost_user_send_device_iotlb_msg, > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > > index 542e003101..9ab7bc8718 100644 > > --- a/hw/virtio/vhost-vdpa.c > > +++ b/hw/virtio/vhost-vdpa.c > > @@ -1317,7 +1317,6 @@ const VhostOps vdpa_ops = { > > .vhost_set_config = vhost_vdpa_set_config, > > .vhost_requires_shm_log = NULL, > > .vhost_migration_done = NULL, > > - .vhost_backend_can_merge = NULL, > > .vhost_net_set_mtu = NULL, > > .vhost_set_iotlb_callback = NULL, > > .vhost_send_device_iotlb_msg = NULL, > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > index b7fb960fa9..9d8662aa98 100644 > > --- a/hw/virtio/vhost.c > > +++ b/hw/virtio/vhost.c > > @@ -733,11 +733,7 @@ static void vhost_region_add_section(struct vhost_dev *dev, > > size_t offset = mrs_gpa - prev_gpa_start; > > > > if (prev_host_start + offset == mrs_host && > > - section->mr == prev_sec->mr && > > - (!dev->vhost_ops->vhost_backend_can_merge || > > - dev->vhost_ops->vhost_backend_can_merge(dev, another question, can it relly happen, i.e. having 2 abut memory sections with the same memory region, is yes then when/why? > > - mrs_host, mrs_size, > > - prev_host_start, prev_size))) { > > + section->mr == prev_sec->mr) { > > uint64_t max_end = MAX(prev_host_end, mrs_host + mrs_size); > > need_add = false; > > prev_sec->offset_within_address_space = > > diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h > > index c5ab49051e..abf1601ba2 100644 > > --- a/include/hw/virtio/vhost-backend.h > > +++ b/include/hw/virtio/vhost-backend.h > > @@ -86,9 +86,6 @@ typedef int (*vhost_set_vring_enable_op)(struct vhost_dev *dev, > > typedef bool (*vhost_requires_shm_log_op)(struct vhost_dev *dev); > > typedef int (*vhost_migration_done_op)(struct vhost_dev *dev, > > char *mac_addr); > > -typedef bool (*vhost_backend_can_merge_op)(struct vhost_dev *dev, > > - uint64_t start1, uint64_t size1, > > - uint64_t start2, uint64_t size2); > > typedef int (*vhost_vsock_set_guest_cid_op)(struct vhost_dev *dev, > > uint64_t guest_cid); > > typedef int (*vhost_vsock_set_running_op)(struct vhost_dev *dev, int start); > > @@ -160,7 +157,6 @@ typedef struct VhostOps { > > vhost_set_vring_enable_op vhost_set_vring_enable; > > vhost_requires_shm_log_op vhost_requires_shm_log; > > vhost_migration_done_op vhost_migration_done; > > - vhost_backend_can_merge_op vhost_backend_can_merge; > > vhost_vsock_set_guest_cid_op vhost_vsock_set_guest_cid; > > vhost_vsock_set_running_op vhost_vsock_set_running; > > vhost_set_iotlb_callback_op vhost_set_iotlb_callback; >
On 07.03.23 12:16, Igor Mammedov wrote: > On Tue, 7 Mar 2023 11:25:48 +0100 > Igor Mammedov <imammedo@redhat.com> wrote: > >> On Thu, 16 Feb 2023 12:47:52 +0100 >> David Hildenbrand <david@redhat.com> wrote: >> >>> Checking whether the memory regions are equal is sufficient: if they are >>> equal, then most certainly the contained fd is equal. >> sounds reasonable to me. >> >>> >>> The whole vhost-user memslot handling is suboptimal and overly >>> complicated. We shouldn't have to lookup a RAM memory regions we got >>> notified about in vhost_user_get_mr_data() using a host pointer. But that >> >> While on janitor duty can you fixup following? >> >> vhost_user_get_mr_data() -> memory_region_from_host -> >> -> qemu_ram_block_from_host() >> for qemu_ram_block_from_host doc comment seems to out of >> sync (ram_addr not longer exists) >> >>> requires a bigger rework -- especially an alternative vhost_set_mem_table() >>> backend call that simply consumes MemoryRegionSections. >> >> just skimming through usage of vhost_user_get_mr_data() it looks like >> we are first collecting MemoryRegionSection-s into tmp_sections >> then we do vhost_commit we convert then into vhost_memory_region list >> and the we are trying hard to convert addresses from the later >> to back to MemoryRegions we've lost during tmp_sections conversion >> all over the place. >> >> To me it looks like we should drop conversion to vhost_dev::mem >> and replace its usage with vhost_dev::mem_sections directly >> to get rid of data duplication and back and forth addr<->mr conversion. >> >>> For now, let's just drop vhost_backend_can_merge(). >>> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >> >> Reviewed-by: Igor Mammedov <imammedo@redhat.com> >> >>> --- >>> hw/virtio/vhost-user.c | 14 -------------- >>> hw/virtio/vhost-vdpa.c | 1 - >>> hw/virtio/vhost.c | 6 +----- >>> include/hw/virtio/vhost-backend.h | 4 ---- >>> 4 files changed, 1 insertion(+), 24 deletions(-) >>> >>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c >>> index e68daa35d4..4bfaf559a7 100644 >>> --- a/hw/virtio/vhost-user.c >>> +++ b/hw/virtio/vhost-user.c >>> @@ -2195,19 +2195,6 @@ static int vhost_user_migration_done(struct vhost_dev *dev, char* mac_addr) >>> return -ENOTSUP; >>> } >>> >>> -static bool vhost_user_can_merge(struct vhost_dev *dev, >>> - uint64_t start1, uint64_t size1, >>> - uint64_t start2, uint64_t size2) >>> -{ >>> - ram_addr_t offset; >>> - int mfd, rfd; >>> - >>> - (void)vhost_user_get_mr_data(start1, &offset, &mfd); >>> - (void)vhost_user_get_mr_data(start2, &offset, &rfd); >>> - >>> - return mfd == rfd; >>> -} >>> - >>> static int vhost_user_net_set_mtu(struct vhost_dev *dev, uint16_t mtu) >>> { >>> VhostUserMsg msg; >>> @@ -2704,7 +2691,6 @@ const VhostOps user_ops = { >>> .vhost_set_vring_enable = vhost_user_set_vring_enable, >>> .vhost_requires_shm_log = vhost_user_requires_shm_log, >>> .vhost_migration_done = vhost_user_migration_done, >>> - .vhost_backend_can_merge = vhost_user_can_merge, >>> .vhost_net_set_mtu = vhost_user_net_set_mtu, >>> .vhost_set_iotlb_callback = vhost_user_set_iotlb_callback, >>> .vhost_send_device_iotlb_msg = vhost_user_send_device_iotlb_msg, >>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c >>> index 542e003101..9ab7bc8718 100644 >>> --- a/hw/virtio/vhost-vdpa.c >>> +++ b/hw/virtio/vhost-vdpa.c >>> @@ -1317,7 +1317,6 @@ const VhostOps vdpa_ops = { >>> .vhost_set_config = vhost_vdpa_set_config, >>> .vhost_requires_shm_log = NULL, >>> .vhost_migration_done = NULL, >>> - .vhost_backend_can_merge = NULL, >>> .vhost_net_set_mtu = NULL, >>> .vhost_set_iotlb_callback = NULL, >>> .vhost_send_device_iotlb_msg = NULL, >>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >>> index b7fb960fa9..9d8662aa98 100644 >>> --- a/hw/virtio/vhost.c >>> +++ b/hw/virtio/vhost.c >>> @@ -733,11 +733,7 @@ static void vhost_region_add_section(struct vhost_dev *dev, >>> size_t offset = mrs_gpa - prev_gpa_start; >>> >>> if (prev_host_start + offset == mrs_host && >>> - section->mr == prev_sec->mr && >>> - (!dev->vhost_ops->vhost_backend_can_merge || >>> - dev->vhost_ops->vhost_backend_can_merge(dev, > > another question, can it relly happen, i.e. having 2 abut memory sections > with the same memory region, is yes then when/why? Unfortunately yet, because vhost relies on some hacks (sorry, but that's what it is) to make huge pages work. The following commit contains some details: commit 76525114736e8f669766e69b715fa59ce8648aae Author: Dr. David Alan Gilbert <dgilbert@redhat.com> Date: Thu Jan 16 20:24:14 2020 +0000 vhost: Only align sections for vhost-user I added hugepage alignment code in c1ece84e7c9 to deal with vhost-user + postcopy which needs aligned pages when using userfault. However, on x86 the lower 2MB of address space tends to be shotgun'd with small fragments around the 512-640k range - e.g. video RAM, and with HyperV synic pages tend to sit around there - again splitting it up. The alignment code complains with a 'Section rounded to ...' error and gives up. Otherwise it wouldn't be needed, because flatview simplification code already merges what's reasonable. [I'll reply to you pother mail regarding that shortly]
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index e68daa35d4..4bfaf559a7 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -2195,19 +2195,6 @@ static int vhost_user_migration_done(struct vhost_dev *dev, char* mac_addr) return -ENOTSUP; } -static bool vhost_user_can_merge(struct vhost_dev *dev, - uint64_t start1, uint64_t size1, - uint64_t start2, uint64_t size2) -{ - ram_addr_t offset; - int mfd, rfd; - - (void)vhost_user_get_mr_data(start1, &offset, &mfd); - (void)vhost_user_get_mr_data(start2, &offset, &rfd); - - return mfd == rfd; -} - static int vhost_user_net_set_mtu(struct vhost_dev *dev, uint16_t mtu) { VhostUserMsg msg; @@ -2704,7 +2691,6 @@ const VhostOps user_ops = { .vhost_set_vring_enable = vhost_user_set_vring_enable, .vhost_requires_shm_log = vhost_user_requires_shm_log, .vhost_migration_done = vhost_user_migration_done, - .vhost_backend_can_merge = vhost_user_can_merge, .vhost_net_set_mtu = vhost_user_net_set_mtu, .vhost_set_iotlb_callback = vhost_user_set_iotlb_callback, .vhost_send_device_iotlb_msg = vhost_user_send_device_iotlb_msg, diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index 542e003101..9ab7bc8718 100644 --- a/hw/virtio/vhost-vdpa.c +++ b/hw/virtio/vhost-vdpa.c @@ -1317,7 +1317,6 @@ const VhostOps vdpa_ops = { .vhost_set_config = vhost_vdpa_set_config, .vhost_requires_shm_log = NULL, .vhost_migration_done = NULL, - .vhost_backend_can_merge = NULL, .vhost_net_set_mtu = NULL, .vhost_set_iotlb_callback = NULL, .vhost_send_device_iotlb_msg = NULL, diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index b7fb960fa9..9d8662aa98 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -733,11 +733,7 @@ static void vhost_region_add_section(struct vhost_dev *dev, size_t offset = mrs_gpa - prev_gpa_start; if (prev_host_start + offset == mrs_host && - section->mr == prev_sec->mr && - (!dev->vhost_ops->vhost_backend_can_merge || - dev->vhost_ops->vhost_backend_can_merge(dev, - mrs_host, mrs_size, - prev_host_start, prev_size))) { + section->mr == prev_sec->mr) { uint64_t max_end = MAX(prev_host_end, mrs_host + mrs_size); need_add = false; prev_sec->offset_within_address_space = diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h index c5ab49051e..abf1601ba2 100644 --- a/include/hw/virtio/vhost-backend.h +++ b/include/hw/virtio/vhost-backend.h @@ -86,9 +86,6 @@ typedef int (*vhost_set_vring_enable_op)(struct vhost_dev *dev, typedef bool (*vhost_requires_shm_log_op)(struct vhost_dev *dev); typedef int (*vhost_migration_done_op)(struct vhost_dev *dev, char *mac_addr); -typedef bool (*vhost_backend_can_merge_op)(struct vhost_dev *dev, - uint64_t start1, uint64_t size1, - uint64_t start2, uint64_t size2); typedef int (*vhost_vsock_set_guest_cid_op)(struct vhost_dev *dev, uint64_t guest_cid); typedef int (*vhost_vsock_set_running_op)(struct vhost_dev *dev, int start); @@ -160,7 +157,6 @@ typedef struct VhostOps { vhost_set_vring_enable_op vhost_set_vring_enable; vhost_requires_shm_log_op vhost_requires_shm_log; vhost_migration_done_op vhost_migration_done; - vhost_backend_can_merge_op vhost_backend_can_merge; vhost_vsock_set_guest_cid_op vhost_vsock_set_guest_cid; vhost_vsock_set_running_op vhost_vsock_set_running; vhost_set_iotlb_callback_op vhost_set_iotlb_callback;
Checking whether the memory regions are equal is sufficient: if they are equal, then most certainly the contained fd is equal. The whole vhost-user memslot handling is suboptimal and overly complicated. We shouldn't have to lookup a RAM memory regions we got notified about in vhost_user_get_mr_data() using a host pointer. But that requires a bigger rework -- especially an alternative vhost_set_mem_table() backend call that simply consumes MemoryRegionSections. For now, let's just drop vhost_backend_can_merge(). Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/virtio/vhost-user.c | 14 -------------- hw/virtio/vhost-vdpa.c | 1 - hw/virtio/vhost.c | 6 +----- include/hw/virtio/vhost-backend.h | 4 ---- 4 files changed, 1 insertion(+), 24 deletions(-)