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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Amit Shah (amit.shah@redhat.com) wrote:
> On (Tue) 16 Jun 2015 [11:26:38], Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > 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 <dgilbert@redhat.com>
> 
> Reviewed-by: Amit Shah <amit.shah@redhat.com>
> 
> 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).

Fixed.

> > +/*
> > + * 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

Fixed.

> > +/*
> > + * 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?)

Fixed (I'm sure there are others, still getting into the hang of that).

> > +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.

Done.

Dave

> 
> 		Amit
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK