On Tue, Jan 27, 2015 at 09:40:12AM +0000, Dr. David Alan Gilbert wrote: > * David Gibson (da...@gibson.dropbear.id.au) wrote: > > On Wed, Jan 14, 2015 at 08:13:27PM +0000, Dr. David Alan Gilbert wrote: > > > * David Gibson (da...@gibson.dropbear.id.au) wrote: > > > > On Fri, Oct 03, 2014 at 06:47:43PM +0100, Dr. David Alan Gilbert (git) > > > > wrote: > > > > > From: "Dr. David Alan Gilbert" <dgilb...@redhat.com> > > > > > > > > > > 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 <dgilb...@redhat.com> > > > > > --- > > > > > 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
pgpEiztSuGy8e.pgp
Description: PGP signature