Re: [PATCH v1 2/2] vhost: Remove vhost_backend_can_merge() callback

[Igor Mammedov]

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;