Re: [Qemu-devel] [PATCH v7 25/42] Postcopy: Maintain sentmap and calculate discard

[Amit Shah]

On (Tue) 16 Jun 2015 [11:26:38], Dr. David Alan Gilbert (git) 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>

Some whitespace issues, and some sentences in comments don't have a
full-stop:

> +/*
> + * Called by the bitmap code for each chunk to discard
> + * May send a discard message, may just leave it queued to
> + * be sent later
> + * 'start' and 'end' describe an inclusive range of pages in the
> + * migration bitmap in the RAM block passed to postcopy_discard_send_init
> + */
> +void postcopy_discard_send_range(MigrationState *ms, PostcopyDiscardState 
> *pds,
> +                                unsigned long start, unsigned long end);

unaligned line; no full-stop in comment above (similar elsewhere, not
repeating that).

> +/*
> + * Discard the contents of memory start..end inclusive.
> + * We can assume that if we've been called postcopy_ram_hosttest returned 
> true
> + */
> +int postcopy_ram_discard_range(MigrationIncomingState *mis, uint8_t *start,
> +                               uint8_t *end)
> +{
> +    trace_postcopy_ram_discard_range(start, end);
> +    if (madvise(start, (end-start)+1, MADV_DONTNEED)) {

whitespace around operators

> +/*
> + * Called by the bitmap code for each chunk to discard
> + * May send a discard message, may just leave it queued to
> + * be sent later
> + * 'start' and 'end' describe an inclusive range of pages in the
> + * migration bitmap in the RAM block passed to postcopy_discard_send_init

missing punctuation

(also, you had started doing doxygen-style comments, want to keep on
following that style?)

> +static RAMBlock *ram_find_block(const char *id)

just a suggestion, not very particular about this:  rename to

ram_find_block_by_id()

instead, so that it's clear what method of finding we're using; also
no name conflicts when there might be other ways of doing a find.


                Amit