From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43148) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaEMa-0002Wv-Pu for qemu-devel@nongnu.org; Mon, 23 Mar 2015 22:14:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YaEMY-0000xq-Pn for qemu-devel@nongnu.org; Mon, 23 Mar 2015 22:14:40 -0400 Received: from ozlabs.org ([2401:3900:2:1::2]:36892) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaEMY-0000xa-4p for qemu-devel@nongnu.org; Mon, 23 Mar 2015 22:14:38 -0400 Date: Tue, 24 Mar 2015 13:15:24 +1100 From: David Gibson Message-ID: <20150324021524.GY25043@voom.fritz.box> References: <1424883128-9841-1-git-send-email-dgilbert@redhat.com> <1424883128-9841-35-git-send-email-dgilbert@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="oWEYV0WmY9Kcrzh0" Content-Disposition: inline In-Reply-To: <1424883128-9841-35-git-send-email-dgilbert@redhat.com> Subject: Re: [Qemu-devel] [PATCH v5 34/45] Page request: Consume pages off the post-copy queue List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert (git)" Cc: aarcange@redhat.com, yamahata@private.email.ne.jp, quintela@redhat.com, qemu-devel@nongnu.org, amit.shah@redhat.com, pbonzini@redhat.com, yanghy@cn.fujitsu.com --oWEYV0WmY9Kcrzh0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Feb 25, 2015 at 04:51:57PM +0000, Dr. David Alan Gilbert (git) wrot= e: > From: "Dr. David Alan Gilbert" >=20 > When transmitting RAM pages, consume pages that have been queued by > MIG_RPCOMM_REQPAGE commands and send them ahead of normal page scanning. >=20 > 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. >=20 > 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. >=20 > Signed-off-by: Dr. David Alan Gilbert > --- > arch_init.c | 154 +++++++++++++++++++++++++++++++++++++++++++++++++----= ------ > trace-events | 2 + > 2 files changed, 131 insertions(+), 25 deletions(-) >=20 > diff --git a/arch_init.c b/arch_init.c > index 9d8fc6b..acf65e1 100644 > --- a/arch_init.c > +++ b/arch_init.c > @@ -328,6 +328,7 @@ static RAMBlock *last_seen_block; > /* This is the last block from where we have sent data */ > static RAMBlock *last_sent_block; > static ram_addr_t last_offset; > +static bool last_was_from_queue; > static unsigned long *migration_bitmap; > static uint64_t migration_dirty_pages; > static uint32_t last_version; > @@ -461,6 +462,19 @@ static inline bool migration_bitmap_set_dirty(ram_ad= dr_t addr) > return ret; > } > =20 > +static inline bool migration_bitmap_clear_dirty(ram_addr_t addr) > +{ > + bool ret; > + int nr =3D addr >> TARGET_PAGE_BITS; > + > + ret =3D test_and_clear_bit(nr, migration_bitmap); > + > + if (ret) { > + migration_dirty_pages--; > + } > + return ret; > +} > + > static void migration_bitmap_sync_range(ram_addr_t start, ram_addr_t len= gth) > { > ram_addr_t addr; > @@ -669,6 +683,39 @@ static int ram_save_page(QEMUFile *f, RAMBlock* bloc= k, ram_addr_t offset, > } > =20 > /* > + * Unqueue a page from the queue fed by postcopy page requests > + * > + * Returns: The RAMBlock* to transmit from (or NULL if the queue is= empty) > + * ms: MigrationState in > + * offset: the byte offset within the RAMBlock for the start of th= e page > + * ram_addr_abs: global offset in the dirty/sent bitmaps > + */ > +static RAMBlock *ram_save_unqueue_page(MigrationState *ms, ram_addr_t *o= ffset, > + ram_addr_t *ram_addr_abs) > +{ > + RAMBlock *result =3D NULL; > + qemu_mutex_lock(&ms->src_page_req_mutex); > + if (!QSIMPLEQ_EMPTY(&ms->src_page_requests)) { > + struct MigrationSrcPageRequest *entry =3D > + QSIMPLEQ_FIRST(&ms->src_page_request= s); > + result =3D entry->rb; > + *offset =3D entry->offset; > + *ram_addr_abs =3D (entry->offset + entry->rb->offset) & TARGET_P= AGE_MASK; > + > + if (entry->len > TARGET_PAGE_SIZE) { > + entry->len -=3D TARGET_PAGE_SIZE; > + entry->offset +=3D TARGET_PAGE_SIZE; > + } else { > + QSIMPLEQ_REMOVE_HEAD(&ms->src_page_requests, next_req); > + g_free(entry); > + } > + } > + qemu_mutex_unlock(&ms->src_page_req_mutex); > + > + return result; > +} > + > +/* > * Queue the pages for transmission, e.g. a request from postcopy destin= ation > * ms: MigrationStatus in which the queue is held > * rbname: The RAMBlock the request is for - may be NULL (to mean reus= e last) > @@ -732,46 +779,102 @@ int ram_save_queue_pages(MigrationState *ms, const= char *rbname, > =20 > static int ram_find_and_save_block(QEMUFile *f, bool last_stage) > { > + MigrationState *ms =3D migrate_get_current(); > RAMBlock *block =3D last_seen_block; > + RAMBlock *tmpblock; > ram_addr_t offset =3D last_offset; > + ram_addr_t tmpoffset; > bool complete_round =3D false; > int bytes_sent =3D 0; > - MemoryRegion *mr; > ram_addr_t dirty_ram_abs; /* Address of the start of the dirty page = in > ram_addr_t space */ > + unsigned long hps =3D sysconf(_SC_PAGESIZE); > =20 > - if (!block) > + if (!block) { > block =3D QTAILQ_FIRST(&ram_list.blocks); > + last_was_from_queue =3D false; > + } > =20 > - while (true) { > - mr =3D block->mr; > - offset =3D migration_bitmap_find_and_reset_dirty(mr, offset, > - &dirty_ram_abs); > - if (complete_round && block =3D=3D last_seen_block && > - offset >=3D last_offset) { > - break; > + while (true) { /* Until we send a block or run out of stuff to send = */ > + tmpblock =3D NULL; > + > + /* > + * 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 pa= ge > + */ > + if (last_was_from_queue || !last_sent_block || > + ((last_offset & (hps - 1)) =3D=3D (hps - TARGET_PAGE_SIZE)))= { > + tmpblock =3D ram_save_unqueue_page(ms, &tmpoffset, &dirty_ra= m_abs); This test for whether we've completed a host page is pretty awkward. I think it would be cleaner to have an inner loop / helper function that completes sending an entire host page (whether requested or scanned), before allowing the outer loop to even come back to here to reconsider the queue. > } > - if (offset >=3D block->used_length) { > - offset =3D 0; > - block =3D QTAILQ_NEXT(block, next); > - if (!block) { > - block =3D QTAILQ_FIRST(&ram_list.blocks); > - complete_round =3D true; > - ram_bulk_stage =3D false; > + > + if (tmpblock) { > + /* We've got a block from the postcopy queue */ > + trace_ram_find_and_save_block_postcopy(tmpblock->idstr, > + (uint64_t)tmpoffset, > + (uint64_t)dirty_ram_a= bs); > + /* > + * 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(dirty_ram_abs)) { > + trace_ram_find_and_save_block_postcopy_not_dirty( > + tmpblock->idstr, (uint64_t)tmpoffset, > + (uint64_t)dirty_ram_abs, > + test_bit(dirty_ram_abs >> TARGET_PAGE_BITS, ms->sent= map)); > + > + continue; > } > + /* > + * As soon as we start servicing pages out of order, then we= have > + * to kill the bulk stage, since the bulk stage assumes > + * in (migration_bitmap_find_and_reset_dirty) that every pag= e is > + * dirty, that's no longer true. > + */ > + ram_bulk_stage =3D false; > + /* > + * We mustn't change block/offset unless it's to a valid one > + * otherwise we can go down some of the exit cases in the no= rmal > + * path. > + */ > + block =3D tmpblock; > + offset =3D tmpoffset; > + last_was_from_queue =3D true; Hrm. So now block can change during the execution of ram_save_block(), which really suggests that splitting by block is no longer a sensible subdivision of the loop surrounding ram_save_block. I think it would make more sense to replace that entire surrounding loop, so that the logic is essentially while (not finished) { if (something is queued) { send that } else { scan for an unsent block and offset send that instead } > } else { > - bytes_sent =3D ram_save_page(f, block, offset, last_stage); > - > - /* if page is unmodified, continue to the next */ > - if (bytes_sent > 0) { > - MigrationState *ms =3D migrate_get_current(); > - if (ms->sentmap) { > - set_bit(dirty_ram_abs >> TARGET_PAGE_BITS, ms->sentm= ap); > + MemoryRegion *mr; > + /* priority queue empty, so just search for something dirty = */ > + mr =3D block->mr; > + offset =3D migration_bitmap_find_and_reset_dirty(mr, offset, > + &dirty_ram_ab= s); > + if (complete_round && block =3D=3D last_seen_block && > + offset >=3D last_offset) { > + break; > + } > + if (offset >=3D block->used_length) { > + offset =3D 0; > + block =3D QTAILQ_NEXT(block, next); > + if (!block) { > + block =3D QTAILQ_FIRST(&ram_list.blocks); > + complete_round =3D true; > + ram_bulk_stage =3D false; > } > + continue; /* pick an offset in the new block */ > + } > + last_was_from_queue =3D false; > + } > =20 > - last_sent_block =3D block; > - break; > + /* We have a page to send, so send it */ > + bytes_sent =3D ram_save_page(f, block, offset, last_stage); > + > + /* if page is unmodified, continue to the next */ > + if (bytes_sent > 0) { > + if (ms->sentmap) { > + set_bit(dirty_ram_abs >> TARGET_PAGE_BITS, ms->sentmap); > } > + > + last_sent_block =3D block; > + break; > } > } > last_seen_block =3D block; > @@ -865,6 +968,7 @@ static void reset_ram_globals(void) > last_offset =3D 0; > last_version =3D ram_list.version; > ram_bulk_stage =3D true; > + last_was_from_queue =3D false; > } > =20 > #define MAX_WAIT 50 /* ms, half buffered_file limit */ > diff --git a/trace-events b/trace-events > index 8a0d70d..781cf5c 100644 > --- a/trace-events > +++ b/trace-events > @@ -1217,6 +1217,8 @@ qemu_file_fclose(void) "" > migration_bitmap_sync_start(void) "" > migration_bitmap_sync_end(uint64_t dirty_pages) "dirty_pages %" PRIu64"" > migration_throttle(void) "" > +ram_find_and_save_block_postcopy(const char *block_name, uint64_t tmp_of= fset, uint64_t ram_addr) "%s/%" PRIx64 " ram_addr=3D%" PRIx64 > +ram_find_and_save_block_postcopy_not_dirty(const char *block_name, uint6= 4_t tmp_offset, uint64_t ram_addr, int sent) "%s/%" PRIx64 " ram_addr=3D%" = PRIx64 " (sent=3D%d)" > ram_postcopy_send_discard_bitmap(void) "" > ram_save_queue_pages(const char *rbname, size_t start, size_t len) "%s: = start: %zx len: %zx" > =20 --=20 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 --oWEYV0WmY9Kcrzh0 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVEMi7AAoJEGw4ysog2bOSUiMP/2j/4p1ZKY6QeS/5+e5mfZat 6V6YOLnw1PKVmMDWCKMGICR/9AyClO3oWmPEhqRRdXqCT7Wt9OfYy+BCzEdLliRB mo8sjGtXs82pYzEFl00jeJxAyGQKdXwb0YfCuOoTe+NhaNgOifsvYFVrczNO7XUl nZuzxWmiwhLPWBJj16JbP25zXUkKWxZWCmCWyKMiP3BJbsLLxFjBQ3h3zXuIlAt7 gZTDfK8eFnzXrmiKOoaW7Yle0iQBwsc2bjKYVR0edFrTdw869JG/Z8ciGWYsBOOj MBXeabcqTRE5Kc6/RvfthEps9WNZ/B8sePgaAIEmV4w48SxQoEYSSI+3diiYqFXm Mil/W+XeCoZz3CvhLAGg81aJBinBqw43aJYcz4a2eSKyD2PMfjSE53OaHvT3ttAX b0S1u+Pz9SA/9NDcKJDTqBAMlcMVTs8DTDwL7oYyLMtsOp7DhGNTZetMbf6c3J5V CxWhu6ALUsyqxpbBnIBfoU8oD0fDH7zZl6FDR+f3IfJtdMYXkOGj2Ih7ANUTHpzP G4SuyQMRI2iBASy4pjC9+QJnS3KkYawLQVpYimTej9Ug665rW39nIBxWZCwdTmZL FRvY5yJLV3m6H58rdWw8Qx5PUHDmYHtVroepVNi6M+869s0Q85DB24yUSuu4BnIg LHLDYdUT0NVXzFmfHVeV =6Mou -----END PGP SIGNATURE----- --oWEYV0WmY9Kcrzh0--