Re: [Qemu-devel] [PATCH 4/6] migration: calculate downtime on dst side (CPUMASK)

[Dr. David Alan Gilbert]

* Alexey (address@hidden) wrote:
> Hello David,
> this mail just for CPUMASK discussion.
> 
> On Fri, Apr 21, 2017 at 01:00:32PM +0100, Dr. David Alan Gilbert wrote:
> > * Alexey Perevalov (address@hidden) wrote:
> > > This patch provides downtime calculation per vCPU,
> > > as a summary and as a overlapped value for all vCPUs.
> > > 
> > > This approach just keeps tree with page fault addr as a key,
> > > and t1-t2 interval of pagefault time and page copy time, with
> > > affected vCPU bit mask.
> > > For more implementation details please see comment to
> > > get_postcopy_total_downtime function.
> > > 
> > > Signed-off-by: Alexey Perevalov <address@hidden>
> > > ---
> > >  include/migration/migration.h |  14 +++
> > >  migration/migration.c         | 280 
> > > +++++++++++++++++++++++++++++++++++++++++-
> > >  migration/postcopy-ram.c      |  24 +++-
> > >  migration/qemu-file.c         |   1 -
> > >  migration/trace-events        |   9 +-
> > >  5 files changed, 323 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/include/migration/migration.h b/include/migration/migration.h
> > > index 5720c88..5d2c628 100644
> > > --- a/include/migration/migration.h
> > > +++ b/include/migration/migration.h
> > > @@ -123,10 +123,24 @@ struct MigrationIncomingState {
> > >  
> > >      /* See savevm.c */
> > >      LoadStateEntry_Head loadvm_handlers;
> > > +
> > > +    /*
> > > +     *  Tree for keeping postcopy downtime,
> > > +     *  necessary to calculate correct downtime, during multiple
> > > +     *  vm suspends, it keeps host page address as a key and
> > > +     *  DowntimeDuration as a data
> > > +     *  NULL means kernel couldn't provide process thread id,
> > > +     *  and QEMU couldn't identify which vCPU raise page fault
> > > +     */
> > > +    GTree *postcopy_downtime;
> > >  };
> > >  
> > >  MigrationIncomingState *migration_incoming_get_current(void);
> > >  void migration_incoming_state_destroy(void);
> > > +void mark_postcopy_downtime_begin(uint64_t addr, int cpu);
> > > +void mark_postcopy_downtime_end(uint64_t addr);
> > > +uint64_t get_postcopy_total_downtime(void);
> > > +void destroy_downtime_duration(gpointer data);
> > >  
> > >  /*
> > >   * An outstanding page request, on the source, having been received
> > > diff --git a/migration/migration.c b/migration/migration.c
> > > index 79f6425..5bac434 100644
> > > --- a/migration/migration.c
> > > +++ b/migration/migration.c
> > > @@ -38,6 +38,8 @@
> > >  #include "io/channel-tls.h"
> > >  #include "migration/colo.h"
> > >  
> > > +#define DEBUG_VCPU_DOWNTIME 1
> > > +
> > >  #define MAX_THROTTLE  (32 << 20)      /* Migration transfer speed 
> > > throttling */
> > >  
> > >  /* Amount of time to allocate to each "chunk" of bandwidth-throttled
> > > @@ -77,6 +79,19 @@ static NotifierList migration_state_notifiers =
> > >  
> > >  static bool deferred_incoming;
> > >  
> > > +typedef struct {
> > > +    int64_t begin;
> > > +    int64_t end;
> > > +    uint64_t *cpus; /* cpus bit mask array, QEMU bit functions support
> > > +     bit operation on memory regions, but doesn't check out of range */
> > > +} DowntimeDuration;
> > > +
> > > +typedef struct {
> > > +    int64_t tp; /* point in time */
> > > +    bool is_end;
> > > +    uint64_t *cpus;
> > > +} OverlapDowntime;
> > > +
> > >  /*
> > >   * Current state of incoming postcopy; note this is not part of
> > >   * MigrationIncomingState since it's state is used during cleanup
> > > @@ -117,6 +132,13 @@ MigrationState *migrate_get_current(void)
> > >      return &current_migration;
> > >  }
> > >  
> > > +void destroy_downtime_duration(gpointer data)
> > > +{
> > > +    DowntimeDuration *dd = (DowntimeDuration *)data;
> > > +    g_free(dd->cpus);
> > > +    g_free(data);
> > > +}
> > > +
> > >  MigrationIncomingState *migration_incoming_get_current(void)
> > >  {
> > >      static bool once;
> > > @@ -138,10 +160,13 @@ void migration_incoming_state_destroy(void)
> > >      struct MigrationIncomingState *mis = 
> > > migration_incoming_get_current();
> > >  
> > >      qemu_event_destroy(&mis->main_thread_load_event);
> > > +    if (mis->postcopy_downtime) {
> > > +        g_tree_destroy(mis->postcopy_downtime);
> > > +        mis->postcopy_downtime = NULL;
> > > +    }
> > >      loadvm_free_handlers(mis);
> > >  }
> > >  
> > > -
> > >  typedef struct {
> > >      bool optional;
> > >      uint32_t size;
> > > @@ -1754,7 +1779,6 @@ static int postcopy_start(MigrationState *ms, bool 
> > > *old_vm_running)
> > >       */
> > >      ms->postcopy_after_devices = true;
> > >      notifier_list_notify(&migration_state_notifiers, ms);
> > > -
> > 
> > Stray deletion
> > 
> > >      ms->downtime =  qemu_clock_get_ms(QEMU_CLOCK_REALTIME) - 
> > > time_at_stop;
> > >  
> > >      qemu_mutex_unlock_iothread();
> > > @@ -2117,3 +2141,255 @@ PostcopyState postcopy_state_set(PostcopyState 
> > > new_state)
> > >      return atomic_xchg(&incoming_postcopy_state, new_state);
> > >  }
> > >  
> > > +#define SIZE_TO_KEEP_CPUBITS (1 + smp_cpus/sizeof(guint64))
> > 
> > Split out your cpu-sets so that you have an 'alloc_cpu_set',
> > a 'set bit' a 'set all bits', dup etc
> > (I see Linux has cpumask.h that has a 'cpu_set' that's
> > basically the same thing, but we need something portablish.)
> > 
> Agree, the way I'm working with cpumask is little bit naive.
> instead of set all_cpumask in case when all vCPU are sleeping with precision
> ((1 << smp_cpus) - 1), I just set ~0 it all, because I didn't use
> functions like cpumask_and.
> If you think, this patch should use cpumask, cpumask patchset/separate
> thread should be introduced before, and then this patchset should be
> rebased on top of it.

Yes, some functions like that - because then it makes this code
clearer; and there's less repetition.

Dave

> 
> BR
> Alexey
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK