* Paolo Bonzini (pbonz...@redhat.com) wrote:
> Il 03/10/2014 19:47, Dr. David Alan Gilbert (git) ha scritto:
> > + /*
> > + * Don't break host-page chunks up with queue items
> > + * so only unqueue if,
> > + * a) The last item came from the queue anyway
> > + * b) The last sent item was the last target-page in a host page
> > + */
> > + if (last_was_from_queue || (!last_sent_block) ||
>
> Extra parentheses.
Fixed.
> Is the last_was_from_queue check necessary? Or
> would one of the other checks be true anyway if last_was_from_queue is true?
if (last_was_from_queue || !last_sent_block ||
((last_offset & (hps - 1)) == (hps - TARGET_PAGE_SIZE))) {
tmpblock = ram_save_unqueue_page(ms, &tmpoffset, &bitoffset);
}
The last_was_from_queue is needed. We're going around this loop in Target-page
chunks (that correspond to one bit in the migration bitmap) and want to make
sure
we don't break into the middle of an existing host-page with an entry off the
queue.
So (going backwards in that if):
We can take something from the queue if:
a) We just sent the last TP in a HP
b) We didn't send anything yet (unlikely)
c) The last thing came from the queue anyway
(c) is needed to override (a) when we've just sent a TP from the queue but it's
not the last TP in the HP that came from the queue; otherwise we'd send the
first
TP from the queue and resume taking pages from the background scan.
> > + /* We're sending this page, and since it's postcopy nothing
> > else
> > + * will dirty it, and we must make sure it doesn't get sent
> > again.
> > + */
> > + if (!migration_bitmap_clear_dirty(bitoffset <<
> > TARGET_PAGE_BITS)) {
> > + DPRINTF("%s: Not dirty for postcopy %s/%zx bito=%zx
> > (sent=%d)",
> > + __func__, tmpblock->idstr, tmpoffset, bitoffset,
> > + test_bit(bitoffset, ms->sentmap));
>
> If a DPRINTF occurs in a loop, please change it to a tracepoint.
OK, I'll look at that - most of arch_init (and migration) still use DPRINTF.
> This function looks like a candidate for cleaning its logic up and/or
> splitting it. But it can be done later by the poor soul who will touch
> it next. :)
Yep, I've already done it once (see 14bcfdc7f - Split ram_save_block).
Dave
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
