From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44532) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gSbp6-0001Je-4c for qemu-devel@nongnu.org; Fri, 30 Nov 2018 00:58:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gSboG-0006RB-H6 for qemu-devel@nongnu.org; Fri, 30 Nov 2018 00:57:55 -0500 Received: from mx1.redhat.com ([209.132.183.28]:41398) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1gSboG-0006Qd-B1 for qemu-devel@nongnu.org; Fri, 30 Nov 2018 00:57:52 -0500 Date: Fri, 30 Nov 2018 13:57:36 +0800 From: Peter Xu Message-ID: <20181130055736.GA15865@xz-x1> References: <1542276484-25508-1-git-send-email-wei.w.wang@intel.com> <1542276484-25508-6-git-send-email-wei.w.wang@intel.com> <20181127073802.GC3205@xz-x1> <5BFD1BA4.5040202@intel.com> <20181128052655.GC12839@xz-x1> <5BFE596B.1080807@intel.com> <20181128093220.GF12839@xz-x1> <5BFF5FC9.2020402@intel.com> <20181129051014.GC29246@xz-x1> <5C00C52F.20606@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <5C00C52F.20606@intel.com> Subject: Re: [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wei Wang Cc: qemu-devel@nongnu.org, virtio-dev@lists.oasis-open.org, mst@redhat.com, quintela@redhat.com, dgilbert@redhat.com, pbonzini@redhat.com, liliang.opensource@gmail.com, nilal@redhat.com, riel@redhat.com On Fri, Nov 30, 2018 at 01:05:51PM +0800, Wei Wang wrote: > On 11/29/2018 01:10 PM, Peter Xu wrote: > > On Thu, Nov 29, 2018 at 11:40:57AM +0800, Wei Wang wrote: > > I think this precopy notifier callchain is expected to be used only for > > the precopy mode. Postcopy has its dedicated notifier callchain that > > users could use. > > > > How about changing the migrate_postcopy() check to "ms->start_postcopy": > > > > bool migration_postcopy_start(void) > > { > > MigrationState *s; > > > > s = migrate_get_current(); > > > > return atomic_read(&s->start_postcopy); > > } > > > > > > static void precopy_notify(PrecopyNotifyReason reason) > > { > > if (migration_postcopy_start()) > > return; > > > > notifier_list_notify(&precopy_notifier_list, &reason); > > } > > > > If postcopy started with precopy, the precopy optimization feature > > could still be used until it switches to the postcopy mode. > > I'm not sure we can use start_postcopy. It's a variable being set in > > the QMP handler but it does not mean postcopy has started. I'm afraid > > there can be race where it's still precopy but the variable is set so > > event could be missed... > > Peter, > I just found that migration_bitmap_sync is also called by > ram_postcopy_send_discard_bitmap(). > But we don't expect notifier_list_notify to be called in the postcopy mode. > > So, probably we still need the start_postcopy check in notifier_list_notify > though we have > the COMPLETE notifier. I still don't think using start_postcopy is a good idea, as explained in my previous reply. Maybe move the notify()s outside migration_bitmap_sync() but only at the call sites in precopy? Say these three: ram_init_bitmaps[3508] migration_bitmap_sync(rs); ram_save_complete[3731] migration_bitmap_sync(rs); ram_save_pending[3776] migration_bitmap_sync(rs); Or you can introduce migration_bitmap_sync_precopy() and let precopy code call that one instead. PS. I'm a bit unsure on why we need to sync bitmap in ram_init_bitmaps. I feel like it can be removed... Regards, -- Peter Xu