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

[David Hildenbrand]

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]

--
Thanks,

David / dhildenb