Re: [Qemu-devel] [PATCH v8 32/54] Postcopy: Maintain sentmap and calculate discard

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <address@hidden> wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
>
> Where postcopy is preceeded by a period of precopy, the destination will
> have received pages that may have been dirtied on the source after the
> page was sent.  The destination must throw these pages away before
> starting it's CPUs.
>
> Maintain a 'sentmap' of pages that have already been sent.
> Calculate list of sent & dirty pages
> Provide helpers on the destination side to discard these.
>
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> Reviewed-by: Amit Shah <address@hidden>


Hi

>      /* Flag set once the migration has been asked to enter postcopy */
>      bool start_postcopy;


This is from a previous patch, but ....

Change the name of the variable or the comment?  From the comment it
sholud be "in_postcopy", no?


> +
> +    /* bitmap of pages that have been sent at least once
> +     * only maintained and used in postcopy at the moment
> +     * where it's used to send the dirtymap at the start
> +     * of the postcopy phase
> +     */
> +    unsigned long *sentmap;
> };

I *think* that patch would be easier if you put this one inside
migration_bitmap_rcu.  If you put it there, yoqu could do the 

> +                if (ms->sentmap) {
> +                    set_bit(dirty_ram_abs >> TARGET_PAGE_BITS, ms->sentmap);
> +                }

And you wouldn't have to change all callers to have an ram_addr_abs
address parameter, right?


> +struct PostcopyDiscardState {
> +    const char *name;

Iht is not obvious to me what name means here.  I assume ram block name,
change it to block_name, ramblock?


> + * returns: 0 on success.
> + */
> +int postcopy_ram_discard_range(MigrationIncomingState *mis, uint8_t *start,
> +                               size_t length)
> +{
> +    trace_postcopy_ram_discard_range(start, length);
> +    if (madvise(start, length, MADV_DONTNEED)) {
> +        error_report("%s MADV_DONTNEED: %s", __func__, strerror(errno));
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
>  #else
>  /* No target OS support, stubs just fail */
>  bool postcopy_ram_supported_by_host(void)
> @@ -153,5 +192,95 @@ bool postcopy_ram_supported_by_host(void)
>      return false;
>  }
>  
> +int postcopy_ram_discard_range(MigrationIncomingState *mis, uint8_t *start,
> +                               size_t length)
> +{
> +    assert(0);

I will assume that just returning -1 would work here.

But yes, I understand that this code shouldn't be reach ...

> +}
>  #endif
>  
> +/* ------------------------------------------------------------------------- 
> */
> +
> +/**
> + * postcopy_discard_send_init: Called at the start of each RAMBlock before
> + *   asking to discard individual ranges.
> + *
> + * @ms: The current migration state.
> + * @offset: the bitmap offset of the named RAMBlock in the migration
> + *   bitmap.
> + * @name: RAMBlock that discards will operate on.
> + *
> + * returns: a new PDS.
> + */
> +PostcopyDiscardState *postcopy_discard_send_init(MigrationState *ms,
> +                                                 unsigned long offset,
> +                                                 const char *name)
> +{
> +    PostcopyDiscardState *res = g_try_malloc(sizeof(PostcopyDiscardState));

Why are we using here g_try_malloc instead of g_malloc()?  Even
g_malloc0()?

Specially when we don't check if res is NULL on return.  Please change.


> +
> +    if (res) {
> +        res->name = name;
> +        res->cur_entry = 0;
> +        res->nsentwords = 0;
> +        res->nsentcmds = 0;

With the zero variant, this three can be removed.

> +        res->offset = offset;
> +    }
> +
> +    return res;
> +}

> -/* Called with rcu_read_lock() to protect migration_bitmap */
> +/* Called with rcu_read_lock() to protect migration_bitmap
> + * mr: The region to search for dirty pages in

Haha, you forgot to update the comment when you moved the function to
use ram blocks O:-)


> @@ -662,6 +672,24 @@ static int save_zero_page(QEMUFile *f, RAMBlock *block, 
> ram_addr_t offset,
>  }
>  
>  /**
> + * ram_find_block_by_id: Find a ramblock by name.
> + *
> + * Returns: The RAMBlock with matching ID, or NULL.
> + */
> +static RAMBlock *ram_find_block_by_id(const char *id)
> +{
> +    RAMBlock *block;
> +
> +    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
> +        if (!strcmp(id, block->idstr)) {
> +            return block;
> +        }
> +    }
> +
> +    return NULL;
> +}

We don't have this function already.....

Once here, could we split it in its own patch and use it in ram_load?


                QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
                    if (!strncmp(id, block->idstr, sizeof(id))) {
                        if (length != block->used_length) {
                            Error *local_err = NULL;

                            ret = qemu_ram_resize(block->offset, length, 
&local_err);
                            if (local_err) {
                                error_report_err(local_err);
                            }
                        }
                        ram_control_load_hook(f, RAM_CONTROL_BLOCK_REG,
                                              block->idstr);
                        break;
                    }
                }

                if (!block) {
                    error_report("Unknown ramblock \"%s\", cannot "
                                 "accept migration", id);
                    ret = -EINVAL;
                }


We could also use it in:

host_from_stream_offset


> +/* **** functions for postcopy ***** */
> +
> +/*
> + * Callback from postcopy_each_ram_send_discard for each RAMBlock
> + * start,end: Indexes into the bitmap for the first and last bit
> + *            representing the named block
> + */
> +static int postcopy_send_discard_bm_ram(MigrationState *ms,
> +                                        PostcopyDiscardState *pds,
> +                                        unsigned long start, unsigned long 
> end)
> +{
> +    unsigned long current;
> +
> +    for (current = start; current <= end; ) {
> +        unsigned long set = find_next_bit(ms->sentmap, end + 1, current);
> +
> +        if (set <= end) {
> +            unsigned long zero = find_next_zero_bit(ms->sentmap,
> +                                                    end + 1, set + 1);
> +
> +            if (zero > end) {
> +                zero = end + 1;
> +            }
> +            postcopy_discard_send_range(ms, pds, set, zero - 1);
> +            current = zero + 1;
> +        } else {
> +            current = set;
> +        }
> +    }

I think I undrestood the logic  here at the end....

But if we change the meaning of postcopy_discard_send_range() from
(begin, end), to (begin, length), I think everything goes clearer, no?

        if (set <= end) {
            unsigned long zero = find_next_zero_bit(ms->sentmap,
                                                    end + 1, set + 1);
            unsigned long length;

            if (zero > end) {
                length = end - set;
            } else {
                lenght = zero - 1 - set;
                current = zero + 1;
            }
            postcopy_discard_send_range(ms, pds, set, len);
        } else {
            current = set;
        }
    }

Y would clame that if we call one zero, the other would be called one.
Or change to set/unset, but that is just me.  Yes, I haven't tested, and
it is possible that there is a off-by-one somewhere...


Looking at postocpy_eand_ram_send_discard, I even think that it would be
a good idea to pass length to this function.

> +/*
> + * Transmit the set of pages to be discarded after precopy to the target
> + * these are pages that:
> + *     a) Have been previously transmitted but are now dirty again
> + *     b) Pages that have never been transmitted, this ensures that
> + *        any pages on the destination that have been mapped by background
> + *        tasks get discarded (transparent huge pages is the specific 
> concern)
> + * Hopefully this is pretty sparse
> + */
> +int ram_postcopy_send_discard_bitmap(MigrationState *ms)
> +{
> +    int ret;
> +
> +    rcu_read_lock();
> +
> +    /* This should be our last sync, the src is now paused */
> +    migration_bitmap_sync();
> +
> +    /*
> +     * Update the sentmap to be sentmap = ~sentmap | dirty
> +     */
> +    bitmap_complement(ms->sentmap, ms->sentmap,
> +               last_ram_offset() >> TARGET_PAGE_BITS);
> +
> +    bitmap_or(ms->sentmap, ms->sentmap, migration_bitmap,
> +               last_ram_offset() >> TARGET_PAGE_BITS);

This bitmaps are really big, I don't know how long would take to do this
operations here, but I think that we can avoid (at least) the
bitmap_complement.  We can change the bitmap name to notsentbitmap, init
it to one and clear it each time that we sent a page, no?

We can also do the bitmap_or() at migration_sync_bitmap() time, at that
point, we shouldn't be on the critical path?

Later, Juan.