Re: [PATCH v2 15/29] migration/ram: Add support for 'fixed-ram' outgoing migration

[Fabiano Rosas]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Oct 23, 2023 at 05:35:54PM -0300, Fabiano Rosas wrote:
>> From: Nikolay Borisov <nborisov@suse.com>
>> 
>> Implement the outgoing migration side for the 'fixed-ram' capability.
>> 
>> A bitmap is introduced to track which pages have been written in the
>> migration file. Pages are written at a fixed location for every
>> ramblock. Zero pages are ignored as they'd be zero in the destination
>> migration as well.
>> 
>> The migration stream is altered to put the dirty pages for a ramblock
>> after its header instead of having a sequential stream of pages that
>> follow the ramblock headers. Since all pages have a fixed location,
>> RAM_SAVE_FLAG_EOS is no longer generated on every migration iteration.
>> 
>> Without fixed-ram (current):
>> 
>> ramblock 1 header|ramblock 2 header|...|RAM_SAVE_FLAG_EOS|stream of
>>  pages (iter 1)|RAM_SAVE_FLAG_EOS|stream of pages (iter 2)|...
>
> Clearer to diagram this vertically I think
>
>  ---------------------
>  | ramblock 1 header |
>  ---------------------
>  | ramblock 2 header |
>  ---------------------
>  | ...               |
>  ---------------------
>  | ramblock n header |
>  ---------------------
>  | RAM_SAVE_FLAG_EOS |
>  ---------------------
>  | stream of pages   |
>  | (iter 1)          |
>  | ...               |
>  ---------------------
>  | RAM_SAVE_FLAG_EOS |
>  ---------------------
>  | stream of pages   |
>  | (iter 2)          |
>  | ...               |
>  ---------------------
>  | ...               |
>  ---------------------
>  
>
>> 
>> With fixed-ram (new):
>> 
>> ramblock 1 header|ramblock 1 fixed-ram header|ramblock 1 pages (fixed
>>  offsets)|ramblock 2 header|ramblock 2 fixed-ram header|ramblock 2
>>  pages (fixed offsets)|...|RAM_SAVE_FLAG_EOS
>
> If I'm reading the code correctly the new format has some padding
> such that each "ramblock pages" region starts on a 1 MB boundary.
>
> eg so we get:
>
>  --------------------------------
>  | ramblock 1 header            |
>  --------------------------------
>  | ramblock 1 fixed-ram header  |
>  --------------------------------
>  | padding to next 1MB boundary |
>  | ...                          |
>  --------------------------------
>  | ramblock 1 pages             |
>  | ...                          |
>  --------------------------------
>  | ramblock 2 header            |
>  --------------------------------
>  | ramblock 2 fixed-ram header  |
>  --------------------------------
>  | padding to next 1MB boundary |
>  | ...                          |
>  --------------------------------
>  | ramblock 2 pages             |
>  | ...                          |
>  --------------------------------
>  | ...                          |
>  --------------------------------
>  | RAM_SAVE_FLAG_EOS            |
>  --------------------------------
>  | ...                          |
>  -------------------------------
>
>> 
>> where:
>>  - ramblock header: the generic information for a ramblock, such as
>>    idstr, used_len, etc.
>> 
>>  - ramblock fixed-ram header: the new information added by this
>>    feature: bitmap of pages written, bitmap size and offset of pages
>>    in the migration file.
>> 
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  include/exec/ramblock.h |  8 ++++
>>  migration/options.c     |  3 --
>>  migration/ram.c         | 98 ++++++++++++++++++++++++++++++++++++-----
>>  3 files changed, 96 insertions(+), 13 deletions(-)
>> 
>> diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
>> index 69c6a53902..e0e3f16852 100644
>> --- a/include/exec/ramblock.h
>> +++ b/include/exec/ramblock.h
>> @@ -44,6 +44,14 @@ struct RAMBlock {
>>      size_t page_size;
>>      /* dirty bitmap used during migration */
>>      unsigned long *bmap;
>> +    /* shadow dirty bitmap used when migrating to a file */
>> +    unsigned long *shadow_bmap;
>> +    /*
>> +     * offset in the file pages belonging to this ramblock are saved,
>> +     * used only during migration to a file.
>> +     */
>> +    off_t bitmap_offset;
>> +    uint64_t pages_offset;
>>      /* bitmap of already received pages in postcopy */
>>      unsigned long *receivedmap;
>>  
>> diff --git a/migration/options.c b/migration/options.c
>> index 2622d8c483..9f693d909f 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -271,12 +271,9 @@ bool migrate_events(void)
>>  
>>  bool migrate_fixed_ram(void)
>>  {
>> -/*
>>      MigrationState *s = migrate_get_current();
>>  
>>      return s->capabilities[MIGRATION_CAPABILITY_FIXED_RAM];
>> -*/
>> -    return false;
>>  }
>>  
>>  bool migrate_ignore_shared(void)
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 92769902bb..152a03604f 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1157,12 +1157,18 @@ static int save_zero_page(RAMState *rs, 
>> PageSearchStatus *pss,
>>          return 0;
>>      }
>>  
>> +    stat64_add(&mig_stats.zero_pages, 1);
>> +
>> +    if (migrate_fixed_ram()) {
>> +        /* zero pages are not transferred with fixed-ram */
>> +        clear_bit(offset >> TARGET_PAGE_BITS, pss->block->shadow_bmap);
>> +        return 1;
>> +    }
>> +
>>      len += save_page_header(pss, file, pss->block, offset | 
>> RAM_SAVE_FLAG_ZERO);
>>      qemu_put_byte(file, 0);
>>      len += 1;
>>      ram_release_page(pss->block->idstr, offset);
>> -
>> -    stat64_add(&mig_stats.zero_pages, 1);
>>      ram_transferred_add(len);
>>  
>>      /*
>> @@ -1220,14 +1226,20 @@ static int save_normal_page(PageSearchStatus *pss, 
>> RAMBlock *block,
>>  {
>>      QEMUFile *file = pss->pss_channel;
>>  
>> -    ram_transferred_add(save_page_header(pss, pss->pss_channel, block,
>> -                                         offset | RAM_SAVE_FLAG_PAGE));
>> -    if (async) {
>> -        qemu_put_buffer_async(file, buf, TARGET_PAGE_SIZE,
>> -                              migrate_release_ram() &&
>> -                              migration_in_postcopy());
>> +    if (migrate_fixed_ram()) {
>> +        qemu_put_buffer_at(file, buf, TARGET_PAGE_SIZE,
>> +                           block->pages_offset + offset);
>> +        set_bit(offset >> TARGET_PAGE_BITS, block->shadow_bmap);
>>      } else {
>> -        qemu_put_buffer(file, buf, TARGET_PAGE_SIZE);
>> +        ram_transferred_add(save_page_header(pss, pss->pss_channel, block,
>> +                                             offset | RAM_SAVE_FLAG_PAGE));
>> +        if (async) {
>> +            qemu_put_buffer_async(file, buf, TARGET_PAGE_SIZE,
>> +                                  migrate_release_ram() &&
>> +                                  migration_in_postcopy());
>> +        } else {
>> +            qemu_put_buffer(file, buf, TARGET_PAGE_SIZE);
>> +        }
>>      }
>>      ram_transferred_add(TARGET_PAGE_SIZE);
>>      stat64_add(&mig_stats.normal_pages, 1);
>> @@ -2475,6 +2487,8 @@ static void ram_save_cleanup(void *opaque)
>>          block->clear_bmap = NULL;
>>          g_free(block->bmap);
>>          block->bmap = NULL;
>> +        g_free(block->shadow_bmap);
>> +        block->shadow_bmap = NULL;
>>      }
>>  
>>      xbzrle_cleanup();
>> @@ -2842,6 +2856,7 @@ static void ram_list_init_bitmaps(void)
>>               */
>>              block->bmap = bitmap_new(pages);
>>              bitmap_set(block->bmap, 0, pages);
>> +            block->shadow_bmap = bitmap_new(block->used_length >> 
>> TARGET_PAGE_BITS);
>>              block->clear_bmap_shift = shift;
>>              block->clear_bmap = bitmap_new(clear_bmap_size(pages, shift));
>>          }
>> @@ -2979,6 +2994,44 @@ void qemu_guest_free_page_hint(void *addr, size_t len)
>>      }
>>  }
>>  
>> +#define FIXED_RAM_HDR_VERSION 1
>> +struct FixedRamHeader {
>> +    uint32_t version;
>> +    uint64_t page_size;
>> +    uint64_t bitmap_offset;
>> +    uint64_t pages_offset;
>> +    /* end of v1 */
>> +} QEMU_PACKED;
>> +
>> +static void fixed_ram_insert_header(QEMUFile *file, RAMBlock *block)
>> +{
>> +    g_autofree struct FixedRamHeader *header;
>> +    size_t header_size, bitmap_size;
>> +    long num_pages;
>> +
>> +    header = g_new0(struct FixedRamHeader, 1);
>> +    header_size = sizeof(struct FixedRamHeader);
>> +
>> +    num_pages = block->used_length >> TARGET_PAGE_BITS;
>> +    bitmap_size = BITS_TO_LONGS(num_pages) * sizeof(unsigned long);
>> +
>> +    /*
>> +     * Save the file offsets of where the bitmap and the pages should
>> +     * go as they are written at the end of migration and during the
>> +     * iterative phase, respectively.
>> +     */
>> +    block->bitmap_offset = qemu_get_offset(file) + header_size;
>> +    block->pages_offset = ROUND_UP(block->bitmap_offset +
>> +                                   bitmap_size, 0x100000);
>
> The 0x100000 gives us our 1 MB alignment.
>
> This is quite an important thing, so can we put a nice clear
> constant at the top of the file:
>
>   /* Align fixed-ram pages data to the next 1 MB boundary */
>   #define FIXED_RAM_PAGE_OFFSET_ALIGNMENT 0x100000
>
>> @@ -3179,7 +3252,6 @@ out:
>>          qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
>>          qemu_fflush(f);
>>          ram_transferred_add(8);
>> -
>>          ret = qemu_file_get_error(f);
>>      }
>>      if (ret < 0) {
>
> Supurious whitspace change, possibly better in whatever patch
> introduced it, or dropped ?
>
>
> With regards,
> Daniel

I'll apply all the suggestions. Thanks!