Re: [Qemu-devel] [PATCH v2] migration: discard non-migratable RAMBlocks

[Cédric Le Goater]

Hello David,

On 04/19/2018 06:58 PM, Dr. David Alan Gilbert wrote:
> * Cédric Le Goater (address@hidden) wrote:
>> On the POWER9 processor, the XIVE interrupt controller can control
>> interrupt sources using MMIO to trigger events, to EOI or to turn off
>> the sources. Priority management and interrupt acknowledgment is also
>> controlled by MMIO in the presenter sub-engine.
>>
>> These MMIO regions are exposed to guests in QEMU with a set of 'ram
>> device' memory mappings, similarly to VFIO, and the VMAs are populated
>> dynamically with the appropriate pages using a fault handler.
>>
>> But, these regions are an issue for migration. We need to discard the
>> associated RAMBlocks from the RAM state on the source VM and let the
>> destination VM rebuild the memory mappings on the new host in the
>> post_load() operation just before resuming the system.
>>
>> To achieve this goal, the following introduces a new RAMBlock flag
>> RAM_MIGRATABLE which is updated in the vmstate_register_ram() and
>> vmstate_unregister_ram() routines. This flag is then used by the
>> migration to identify RAMBlocks to discard on the source. Some checks
>> are also performed on the destination to make sure nothing invalid was
>> sent.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
> 
> Hi Cédric,
>   Yes, this is looking nicer; the macro makes the changes quite small.
> A couple of questions:
> 
>> ---
>>
>>  Changes since v1:
>>
>>  - introduced a new RAMBlock flag RAM_MIGRATABLE
>>  - fixed RAMBLOCK_FOREACH_MIGRATABLE() macro to handle code that
>>    omitted {}. 
>>
>>  exec.c                    | 10 ++++++++++
>>  include/exec/cpu-common.h |  1 +
>>  migration/ram.c           | 42 ++++++++++++++++++++++++++++++++----------
>>  3 files changed, 43 insertions(+), 10 deletions(-)
>>
>> diff --git a/exec.c b/exec.c
>> index 02b1efebb7c3..e9432c06294e 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -104,6 +104,9 @@ static MemoryRegion io_mem_unassigned;
>>   * (Set during postcopy)
>>   */
>>  #define RAM_UF_ZEROPAGE (1 << 3)
>> +
>> +/* RAM can be migrated */
>> +#define RAM_MIGRATABLE (1 << 4)
>>  #endif
>>  
>>  #ifdef TARGET_PAGE_BITS_VARY
>> @@ -1807,6 +1810,11 @@ void qemu_ram_set_uf_zeroable(RAMBlock *rb)
>>      rb->flags |= RAM_UF_ZEROPAGE;
>>  }
>>  
>> +bool qemu_ram_is_migratable(RAMBlock *rb)
>> +{
>> +    return rb->flags & RAM_MIGRATABLE;
>> +}
>> +
>>  /* Called with iothread lock held.  */
>>  void qemu_ram_set_idstr(RAMBlock *new_block, const char *name, DeviceState 
>> *dev)
>>  {
>> @@ -1823,6 +1831,7 @@ void qemu_ram_set_idstr(RAMBlock *new_block, const 
>> char *name, DeviceState *dev)
>>          }
>>      }
>>      pstrcat(new_block->idstr, sizeof(new_block->idstr), name);
>> +    new_block->flags |= RAM_MIGRATABLE;
>>  
>>      rcu_read_lock();
>>      RAMBLOCK_FOREACH(block) {
>> @@ -1845,6 +1854,7 @@ void qemu_ram_unset_idstr(RAMBlock *block)
>>       */
>>      if (block) {
>>          memset(block->idstr, 0, sizeof(block->idstr));
>> +        block->flags &= ~RAM_MIGRATABLE;
>>      }
>>  }
> 
> Why in qemu_ram_set_idstr and qemu_ram_(un)set_idstr ?  It seems an
> odd place to put them.

The only place where this routines are called is from vmstate_un/register_ram()
It seemed unnecessary to add an extra interface qemu_ram_un/set_migratable().

May be should just rename qemu_ram_un/set_idstr() ?

> 
>> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
>> index 24d335f95d45..96854519b514 100644
>> --- a/include/exec/cpu-common.h
>> +++ b/include/exec/cpu-common.h
>> @@ -75,6 +75,7 @@ const char *qemu_ram_get_idstr(RAMBlock *rb);
>>  bool qemu_ram_is_shared(RAMBlock *rb);
>>  bool qemu_ram_is_uf_zeroable(RAMBlock *rb);
>>  void qemu_ram_set_uf_zeroable(RAMBlock *rb);
>> +bool qemu_ram_is_migratable(RAMBlock *rb);
>>  
>>  size_t qemu_ram_pagesize(RAMBlock *block);
>>  size_t qemu_ram_pagesize_largest(void);
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 0e90efa09236..89c3accc4e26 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -187,6 +187,11 @@ void ramblock_recv_bitmap_set_range(RAMBlock *rb, void 
>> *host_addr,
>>                        nr);
>>  }
>>  
>> +/* Should be holding either ram_list.mutex, or the RCU lock. */
>> +#define RAMBLOCK_FOREACH_MIGRATABLE(block)             \
>> +    RAMBLOCK_FOREACH(block)                            \
>> +        if (!qemu_ram_is_migratable(block)) {} else
>> +
>>  /*
>>   * An outstanding page request, on the source, having been received
>>   * and queued
>> @@ -780,6 +785,10 @@ unsigned long migration_bitmap_find_dirty(RAMState *rs, 
>> RAMBlock *rb,
>>      unsigned long *bitmap = rb->bmap;
>>      unsigned long next;
>>  
>> +    if (!qemu_ram_is_migratable(rb)) {
>> +        return size;
>> +    }
>> +
>>      if (rs->ram_bulk_stage && start > 0) {
>>          next = start + 1;
>>      } else {
>> @@ -825,7 +834,7 @@ uint64_t ram_pagesize_summary(void)
>>      RAMBlock *block;
>>      uint64_t summary = 0;
>>  
>> -    RAMBLOCK_FOREACH(block) {
>> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
>>          summary |= block->page_size;
>>      }
>>  
>> @@ -849,7 +858,7 @@ static void migration_bitmap_sync(RAMState *rs)
>>  
>>      qemu_mutex_lock(&rs->bitmap_mutex);
>>      rcu_read_lock();
>> -    RAMBLOCK_FOREACH(block) {
>> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
>>          migration_bitmap_sync_range(rs, block, 0, block->used_length);
>>      }
>>      rcu_read_unlock();
>> @@ -1499,6 +1508,10 @@ static int ram_save_host_page(RAMState *rs, 
>> PageSearchStatus *pss,
>>      size_t pagesize_bits =
>>          qemu_ram_pagesize(pss->block) >> TARGET_PAGE_BITS;
>>  
>> +    if (!qemu_ram_is_migratable(pss->block)) {
>> +        return 0;
>> +    }
>> +
> 
> We might want to print a diagnostic there - it shouldn't happen right?

yes it should not. it should even be fatal. 

>>      do {
>>          tmppages = ram_save_target_page(rs, pss, last_stage);
>>          if (tmppages < 0) {
>> @@ -1587,7 +1600,7 @@ uint64_t ram_bytes_total(void)
>>      uint64_t total = 0;
>>  
>>      rcu_read_lock();
>> -    RAMBLOCK_FOREACH(block) {
>> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
>>          total += block->used_length;
>>      }
>>      rcu_read_unlock();
>> @@ -1642,7 +1655,7 @@ static void ram_save_cleanup(void *opaque)
>>       */
>>      memory_global_dirty_log_stop();
>>  
>> -    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
>> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
>>          g_free(block->bmap);
>>          block->bmap = NULL;
>>          g_free(block->unsentmap);
>> @@ -1705,7 +1718,7 @@ void 
>> ram_postcopy_migrated_memory_release(MigrationState *ms)
>>  {
>>      struct RAMBlock *block;
>>  
>> -    RAMBLOCK_FOREACH(block) {
>> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
>>          unsigned long *bitmap = block->bmap;
>>          unsigned long range = block->used_length >> TARGET_PAGE_BITS;
>>          unsigned long run_start = find_next_zero_bit(bitmap, range, 0);
>> @@ -1783,7 +1796,7 @@ static int 
>> postcopy_each_ram_send_discard(MigrationState *ms)
>>      struct RAMBlock *block;
>>      int ret;
>>  
>> -    RAMBLOCK_FOREACH(block) {
>> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
>>          PostcopyDiscardState *pds =
>>              postcopy_discard_send_init(ms, block->idstr);
>>  
>> @@ -1991,7 +2004,7 @@ int ram_postcopy_send_discard_bitmap(MigrationState 
>> *ms)
>>      rs->last_sent_block = NULL;
>>      rs->last_page = 0;
>>  
>> -    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
>> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
>>          unsigned long pages = block->used_length >> TARGET_PAGE_BITS;
>>          unsigned long *bitmap = block->bmap;
>>          unsigned long *unsentmap = block->unsentmap;
>> @@ -2150,7 +2163,7 @@ static void ram_list_init_bitmaps(void)
>>  
>>      /* Skip setting bitmap if there is no RAM */
>>      if (ram_bytes_total()) {
>> -        QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
>> +        RAMBLOCK_FOREACH_MIGRATABLE(block) {
>>              pages = block->max_length >> TARGET_PAGE_BITS;
>>              block->bmap = bitmap_new(pages);
>>              bitmap_set(block->bmap, 0, pages);
>> @@ -2226,7 +2239,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>>  
>>      qemu_put_be64(f, ram_bytes_total() | RAM_SAVE_FLAG_MEM_SIZE);
>>  
>> -    RAMBLOCK_FOREACH(block) {
>> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
>>          qemu_put_byte(f, strlen(block->idstr));
>>          qemu_put_buffer(f, (uint8_t *)block->idstr, strlen(block->idstr));
>>          qemu_put_be64(f, block->used_length);
> 
> Good, that's all quite neat;  we've just got to watch out we don't
> accidentally add any RAMBLOCK_FOREACH's back.

Thank,

C.

>> @@ -2471,6 +2484,11 @@ static inline RAMBlock 
>> *ram_block_from_stream(QEMUFile *f, int flags)
>>          return NULL;
>>      }
>>  
>> +    if (!qemu_ram_is_migratable(block)) {
>> +        error_report("block %s should not be migrated !", id);
>> +        return NULL;
>> +    }
>> +
>>      return block;
>>  }
>>  
>> @@ -2921,7 +2939,11 @@ static int ram_load(QEMUFile *f, void *opaque, int 
>> version_id)
>>                  length = qemu_get_be64(f);
>>  
>>                  block = qemu_ram_block_by_name(id);
>> -                if (block) {
>> +                if (block && !qemu_ram_is_migratable(block)) {
>> +                    error_report("block %s should not be migrated !", id);
>> +                    ret = -EINVAL;
>> +
>> +                } else if (block) {
>>                      if (length != block->used_length) {
>>                          Error *local_err = NULL;
>>  
>> -- 
>> 2.13.6
> 
> Dave
> 
>>
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK
>