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

[Dr. David Alan Gilbert]

* David Gibson (address@hidden) wrote:
> On Wed, Jan 14, 2015 at 08:13:27PM +0000, Dr. David Alan Gilbert wrote:
> > * David Gibson (address@hidden) wrote:
> > > On Fri, Oct 03, 2014 at 06:47:43PM +0100, Dr. David Alan Gilbert (git) 
> > > wrote:
> > > > From: "Dr. David Alan Gilbert" <address@hidden>
> > > > 
> > > > When transmitting RAM pages, consume pages that have been queued by
> > > > MIG_RPCOMM_REQPAGE commands and send them ahead of normal page scanning.
> > > > 
> > > > Note:
> > > >   a) After a queued page the linear walk carries on from after the
> > > > unqueued page; there is a reasonable chance that the destination
> > > > was about to ask for other closeby pages anyway.
> > > > 
> > > >   b) We have to be careful of any assumptions that the page walking
> > > > code makes, in particular it does some short cuts on its first linear
> > > > walk that break as soon as we do a queued page.
> > > > 
> > > > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > > > ---
> > > >  arch_init.c | 149 
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++----------
> > > >  1 file changed, 125 insertions(+), 24 deletions(-)
> > > > 
> > > > diff --git a/arch_init.c b/arch_init.c
> > > > index 72f9e17..a945990 100644
> > > > +
> > > > +        /*
> > > > +         * Don't break host-page chunks up with queue items
> > > 
> > > Why does this matter?
> > 
> > See the comment you make in a few patches time, it's about being able
> > to place the pages atomically on the destination.
> 
> Hmm.  But if the destination has to wait for all the pieces of a host
> page to arrive anyway, does it really make any difference if they're
> contiguous in the stream?

The problem is knowing where to put the arriving target-pages until you've
got a full host-page; you've got to put the arriving TP into a temporary
until you have the full set, if they're not contiguous in the stream
then you have to have multiple temporarys dealing with the set of outstanding
host pages that you've not got the full set for; and you've still got to be
careful on the sending side to have a bounded-number of host-pages on the run
at any time.   Making that bound 1 makes the code simpler.

> > > > +         * 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) ||
> > > > +            ((last_offset & (hps - 1)) == (hps - TARGET_PAGE_SIZE))) {
> > > > +            tmpblock = ram_save_unqueue_page(ms, &tmpoffset, 
> > > > &bitoffset);
> > > >          }
> > > > -        if (offset >= block->length) {
> > > > -            offset = 0;
> > > > -            block = QTAILQ_NEXT(block, next);
> > > > -            if (!block) {
> > > > -                block = QTAILQ_FIRST(&ram_list.blocks);
> > > > -                complete_round = true;
> > > > -                ram_bulk_stage = false;
> > > > +
> > > > +        if (tmpblock) {
> > > > +            /* We've got a block from the postcopy queue */
> > > > +            DPRINTF("%s: Got postcopy item '%s' offset=%zx 
> > > > bitoffset=%zx",
> > > > +                    __func__, tmpblock->idstr, tmpoffset, bitoffset);
> > > > +            /* 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)) {
> > > 
> > > Ugh.. that's kind of subtle.  I think it would be clearer if you work
> > > in terms of a ram_addr_t throughout, rather than "bitoffset" whose
> > > meaning is not terribly obvious.
> > 
> > I've changed it to ram_addr_t as requested; it's slightly clearer but there
> > are a few places where we're dealing with the sentmap where we now need to 
> > shift
> > the other way.  In the end ram_addr_t is really a scaled offset into those
> > bitmaps.
> 
> Right, but to someone who isn't deeply familiar with the code, they're
> more likely to understand what the ram address means than the bitmap
> offset.

Fair enough.

Dave

> 
> -- 
> David Gibson                  | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au        | minimalist, thank you.  NOT _the_ 
> _other_
>                               | _way_ _around_!
> http://www.ozlabs.org/~dgibson


--
Dr. David Alan Gilbert / address@hidden / Manchester, UK