Re: [PATCH RESEND v6 22/36] multi-process: Synchronize remote memory

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> On Wed, Apr 22, 2020 at 09:13:57PM -0700, elena.ufimtseva@oracle.com wrote:
> > diff --git a/hw/proxy/memory-sync.c b/hw/proxy/memory-sync.c
> > new file mode 100644
> > index 0000000000..b3f57747f3
> > --- /dev/null
> > +++ b/hw/proxy/memory-sync.c
> > @@ -0,0 +1,217 @@
> > +/*
> > + * Copyright © 2018, 2020 Oracle and/or its affiliates.
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#include <sys/types.h>
> > +#include <stdio.h>
> > +#include <string.h>
> 
> These headers should already be included by "qemu/osdep.h".
> 
> > +static void proxy_ml_region_addnop(MemoryListener *listener,
> > +                                   MemoryRegionSection *section)
> > +{
> > +    RemoteMemSync *sync = container_of(listener, RemoteMemSync, listener);
> > +    bool need_add = true;
> > +    uint64_t mrs_size, mrs_gpa, mrs_page;
> > +    uintptr_t mrs_host;
> > +    RAMBlock *mrs_rb;
> > +    MemoryRegionSection *prev_sec;
> > +
> > +    if (!(memory_region_is_ram(section->mr) &&
> > +          !memory_region_is_rom(section->mr))) {
> > +        return;
> > +    }
> > +
> > +    mrs_rb = section->mr->ram_block;
> > +    mrs_page = (uint64_t)qemu_ram_pagesize(mrs_rb);
> > +    mrs_size = int128_get64(section->size);
> > +    mrs_gpa = section->offset_within_address_space;
> > +    mrs_host = (uintptr_t)memory_region_get_ram_ptr(section->mr) +
> > +               section->offset_within_region;
> 
> These variables are only used in the if (sync->n_mr_sections) case. This
> function could be split into a something like this:
> 
>   static void proxy_ml_region_addnop(MemoryListener *listener,
>                                      MemoryRegionSection *section)
>       RemoteMemSync *sync = container_of(listener, RemoteMemSync, listener);
> 
>       if (!(memory_region_is_ram(section->mr) &&
>             !memory_region_is_rom(section->mr))) {
>           return;
>       }
> 
>       if (try_merge(sync, section)) {
>           return;
>       }
> 
>       ...add new section...
>   }
> 
> And the try_merge() helper function has the rest of the code:
> 
>   /* Returns true if the section was merged */
>   static bool try_merge(RemoteMemSync *sync, MemoryRegionSection *section)
>   {
>       if (sync->n_mr_sections == 0) {
>           return false;
>       }
> 
>       ...most of the code...
>   }
> 
> > +
> > +    if (get_fd_from_hostaddr(mrs_host, NULL) <= 0) {
> 
> 0 is a valid fd number, the comparison should probably be < 0?
> 
> > +        return;
> > +    }
> > +
> > +    mrs_host = mrs_host & ~(mrs_page - 1);
> > +    mrs_gpa = mrs_gpa & ~(mrs_page - 1);
> > +    mrs_size = ROUND_UP(mrs_size, mrs_page);
> 
> Why is it necessary to align to the RAM block's page size?
> 
> Can mrs_host and mrs_size be misaligned to the RAM block's page size?
> 
> Why round the *guest* physical address down using the *host* page size?

That sounds like the type of magic we do for postcopy; where we can only
'place' pages atomically on a host page boundary.

Dave

> > +
> > +    if (sync->n_mr_sections) {
> > +        prev_sec = sync->mr_sections + (sync->n_mr_sections - 1);
> > +        uint64_t prev_gpa_start = prev_sec->offset_within_address_space;
> > +        uint64_t prev_size = int128_get64(prev_sec->size);
> > +        uint64_t prev_gpa_end   = range_get_last(prev_gpa_start, prev_size);
> > +        uint64_t prev_host_start =
> > +            (uintptr_t)memory_region_get_ram_ptr(prev_sec->mr) +
> > +            prev_sec->offset_within_region;
> > +        uint64_t prev_host_end = range_get_last(prev_host_start, prev_size);
> 
> Is it okay not to do the page alignment stuff for the previous
> MemoryRegionSection?
> 
> > +void deconfigure_memory_sync(RemoteMemSync *sync)
> > +{
> > +    memory_listener_unregister(&sync->listener);
> > +}
> 
> This function is unused? It must be tied into the mpqemu_link lifecycle.
> It must be possible to hot plug/unplug proxy PCI devices without memory
> leaks or use-after-frees.
> 
> > diff --git a/include/hw/proxy/memory-sync.h b/include/hw/proxy/memory-sync.h
> > new file mode 100644
> > index 0000000000..d8329c9b52
> > --- /dev/null
> > +++ b/include/hw/proxy/memory-sync.h
> > @@ -0,0 +1,37 @@
> > +/*
> > + * Copyright © 2018, 2020 Oracle and/or its affiliates.
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#ifndef MEMORY_SYNC_H
> > +#define MEMORY_SYNC_H
> > +
> > +#include <sys/types.h>
> > +
> > +#include "qemu/osdep.h"
> > +#include "qom/object.h"
> > +#include "exec/memory.h"
> > +#include "io/mpqemu-link.h"
> > +
> > +#define TYPE_MEMORY_LISTENER "memory-listener"
> 
> This name is too generic. There is already a C struct called
> MemoryListener. Please call this class "remote-memory-sync".
> 
> I'm not sure if a QOM object is needed here. Can this just be a plain C
> struct? If you're not using QOM object-orientated features then there is
> no need to define a QOM object.
> 
> > @@ -39,8 +40,13 @@ typedef struct ProxyMemoryRegion {
> >  struct PCIProxyDev {
> >      PCIDevice parent_dev;
> >  
> > +    int n_mr_sections;
> > +    MemoryRegionSection *mr_sections;
> 
> Is it necessary to duplicate these fields here since a RemoteMemSync
> field is also being added and it contains these same fields?


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK