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?
> > > + * 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.
--
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
pgpAWP6hFENIb.pgp
Description: PGP signature
