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

[David Gibson]

On Tue, Jan 27, 2015 at 09:40:12AM +0000, Dr. David Alan Gilbert wrote:
> * 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.

Ah, right, I see your point.

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

pgpNCpy0a3Y1J.pgp
Description: PGP signature