From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47609) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZEwMM-0006bn-Va for qemu-devel@nongnu.org; Tue, 14 Jul 2015 05:18:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZEwMH-00087f-QA for qemu-devel@nongnu.org; Tue, 14 Jul 2015 05:18:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54241) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZEwMH-00087W-Jd for qemu-devel@nongnu.org; Tue, 14 Jul 2015 05:18:37 -0400 From: Juan Quintela In-Reply-To: <1434450415-11339-32-git-send-email-dgilbert@redhat.com> (David Alan Gilbert's message of "Tue, 16 Jun 2015 11:26:44 +0100") References: <1434450415-11339-1-git-send-email-dgilbert@redhat.com> <1434450415-11339-32-git-send-email-dgilbert@redhat.com> Date: Tue, 14 Jul 2015 11:18:33 +0200 Message-ID: <87egkbgkra.fsf@neno.neno> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v7 31/42] Page request: Process incoming page request Reply-To: quintela@redhat.com 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, liang.z.li@intel.com, qemu-devel@nongnu.org, luis@cs.umu.se, amit.shah@redhat.com, pbonzini@redhat.com, david@gibson.dropbear.id.au "Dr. David Alan Gilbert (git)" wrote: > From: "Dr. David Alan Gilbert" > > On receiving MIG_RPCOMM_REQ_PAGES look up the address and > queue the page. > > Signed-off-by: Dr. David Alan Gilbert > migrate_fd_cleanup_src_rp(s); > > + /* This queue generally should be empty - but in the case of a failed > + * migration might have some droppings in. > + */ > + struct MigrationSrcPageRequest *mspr, *next_mspr; > + QSIMPLEQ_FOREACH_SAFE(mspr, &s->src_page_requests, next_req, next_mspr) { > + QSIMPLEQ_REMOVE_HEAD(&s->src_page_requests, next_req); How nice of QSIMPLEQ. To remove elements you don't use mspr.... > + g_free(mspr); > + } > + > if (s->file) { > trace_migrate_fd_cleanup(); > qemu_mutex_unlock_iothread(); > @@ -713,6 +729,8 @@ MigrationState *migrate_init(const MigrationParams *params) > s->state = MIGRATION_STATUS_SETUP; > trace_migrate_set_state(MIGRATION_STATUS_SETUP); > > + QSIMPLEQ_INIT(&s->src_page_requests); > + > s->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > return s; > } > @@ -976,7 +994,25 @@ static void source_return_path_bad(MigrationState *s) > static void migrate_handle_rp_req_pages(MigrationState *ms, const char* rbname, > ram_addr_t start, ram_addr_t len) > { > + long our_host_ps = getpagesize(); > + > trace_migrate_handle_rp_req_pages(rbname, start, len); > + > + /* > + * Since we currently insist on matching page sizes, just sanity check > + * we're being asked for whole host pages. > + */ > + if (start & (our_host_ps-1) || > + (len & (our_host_ps-1))) { I don't know if creating a macro is a good idea? #define HOST_ALIGN_CHECK(addr) (addr & (getpagesize()-1)) ??? Don't me wave a macro for this in qemu? > index f7d957e..da3e9ea 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -924,6 +924,69 @@ static int ram_save_compressed_page(QEMUFile *f, RAMBlock *block, > } > > /** > + * Queue the pages for transmission, e.g. a request from postcopy destination > + * ms: MigrationStatus in which the queue is held > + * rbname: The RAMBlock the request is for - may be NULL (to mean reuse last) > + * start: Offset from the start of the RAMBlock > + * len: Length (in bytes) to send > + * Return: 0 on success > + */ > +int ram_save_queue_pages(MigrationState *ms, const char *rbname, > + ram_addr_t start, ram_addr_t len) > +{ > + RAMBlock *ramblock; > + > + rcu_read_lock(); > + if (!rbname) { > + /* Reuse last RAMBlock */ > + ramblock = ms->last_req_rb; > + > + if (!ramblock) { > + /* > + * Shouldn't happen, we can't reuse the last RAMBlock if > + * it's the 1st request. > + */ > + error_report("ram_save_queue_pages no previous block"); > + goto err; > + } > + } else { > + ramblock = ram_find_block(rbname); > + > + if (!ramblock) { > + /* We shouldn't be asked for a non-existent RAMBlock */ > + error_report("ram_save_queue_pages no block '%s'", rbname); > + goto err; > + } Here? > + } > + trace_ram_save_queue_pages(ramblock->idstr, start, len); > + if (start+len > ramblock->used_length) { > + error_report("%s request overrun start=%zx len=%zx blocklen=%zx", > + __func__, start, len, ramblock->used_length); > + goto err; > + } > + > + struct MigrationSrcPageRequest *new_entry = > + g_malloc0(sizeof(struct MigrationSrcPageRequest)); > + new_entry->rb = ramblock; > + new_entry->offset = start; > + new_entry->len = len; > + ms->last_req_rb = ramblock; Can we move this line to the else? > + > + qemu_mutex_lock(&ms->src_page_req_mutex); > + memory_region_ref(ramblock->mr); I haven't looked further in the patch series yet, but I can't see on this patch a memory_region_unref .... Don't we need it? > + QSIMPLEQ_INSERT_TAIL(&ms->src_page_requests, new_entry, next_req); > + qemu_mutex_unlock(&ms->src_page_req_mutex); > + rcu_read_unlock(); Of everything that we have inside the rcu_read_lock() .... Is there anything else that the memory_region_ref() that needs rcu? Would not be possible to do the memory reference before asking for the mutex? Once here, do we care about calling malloc with the rcu set? or could we just call malloc at the beggining of the function and free it in case that it is not needed on err? Thanks, Juan.