Re: [PATCH v5 40/50] multi-process/mig: build migration module in the remote process

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

* Jag Raman (jag.raman@oracle.com) wrote:
> 
> 
> On 3/4/2020 2:52 PM, Dr. David Alan Gilbert wrote:
> > * Jag Raman (jag.raman@oracle.com) wrote:
> > > 
> > > 
> > > On 3/4/2020 10:58 AM, Dr. David Alan Gilbert wrote:
> > > > * Jagannathan Raman (jag.raman@oracle.com) wrote:
> > > > > Add Makefile support to enable migration in remote process
> > > > > 
> > > > > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> > > > > Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> > > > > Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> > > > > ---
> > > > >    Makefile.objs           |  4 +++-
> > > > >    Makefile.target         |  1 +
> > > > >    migration/Makefile.objs | 13 ++++++++++++-
> > > > >    net/Makefile.objs       |  2 ++
> > > > >    softmmu/vl.c            |  2 --
> > > > >    stubs/migration.c       | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >    stubs/net-stub.c        | 21 +++++++++++++++++++++
> > > > >    stubs/qapi-misc.c       |  2 ++
> > > > >    stubs/replay.c          |  8 ++++++++
> > > > >    stubs/vl-stub.c         | 24 ++++++++++++++++++++++++
> > > > >    vl-parse.c              |  3 +++
> > > > >    11 files changed, 125 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/Makefile.objs b/Makefile.objs
> > > > > index 4b5db09..65009da 100644
> > > > > --- a/Makefile.objs
> > > > > +++ b/Makefile.objs
> > > > > @@ -74,6 +74,8 @@ common-obj-y += qdev-monitor.o device-hotplug.o
> > > > >    common-obj-$(CONFIG_WIN32) += os-win32.o
> > > > >    common-obj-$(CONFIG_POSIX) += os-posix.o
> > > > > +remote-pci-obj-$(CONFIG_POSIX) += os-posix.o
> > > > > +
> > > > >    common-obj-$(CONFIG_LINUX) += fsdev/
> > > > >    common-obj-y += accel/
> > > > > @@ -104,11 +106,11 @@ common-obj-y += vl-parse.o
> > > > >    #######################################################################
> > > > >    # qapi
> > > > > -
> > > > >    common-obj-y += qapi/
> > > > >    endif # CONFIG_SOFTMMU
> > > > > +remote-pci-obj-$(CONFIG_MPQEMU) += net/
> > > > >    remote-pci-obj-$(CONFIG_MPQEMU) += qapi/
> > > > >    remote-pci-obj-$(CONFIG_MPQEMU) += blockdev-nbd.o
> > > > >    remote-pci-obj-$(CONFIG_MPQEMU) += job-qmp.o
> > > > > diff --git a/Makefile.target b/Makefile.target
> > > > > index 4ead5c3..4012ae5 100644
> > > > > --- a/Makefile.target
> > > > > +++ b/Makefile.target
> > > > > @@ -240,6 +240,7 @@ all-remote-pci-obj-y += exec.o
> > > > >    all-remote-pci-obj-y += exec-vary.o
> > > > >    all-remote-pci-obj-y += ioport.o
> > > > >    all-remote-pci-obj-y += cpus.o
> > > > > +all-remote-pci-obj-y += migration/ram.o
> > > > >    endif
> > > > >    remote-pci-obj-y :=
> > > > > diff --git a/migration/Makefile.objs b/migration/Makefile.objs
> > > > > index e7cdc76..21f9d8d 100644
> > > > > --- a/migration/Makefile.objs
> > > > > +++ b/migration/Makefile.objs
> > > > > @@ -15,4 +15,15 @@ common-obj-$(CONFIG_LIVE_BLOCK_MIGRATION) += block.o
> > > > >    rdma.o-libs := $(RDMA_LIBS)
> > > > > -remote-pci-obj-$(CONFIG_MPQEMU) += qemu-file.o vmstate.o qjson.o vmstate-types.o
> > > > > +remote-pci-obj-$(CONFIG_MPQEMU) += migration.o socket.o fd.o exec.o
> > > > > +remote-pci-obj-$(CONFIG_MPQEMU) += tls.o channel.o savevm.o
> > > > > +remote-pci-obj-$(CONFIG_MPQEMU) += colo.o colo-failover.o
> > > > > +remote-pci-obj-$(CONFIG_MPQEMU) += vmstate.o vmstate-types.o page_cache.o
> > > > > +remote-pci-obj-$(CONFIG_MPQEMU) += qemu-file.o global_state.o
> > > > > +remote-pci-obj-$(CONFIG_MPQEMU) += qemu-file-channel.o
> > > > > +remote-pci-obj-$(CONFIG_MPQEMU) += xbzrle.o postcopy-ram.o
> > > > > +remote-pci-obj-$(CONFIG_MPQEMU) += qjson.o
> > > > > +remote-pci-obj-$(CONFIG_MPQEMU) += block-dirty-bitmap.o
> > > > > +remote-pci-obj-$(CONFIG_RDMA) += rdma.o
> > > > > +remote-pci-obj-$(CONFIG_MPQEMU) += block.o
> > > > > +remote-pci-obj-$(CONFIG_MPQEMU) += multifd.o
> > > > 
> > > > Hmm, are you really going to want all this lot in your remote process?
> > > > Assuming it's just devices, I can understand the first line or two, but
> > > > it seems odd to have all of this.
> > > 
> > > Yeah, we ended up needing to compile these in to enable migration. We
> > > are only using "fd" to enable migration. Although we don't use tls,
> > > xbzrle, rdma, multifd, etc... for example, the migration code does
> > > support these protocols and, therefore, we had to compile them in.
> > 
> > But are you even running a migration stream from the remote process?
> > Aren't you just doing vmstate migration of devices; i.e. do you need
> > anything relating to incremental RAM migration (e.g. xbzrle, rdma,
> > postcopy).
> 
> We are running a migration stream from the remote process. We are only
> doing the vmstate migration of devices, and not anything incremental
> related to RAM.
> 
> We are using QEMU's existing migration infrastructure (vmstate_save /
> qemu_loadvm_section_start_full) to move the vmstate. Based on my limited
> experience with the migration code, I get that it comes as a suite.
> Without some refactoring, we would need to build all of the files within
> the migration folder.
> 
> The vmstate functions communicate over QEMUFile, which would be using
> fd, tcp, rdma, etc... at the backend. These functions also need other
> functions defined in migration.c, which require the building of xbzrle,
> postcopy, etc...

OK, I bet we can refactor some of that to be modular fairly easily; some
is harder than others.  For example there are different QEMUFile
subclasses for RDMA, so you should be able to have just the top level
QEMUFile and the implementations you need.
Similarly the vmstate code shouldn't depend on the ram code;  we can
look at splitting it up some how.

Dave

> Thank you!
> --
> Jag
> 
> > 
> > Dave
> > 
> > > Thank you!
> > > --
> > > Jag
> > > 
> > > > 
> > > > Dave
> > > > 
> > > > > diff --git a/net/Makefile.objs b/net/Makefile.objs
> > > > > index c5d076d..a8ad986 100644
> > > > > --- a/net/Makefile.objs
> > > > > +++ b/net/Makefile.objs
> > > > > @@ -30,3 +30,5 @@ common-obj-$(CONFIG_WIN32) += tap-win32.o
> > > > >    vde.o-libs = $(VDE_LIBS)
> > > > >    common-obj-$(CONFIG_CAN_BUS) += can/
> > > > > +
> > > > > +remote-pci-obj-$(CONFIG_MPQEMU) += announce.o
> > > > > diff --git a/softmmu/vl.c b/softmmu/vl.c
> > > > > index 4a4f52c..42d5682 100644
> > > > > --- a/softmmu/vl.c
> > > > > +++ b/softmmu/vl.c
> > > > > @@ -128,7 +128,6 @@ const char* keyboard_layout = NULL;
> > > > >    ram_addr_t ram_size;
> > > > >    const char *mem_path = NULL;
> > > > >    int mem_prealloc = 0; /* force preallocation of physical target memory */
> > > > > -bool enable_mlock = false;
> > > > >    bool enable_cpu_pm = false;
> > > > >    int nb_nics;
> > > > >    NICInfo nd_table[MAX_NICS];
> > > > > @@ -168,7 +167,6 @@ const char *prom_envs[MAX_PROM_ENVS];
> > > > >    int boot_menu;
> > > > >    bool boot_strict;
> > > > >    uint8_t *boot_splash_filedata;
> > > > > -int only_migratable; /* turn it off unless user states otherwise */
> > > > >    bool wakeup_suspend_enabled;
> > > > >    int icount_align_option;
> > > > > diff --git a/stubs/migration.c b/stubs/migration.c
> > > > > index 28ccf80..dbd12db 100644
> > > > > --- a/stubs/migration.c
> > > > > +++ b/stubs/migration.c
> > > > > @@ -6,6 +6,35 @@
> > > > >    #include "qapi/qapi-types-migration.h"
> > > > >    #include "qapi/qapi-commands-migration.h"
> > > > >    #include "qapi/qapi-types-net.h"
> > > > > +#include "net/filter.h"
> > > > > +#include "net/colo-compare.h"
> > > > > +
> > > > > +#pragma weak qmp_query_migrate_capabilities
> > > > > +#pragma weak qmp_query_migrate_parameters
> > > > > +#pragma weak migrate_announce_params
> > > > > +#pragma weak qmp_query_migrate
> > > > > +#pragma weak qmp_migrate_set_capabilities
> > > > > +#pragma weak qmp_migrate_set_parameters
> > > > > +#pragma weak qmp_migrate_incoming
> > > > > +#pragma weak qmp_migrate_recover
> > > > > +#pragma weak qmp_migrate_pause
> > > > > +#pragma weak qmp_migrate
> > > > > +#pragma weak qmp_migrate_cancel
> > > > > +#pragma weak qmp_migrate_continue
> > > > > +#pragma weak qmp_migrate_set_cache_size
> > > > > +#pragma weak qmp_query_migrate_cache_size
> > > > > +#pragma weak qmp_migrate_set_speed
> > > > > +#pragma weak qmp_migrate_set_downtime
> > > > > +#pragma weak qmp_migrate_start_postcopy
> > > > > +#pragma weak migration_global_dump
> > > > > +#pragma weak save_snapshot
> > > > > +#pragma weak qmp_xen_save_devices_state
> > > > > +#pragma weak load_snapshot
> > > > > +#pragma weak qmp_xen_set_replication
> > > > > +#pragma weak qmp_query_xen_replication_status
> > > > > +#pragma weak qmp_xen_colo_do_checkpoint
> > > > > +#pragma weak qmp_query_colo_status
> > > > > +#pragma weak qmp_x_colo_lost_heartbeat
> > > > >    MigrationInfo *qmp_query_migrate(Error **errp)
> > > > >    {
> > > > > @@ -160,3 +189,23 @@ AnnounceParameters *migrate_announce_params(void)
> > > > >        return NULL;
> > > > >    }
> > > > > +
> > > > > +void colo_notify_filters_event(int event, Error **errp)
> > > > > +{
> > > > > +    qemu_debug_assert(0);
> > > > > +}
> > > > > +
> > > > > +void colo_notify_compares_event(void *opaque, int event, Error **errp)
> > > > > +{
> > > > > +    qemu_debug_assert(0);
> > > > > +}
> > > > > +
> > > > > +void colo_compare_register_notifier(Notifier *notify)
> > > > > +{
> > > > > +    qemu_debug_assert(0);
> > > > > +}
> > > > > +
> > > > > +void colo_compare_unregister_notifier(Notifier *notify)
> > > > > +{
> > > > > +    qemu_debug_assert(0);
> > > > > +}
> > > > > diff --git a/stubs/net-stub.c b/stubs/net-stub.c
> > > > > index 962827e..ddfd1e4 100644
> > > > > --- a/stubs/net-stub.c
> > > > > +++ b/stubs/net-stub.c
> > > > > @@ -5,6 +5,8 @@
> > > > >    #include "qapi/qapi-commands-net.h"
> > > > >    #include "qapi/qapi-commands-rocker.h"
> > > > > +#pragma weak qmp_announce_self
> > > > > +
> > > > >    int qemu_find_net_clients_except(const char *id, NetClientState **ncs,
> > > > >                                     NetClientDriver type, int max)
> > > > >    {
> > > > > @@ -98,3 +100,22 @@ void netdev_add(QemuOpts *opts, Error **errp)
> > > > >    {
> > > > >        qemu_debug_assert(0);
> > > > >    }
> > > > > +
> > > > > +NetClientState *qemu_get_queue(NICState *nic)
> > > > > +{
> > > > > +    qemu_debug_assert(0);
> > > > > +
> > > > > +    return NULL;
> > > > > +}
> > > > > +
> > > > > +ssize_t qemu_send_packet_raw(NetClientState *nc, const uint8_t *buf, int size)
> > > > > +{
> > > > > +    qemu_debug_assert(0);
> > > > > +
> > > > > +    return 0;
> > > > > +}
> > > > > +
> > > > > +void qemu_foreach_nic(qemu_nic_foreach func, void *opaque)
> > > > > +{
> > > > > +    qemu_debug_assert(0);
> > > > > +}
> > > > > diff --git a/stubs/qapi-misc.c b/stubs/qapi-misc.c
> > > > > index 3eeedd9..824eac1 100644
> > > > > --- a/stubs/qapi-misc.c
> > > > > +++ b/stubs/qapi-misc.c
> > > > > @@ -5,6 +5,8 @@
> > > > >    #include "./qapi/qapi-types-dump.h"
> > > > >    #include "qapi/qapi-commands-dump.h"
> > > > > +#pragma weak qmp_xen_load_devices_state
> > > > > +
> > > > >    void qmp_dump_guest_memory(bool paging, const char *file,
> > > > >                               bool has_detach, bool detach,
> > > > >                               bool has_begin, int64_t begin, bool has_length,
> > > > > diff --git a/stubs/replay.c b/stubs/replay.c
> > > > > index 9b53c0c..6fc7850 100644
> > > > > --- a/stubs/replay.c
> > > > > +++ b/stubs/replay.c
> > > > > @@ -1,4 +1,5 @@
> > > > >    #include "qemu/osdep.h"
> > > > > +#include "qemu-common.h"
> > > > >    #include "sysemu/replay.h"
> > > > >    ReplayMode replay_mode;
> > > > > @@ -106,3 +107,10 @@ void replay_account_executed_instructions(void)
> > > > >    void replay_add_blocker(Error *reason)
> > > > >    {
> > > > >    }
> > > > > +
> > > > > +bool replay_can_snapshot(void)
> > > > > +{
> > > > > +    qemu_debug_assert(0);
> > > > > +
> > > > > +    return false;
> > > > > +}
> > > > > diff --git a/stubs/vl-stub.c b/stubs/vl-stub.c
> > > > > index 606f078..5f308c1 100644
> > > > > --- a/stubs/vl-stub.c
> > > > > +++ b/stubs/vl-stub.c
> > > > > @@ -14,6 +14,8 @@
> > > > >    #include "disas/disas.h"
> > > > >    #include "audio/audio.h"
> > > > > +#pragma weak qemu_add_exit_notifier
> > > > > +
> > > > >    bool tcg_allowed;
> > > > >    bool xen_allowed;
> > > > >    bool boot_strict;
> > > > > @@ -169,3 +171,25 @@ int wav_start_capture(AudioState *state, CaptureState *s, const char *path,
> > > > >        return -1;
> > > > >    }
> > > > > +
> > > > > +void qemu_system_killed(int signal, pid_t pid)
> > > > > +{
> > > > > +    qemu_debug_assert(0);
> > > > > +}
> > > > > +
> > > > > +void qemu_system_reset(ShutdownCause reason)
> > > > > +{
> > > > > +    qemu_debug_assert(0);
> > > > > +}
> > > > > +
> > > > > +bool runstate_store(char *str, size_t size)
> > > > > +{
> > > > > +    qemu_debug_assert(0);
> > > > > +
> > > > > +    return false;
> > > > > +}
> > > > > +
> > > > > +void qemu_add_exit_notifier(Notifier *notify)
> > > > > +{
> > > > > +    qemu_debug_assert(0);
> > > > > +}
> > > > > diff --git a/vl-parse.c b/vl-parse.c
> > > > > index 1f6a3f0..423f4a0 100644
> > > > > --- a/vl-parse.c
> > > > > +++ b/vl-parse.c
> > > > > @@ -27,6 +27,9 @@
> > > > >    #include "vl.h"
> > > > > +int only_migratable; /* turn it off unless user states otherwise */
> > > > > +bool enable_mlock;
> > > > > +
> > > > >    /***********************************************************/
> > > > >    /* QEMU Block devices */
> > > > > -- 
> > > > > 1.8.3.1
> > > > > 
> > > > --
> > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > > > 
> > > 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK