From: Wei Wang <wei.w.wang@intel.com> To: Peter Xu <peterx@redhat.com> 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 Subject: Re: [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy Date: Thu, 29 Nov 2018 14:30:27 +0800 [thread overview] Message-ID: <5BFF8783.4080201@intel.com> (raw) In-Reply-To: <20181129051014.GC29246@xz-x1> On 11/29/2018 01:10 PM, Peter Xu wrote: > On Thu, Nov 29, 2018 at 11:40:57AM +0800, Wei Wang wrote: >> On 11/28/2018 05:32 PM, Peter Xu wrote: > 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... > > IMHO the problem is not that complicated. How about this proposal: > > [1] > > typedef enum PrecopyNotifyReason { > PRECOPY_NOTIFY_RAM_START, > PRECOPY_NOTIFY_RAM_BEFORE_SYNC, > PRECOPY_NOTIFY_RAM_AFTER_SYNC, > PRECOPY_NOTIFY_COMPLETE, > PRECOPY_NOTIFY_RAM_CLEANUP, > }; > > The first three keep the same as your old ones. Notify RAM_CLEANUP in > ram_save_cleanup() to make sure it'll always be cleaned up (the same > as PRECOPY_END, just another name). Notify COMPLETE in > qemu_savevm_state_complete_precopy() to show that precopy is > completed. Meanwhile on balloon side you should stop the hinting for > either RAM_CLEANUP or COMPLETE event. Then either: > > - precopy is switching to postcopy, or > - precopy completed, or > - precopy failed/cancelled > > You should always get at least a notification to stop the balloon. > Though you could also get one RAM_CLEANUP after one COMPLETE, but > the balloon should easily handle it (stop the hinting twice). Sounds better, thanks. > > Here maybe you can even remove the "RAM_" in both RAM_START and > RAM_CLEANUP if we're going to have COMPLETE since after all it'll be > not only limited to RAM. > > Another suggestion is that you can add an Error into the notify hooks, > please refer to the postcopy one: > > int postcopy_notify(enum PostcopyNotifyReason reason, Error **errp); > > So the hook functions have a way to even stop the migration (though > for balloon hinting it'll be always optional so no error should be > reported...), then the two interfaces are matched. Yes, I removed that "errp" as it's not needed by the balloon usage. But no problem, I can add it back if no objections from others. Best, Wei
WARNING: multiple messages have this Message-ID (diff)
From: Wei Wang <wei.w.wang@intel.com> To: Peter Xu <peterx@redhat.com> 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 Subject: [virtio-dev] Re: [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy Date: Thu, 29 Nov 2018 14:30:27 +0800 [thread overview] Message-ID: <5BFF8783.4080201@intel.com> (raw) In-Reply-To: <20181129051014.GC29246@xz-x1> On 11/29/2018 01:10 PM, Peter Xu wrote: > On Thu, Nov 29, 2018 at 11:40:57AM +0800, Wei Wang wrote: >> On 11/28/2018 05:32 PM, Peter Xu wrote: > 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... > > IMHO the problem is not that complicated. How about this proposal: > > [1] > > typedef enum PrecopyNotifyReason { > PRECOPY_NOTIFY_RAM_START, > PRECOPY_NOTIFY_RAM_BEFORE_SYNC, > PRECOPY_NOTIFY_RAM_AFTER_SYNC, > PRECOPY_NOTIFY_COMPLETE, > PRECOPY_NOTIFY_RAM_CLEANUP, > }; > > The first three keep the same as your old ones. Notify RAM_CLEANUP in > ram_save_cleanup() to make sure it'll always be cleaned up (the same > as PRECOPY_END, just another name). Notify COMPLETE in > qemu_savevm_state_complete_precopy() to show that precopy is > completed. Meanwhile on balloon side you should stop the hinting for > either RAM_CLEANUP or COMPLETE event. Then either: > > - precopy is switching to postcopy, or > - precopy completed, or > - precopy failed/cancelled > > You should always get at least a notification to stop the balloon. > Though you could also get one RAM_CLEANUP after one COMPLETE, but > the balloon should easily handle it (stop the hinting twice). Sounds better, thanks. > > Here maybe you can even remove the "RAM_" in both RAM_START and > RAM_CLEANUP if we're going to have COMPLETE since after all it'll be > not only limited to RAM. > > Another suggestion is that you can add an Error into the notify hooks, > please refer to the postcopy one: > > int postcopy_notify(enum PostcopyNotifyReason reason, Error **errp); > > So the hook functions have a way to even stop the migration (though > for balloon hinting it'll be always optional so no error should be > reported...), then the two interfaces are matched. Yes, I removed that "errp" as it's not needed by the balloon usage. But no problem, I can add it back if no objections from others. Best, Wei --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
next prev parent reply other threads:[~2018-11-29 6:25 UTC|newest] Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-11-15 10:07 [Qemu-devel] [PATCH v9 0/8] virtio-balloon: free page hint support Wei Wang 2018-11-15 10:07 ` [virtio-dev] " Wei Wang 2018-11-15 10:07 ` [Qemu-devel] [PATCH v9 1/8] bitmap: fix bitmap_count_one Wei Wang 2018-11-15 10:07 ` [virtio-dev] " Wei Wang 2018-11-15 10:07 ` [Qemu-devel] [PATCH v9 2/8] bitmap: bitmap_count_one_with_offset Wei Wang 2018-11-15 10:07 ` [virtio-dev] " Wei Wang 2018-11-15 10:07 ` [Qemu-devel] [PATCH v9 3/8] migration: use bitmap_mutex in migration_bitmap_clear_dirty Wei Wang 2018-11-15 10:07 ` [virtio-dev] " Wei Wang 2018-11-27 5:40 ` [Qemu-devel] " Peter Xu 2018-11-27 6:02 ` Wei Wang 2018-11-27 6:02 ` [virtio-dev] " Wei Wang 2018-11-27 6:12 ` [Qemu-devel] " Wei Wang 2018-11-27 6:12 ` Wei Wang 2018-11-27 7:41 ` [Qemu-devel] " Peter Xu 2018-11-27 10:17 ` Wei Wang 2018-11-27 10:17 ` Wei Wang 2018-11-15 10:08 ` [Qemu-devel] [PATCH v9 4/8] migration: API to clear bits of guest free pages from the dirty bitmap Wei Wang 2018-11-15 10:08 ` [virtio-dev] " Wei Wang 2018-11-27 6:06 ` [Qemu-devel] " Peter Xu 2018-11-27 6:52 ` Wei Wang 2018-11-27 6:52 ` [virtio-dev] " Wei Wang 2018-11-27 7:43 ` [Qemu-devel] " Peter Xu 2018-11-15 10:08 ` [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy Wei Wang 2018-11-15 10:08 ` [virtio-dev] " Wei Wang 2018-11-27 7:38 ` [Qemu-devel] " Peter Xu 2018-11-27 10:25 ` Wei Wang 2018-11-27 10:25 ` [virtio-dev] " Wei Wang 2018-11-28 5:26 ` [Qemu-devel] " Peter Xu 2018-11-28 9:01 ` Wei Wang 2018-11-28 9:01 ` [virtio-dev] " Wei Wang 2018-11-28 9:32 ` [Qemu-devel] " Peter Xu 2018-11-29 3:40 ` Wei Wang 2018-11-29 3:40 ` [virtio-dev] " Wei Wang 2018-11-29 5:10 ` [Qemu-devel] " Peter Xu 2018-11-29 5:47 ` Peter Xu 2018-11-29 6:30 ` Wei Wang [this message] 2018-11-29 6:30 ` [virtio-dev] " Wei Wang 2018-11-30 5:05 ` [Qemu-devel] " Wei Wang 2018-11-30 5:05 ` [virtio-dev] " Wei Wang 2018-11-30 5:57 ` [Qemu-devel] " Peter Xu 2018-11-30 7:09 ` Wei Wang 2018-11-30 7:09 ` [virtio-dev] " Wei Wang 2018-11-15 10:08 ` [Qemu-devel] [PATCH v9 6/8] migration/ram.c: add a function to disable the bulk stage Wei Wang 2018-11-15 10:08 ` [virtio-dev] " Wei Wang 2018-11-15 10:08 ` [Qemu-devel] [PATCH v9 7/8] migration: move migrate_postcopy() to include/migration/misc.h Wei Wang 2018-11-15 10:08 ` [virtio-dev] " Wei Wang 2018-11-15 10:08 ` [Qemu-devel] [PATCH v9 8/8] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT Wei Wang 2018-11-15 10:08 ` [virtio-dev] " Wei Wang 2018-11-15 18:50 ` [Qemu-devel] [PATCH v9 0/8] virtio-balloon: free page hint support no-reply 2018-11-16 1:38 ` Wei Wang 2018-11-16 1:38 ` [virtio-dev] " Wei Wang 2018-11-27 3:11 ` Wei Wang 2018-11-27 3:11 ` [virtio-dev] " Wei Wang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=5BFF8783.4080201@intel.com \ --to=wei.w.wang@intel.com \ --cc=dgilbert@redhat.com \ --cc=liliang.opensource@gmail.com \ --cc=mst@redhat.com \ --cc=nilal@redhat.com \ --cc=pbonzini@redhat.com \ --cc=peterx@redhat.com \ --cc=qemu-devel@nongnu.org \ --cc=quintela@redhat.com \ --cc=riel@redhat.com \ --cc=virtio-dev@lists.oasis-open.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.