Re: [Qemu-devel] [PATCH v4 37/47] Page request: Consume pages off the post-copy queue

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

* Paolo Bonzini (pbonzini@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 / dgilbert@redhat.com / Manchester, UK