| Message ID | 1542276484-25508-6-git-send-email-wei.w.wang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-balloon: free page hint support | expand |
On Thu, Nov 15, 2018 at 06:08:01PM +0800, Wei Wang wrote: > This patch adds a notifier chain for the memory precopy. This enables various > precopy optimizations to be invoked at specific places. > > Signed-off-by: Wei Wang <wei.w.wang@intel.com> > CC: Dr. David Alan Gilbert <dgilbert@redhat.com> > CC: Juan Quintela <quintela@redhat.com> > CC: Michael S. Tsirkin <mst@redhat.com> > CC: Peter Xu <peterx@redhat.com> > --- > include/migration/misc.h | 12 ++++++++++++ > migration/ram.c | 31 +++++++++++++++++++++++++++++++ > vl.c | 1 + > 3 files changed, 44 insertions(+) > > diff --git a/include/migration/misc.h b/include/migration/misc.h > index 113320e..0bac623 100644 > --- a/include/migration/misc.h > +++ b/include/migration/misc.h > @@ -19,6 +19,18 @@ > > /* migration/ram.c */ > > +typedef enum PrecopyNotifyReason { > + PRECOPY_NOTIFY_ERR = 0, > + PRECOPY_NOTIFY_START_ITERATION = 1, > + PRECOPY_NOTIFY_BEFORE_SYNC_BITMAP = 2, > + PRECOPY_NOTIFY_AFTER_SYNC_BITMAP = 3, > + PRECOPY_NOTIFY_MAX = 4, It would be nice to add some comments for each of the notify reason. E.g., from the name PRECOPY_NOTIFY_START_ITERATION seems more like a hook at the start of each iteration but according to [1] it should be at the start of migration rather than each iteration (or when migration restarts, though I'm not sure whether we really have this yet). > +} PrecopyNotifyReason; > + > +void precopy_infrastructure_init(void); > +void precopy_add_notifier(Notifier *n); > +void precopy_remove_notifier(Notifier *n); > + > void ram_mig_init(void); > void qemu_guest_free_page_hint(void *addr, size_t len); > > diff --git a/migration/ram.c b/migration/ram.c > index 229b791..65b1223 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -292,6 +292,8 @@ struct RAMState { > bool ram_bulk_stage; > /* How many times we have dirty too many pages */ > int dirty_rate_high_cnt; > + /* ram save states used for notifiers */ > + int ram_save_state; This can be removed? > /* these variables are used for bitmap sync */ > /* last time we did a full bitmap_sync */ > int64_t time_last_bitmap_sync; > @@ -328,6 +330,28 @@ typedef struct RAMState RAMState; > > static RAMState *ram_state; > > +static NotifierList precopy_notifier_list; > + > +void precopy_infrastructure_init(void) > +{ > + notifier_list_init(&precopy_notifier_list); > +} > + > +void precopy_add_notifier(Notifier *n) > +{ > + notifier_list_add(&precopy_notifier_list, n); > +} > + > +void precopy_remove_notifier(Notifier *n) > +{ > + notifier_remove(n); > +} > + > +static void precopy_notify(PrecopyNotifyReason reason) > +{ > + notifier_list_notify(&precopy_notifier_list, &reason); > +} > + > uint64_t ram_bytes_remaining(void) > { > return ram_state ? (ram_state->migration_dirty_pages * TARGET_PAGE_SIZE) : > @@ -1642,6 +1666,8 @@ static void migration_bitmap_sync(RAMState *rs) > int64_t end_time; > uint64_t bytes_xfer_now; > > + precopy_notify(PRECOPY_NOTIFY_BEFORE_SYNC_BITMAP); > + > ram_counters.dirty_sync_count++; > > if (!rs->time_last_bitmap_sync) { > @@ -1699,6 +1725,8 @@ static void migration_bitmap_sync(RAMState *rs) > if (migrate_use_events()) { > qapi_event_send_migration_pass(ram_counters.dirty_sync_count); > } > + > + precopy_notify(PRECOPY_NOTIFY_AFTER_SYNC_BITMAP); > } > > /** > @@ -2555,6 +2583,8 @@ static void ram_state_reset(RAMState *rs) > rs->last_page = 0; > rs->last_version = ram_list.version; > rs->ram_bulk_stage = true; > + > + precopy_notify(PRECOPY_NOTIFY_START_ITERATION); [1] > } > > #define MAX_WAIT 50 /* ms, half buffered_file limit */ > @@ -3324,6 +3354,7 @@ out: > > ret = qemu_file_get_error(f); > if (ret < 0) { > + precopy_notify(PRECOPY_NOTIFY_ERR); Could you show me which function is this line in? Line 3324 here is ram_save_complete(), but I cannot find this exact place. Another thing to mention about the "reasons" (though I see it more like "events"): have you thought about adding a PRECOPY_NOTIFY_END? It might help in some cases: - then you don't need to trickily export the migrate_postcopy() since you'll notify that before postcopy starts - you'll have a solid point that you'll 100% guarantee that we'll stop the free page hinting and don't need to worry about whether there is chance the hinting will be running without an end [2]. Regarding [2] above: now the series only stops the hinting when PRECOPY_NOTIFY_BEFORE_SYNC_BITMAP is notified. Could there be a case that it's missing? E.g., what if we cancel/fail a migration during precopy? Have you tried it? Regards,
On 11/27/2018 03:38 PM, Peter Xu wrote: > On Thu, Nov 15, 2018 at 06:08:01PM +0800, Wei Wang wrote: >> >> +typedef enum PrecopyNotifyReason { >> + PRECOPY_NOTIFY_ERR = 0, >> + PRECOPY_NOTIFY_START_ITERATION = 1, >> + PRECOPY_NOTIFY_BEFORE_SYNC_BITMAP = 2, >> + PRECOPY_NOTIFY_AFTER_SYNC_BITMAP = 3, >> + PRECOPY_NOTIFY_MAX = 4, > It would be nice to add some comments for each of the notify reason. > E.g., from the name PRECOPY_NOTIFY_START_ITERATION seems more like a > hook at the start of each iteration but according to [1] it should be > at the start of migration rather than each iteration (or when > migration restarts, though I'm not sure whether we really have this > yet). OK. I think It would be better if the name itself could be straightforward. Probably we could change PRECOPY_NOTIFY_START_ITERATION to PRECOPY_NOTIFY_START_MIGRATION. >> +} PrecopyNotifyReason; >> + >> +void precopy_infrastructure_init(void); >> +void precopy_add_notifier(Notifier *n); >> +void precopy_remove_notifier(Notifier *n); >> + >> void ram_mig_init(void); >> void qemu_guest_free_page_hint(void *addr, size_t len); >> >> diff --git a/migration/ram.c b/migration/ram.c >> index 229b791..65b1223 100644 >> --- a/migration/ram.c >> +++ b/migration/ram.c >> @@ -292,6 +292,8 @@ struct RAMState { >> bool ram_bulk_stage; >> /* How many times we have dirty too many pages */ >> int dirty_rate_high_cnt; >> + /* ram save states used for notifiers */ >> + int ram_save_state; > This can be removed? Yes, thanks. > >> /* these variables are used for bitmap sync */ >> /* last time we did a full bitmap_sync */ >> int64_t time_last_bitmap_sync; >> @@ -328,6 +330,28 @@ typedef struct RAMState RAMState; >> >> static RAMState *ram_state; >> >> +static NotifierList precopy_notifier_list; >> + >> +void precopy_infrastructure_init(void) >> +{ >> + notifier_list_init(&precopy_notifier_list); >> +} >> + >> +void precopy_add_notifier(Notifier *n) >> +{ >> + notifier_list_add(&precopy_notifier_list, n); >> +} >> + >> +void precopy_remove_notifier(Notifier *n) >> +{ >> + notifier_remove(n); >> +} >> + >> +static void precopy_notify(PrecopyNotifyReason reason) >> +{ >> + notifier_list_notify(&precopy_notifier_list, &reason); >> +} >> + >> uint64_t ram_bytes_remaining(void) >> { >> return ram_state ? (ram_state->migration_dirty_pages * TARGET_PAGE_SIZE) : >> @@ -1642,6 +1666,8 @@ static void migration_bitmap_sync(RAMState *rs) >> int64_t end_time; >> uint64_t bytes_xfer_now; >> >> + precopy_notify(PRECOPY_NOTIFY_BEFORE_SYNC_BITMAP); >> + >> ram_counters.dirty_sync_count++; >> >> if (!rs->time_last_bitmap_sync) { >> @@ -1699,6 +1725,8 @@ static void migration_bitmap_sync(RAMState *rs) >> if (migrate_use_events()) { >> qapi_event_send_migration_pass(ram_counters.dirty_sync_count); >> } >> + >> + precopy_notify(PRECOPY_NOTIFY_AFTER_SYNC_BITMAP); >> } >> >> /** >> @@ -2555,6 +2583,8 @@ static void ram_state_reset(RAMState *rs) >> rs->last_page = 0; >> rs->last_version = ram_list.version; >> rs->ram_bulk_stage = true; >> + >> + precopy_notify(PRECOPY_NOTIFY_START_ITERATION); > [1] > >> } >> >> #define MAX_WAIT 50 /* ms, half buffered_file limit */ >> @@ -3324,6 +3354,7 @@ out: >> >> ret = qemu_file_get_error(f); >> if (ret < 0) { >> + precopy_notify(PRECOPY_NOTIFY_ERR); > Could you show me which function is this line in? > > Line 3324 here is ram_save_complete(), but I cannot find this exact > place. Sure, it's in ram_save_iterate(): ... out: qemu_put_be64(f, RAM_SAVE_FLAG_EOS); qemu_fflush(f); ram_counters.transferred += 8; ret = qemu_file_get_error(f); if (ret < 0) { + precopy_notify(PRECOPY_NOTIFY_ERR); return ret; } return done; } > > Another thing to mention about the "reasons" (though I see it more > like "events"): have you thought about adding a PRECOPY_NOTIFY_END? > It might help in some cases: > > - then you don't need to trickily export the migrate_postcopy() > since you'll notify that before postcopy starts I'm thinking probably we don't need to export migrate_postcopy even now. It's more like a sanity check, and not needed because now we have the notifier registered to the precopy specific callchain, which has ensured that it is invoked via precopy. > - you'll have a solid point that you'll 100% guarantee that we'll > stop the free page hinting and don't need to worry about whether > there is chance the hinting will be running without an end [2]. Thanks, I think it makes sense. Plan to add PRECOPY_NOTIFY_END in ram_save_complete. > > Regarding [2] above: now the series only stops the hinting when > PRECOPY_NOTIFY_BEFORE_SYNC_BITMAP is notified. Could there be a case > that it's missing? E.g., what if we cancel/fail a migration during > precopy? Have you tried it? > I think it has been handled by the above PRECOPY_NOTIFY_ERR Best, Wei
On Tue, Nov 27, 2018 at 06:25:40PM +0800, Wei Wang wrote: > On 11/27/2018 03:38 PM, Peter Xu wrote: > > On Thu, Nov 15, 2018 at 06:08:01PM +0800, Wei Wang wrote: > > > +typedef enum PrecopyNotifyReason { > > > + PRECOPY_NOTIFY_ERR = 0, > > > + PRECOPY_NOTIFY_START_ITERATION = 1, > > > + PRECOPY_NOTIFY_BEFORE_SYNC_BITMAP = 2, > > > + PRECOPY_NOTIFY_AFTER_SYNC_BITMAP = 3, > > > + PRECOPY_NOTIFY_MAX = 4, > > It would be nice to add some comments for each of the notify reason. > > E.g., from the name PRECOPY_NOTIFY_START_ITERATION seems more like a > > hook at the start of each iteration but according to [1] it should be > > at the start of migration rather than each iteration (or when > > migration restarts, though I'm not sure whether we really have this > > yet). > > OK. I think It would be better if the name itself could be straightforward. > Probably we could change PRECOPY_NOTIFY_START_ITERATION to > PRECOPY_NOTIFY_START_MIGRATION. Sounds good. > > > > > +} PrecopyNotifyReason; > > > + > > > +void precopy_infrastructure_init(void); > > > +void precopy_add_notifier(Notifier *n); > > > +void precopy_remove_notifier(Notifier *n); > > > + > > > void ram_mig_init(void); > > > void qemu_guest_free_page_hint(void *addr, size_t len); > > > diff --git a/migration/ram.c b/migration/ram.c > > > index 229b791..65b1223 100644 > > > --- a/migration/ram.c > > > +++ b/migration/ram.c > > > @@ -292,6 +292,8 @@ struct RAMState { > > > bool ram_bulk_stage; > > > /* How many times we have dirty too many pages */ > > > int dirty_rate_high_cnt; > > > + /* ram save states used for notifiers */ > > > + int ram_save_state; > > This can be removed? > > Yes, thanks. > > > > > > /* these variables are used for bitmap sync */ > > > /* last time we did a full bitmap_sync */ > > > int64_t time_last_bitmap_sync; > > > @@ -328,6 +330,28 @@ typedef struct RAMState RAMState; > > > static RAMState *ram_state; > > > +static NotifierList precopy_notifier_list; > > > + > > > +void precopy_infrastructure_init(void) > > > +{ > > > + notifier_list_init(&precopy_notifier_list); > > > +} > > > + > > > +void precopy_add_notifier(Notifier *n) > > > +{ > > > + notifier_list_add(&precopy_notifier_list, n); > > > +} > > > + > > > +void precopy_remove_notifier(Notifier *n) > > > +{ > > > + notifier_remove(n); > > > +} > > > + > > > +static void precopy_notify(PrecopyNotifyReason reason) > > > +{ > > > + notifier_list_notify(&precopy_notifier_list, &reason); > > > +} > > > + > > > uint64_t ram_bytes_remaining(void) > > > { > > > return ram_state ? (ram_state->migration_dirty_pages * TARGET_PAGE_SIZE) : > > > @@ -1642,6 +1666,8 @@ static void migration_bitmap_sync(RAMState *rs) > > > int64_t end_time; > > > uint64_t bytes_xfer_now; > > > + precopy_notify(PRECOPY_NOTIFY_BEFORE_SYNC_BITMAP); > > > + > > > ram_counters.dirty_sync_count++; > > > if (!rs->time_last_bitmap_sync) { > > > @@ -1699,6 +1725,8 @@ static void migration_bitmap_sync(RAMState *rs) > > > if (migrate_use_events()) { > > > qapi_event_send_migration_pass(ram_counters.dirty_sync_count); > > > } > > > + > > > + precopy_notify(PRECOPY_NOTIFY_AFTER_SYNC_BITMAP); > > > } > > > /** > > > @@ -2555,6 +2583,8 @@ static void ram_state_reset(RAMState *rs) > > > rs->last_page = 0; > > > rs->last_version = ram_list.version; > > > rs->ram_bulk_stage = true; > > > + > > > + precopy_notify(PRECOPY_NOTIFY_START_ITERATION); > > [1] > > > > > } > > > #define MAX_WAIT 50 /* ms, half buffered_file limit */ > > > @@ -3324,6 +3354,7 @@ out: > > > ret = qemu_file_get_error(f); > > > if (ret < 0) { > > > + precopy_notify(PRECOPY_NOTIFY_ERR); > > Could you show me which function is this line in? > > > > Line 3324 here is ram_save_complete(), but I cannot find this exact > > place. > > Sure, it's in ram_save_iterate(): > ... > out: > qemu_put_be64(f, RAM_SAVE_FLAG_EOS); > qemu_fflush(f); > ram_counters.transferred += 8; > > ret = qemu_file_get_error(f); > if (ret < 0) { > + precopy_notify(PRECOPY_NOTIFY_ERR); > return ret; > } > > return done; > } Ok thanks. Please just make sure you will capture all the error cases, e.g., I also see path like this (a few lines below): if (pages < 0) { qemu_file_set_error(f, pages); break; } It seems that you missed that one. I would even suggest that you capture the error with higher level. E.g., in migration_iteration_run() after qemu_savevm_state_iterate(). Or we can just check the return value of qemu_savevm_state_iterate(), which we have had ignored so far. [1] > > > > > > Another thing to mention about the "reasons" (though I see it more > > like "events"): have you thought about adding a PRECOPY_NOTIFY_END? > > It might help in some cases: > > > > - then you don't need to trickily export the migrate_postcopy() > > since you'll notify that before postcopy starts > > I'm thinking probably we don't need to export migrate_postcopy even now. > It's more like a sanity check, and not needed because now we have the > notifier registered to the precopy specific callchain, which has ensured > that > it is invoked via precopy. But postcopy will always start with precopy, no? > > > - you'll have a solid point that you'll 100% guarantee that we'll > > stop the free page hinting and don't need to worry about whether > > there is chance the hinting will be running without an end [2]. > > Thanks, I think it makes sense. Plan to add PRECOPY_NOTIFY_END in > ram_save_complete. Yeah you can. Btw, if you're mostly adding the notifies only in RAM-only codes, then you can consider add the "RAM" into the names of events too to be clear. My suggestion at [1] is precopy general, but you can still capture it at the end of ram_save_iterate, then they are RAM-only again. Please feel free to choose what fits more... > > > > > > Regarding [2] above: now the series only stops the hinting when > > PRECOPY_NOTIFY_BEFORE_SYNC_BITMAP is notified. Could there be a case > > that it's missing? E.g., what if we cancel/fail a migration during > > precopy? Have you tried it? > > > > I think it has been handled by the above PRECOPY_NOTIFY_ERR Indeed it should, as long as you're covering all the error cases. Thanks,
On 11/28/2018 01:26 PM, Peter Xu wrote: > > Ok thanks. Please just make sure you will capture all the error > cases, e.g., I also see path like this (a few lines below): > > if (pages < 0) { > qemu_file_set_error(f, pages); > break; > } > > It seems that you missed that one. I think that one should be fine. This notification is actually put at the bottom of ram_save_iterate. All the above error will bail out to the "out:" path and then go to call precopy_notify(PRECOPY_NOTIFY_ERR). > > I would even suggest that you capture the error with higher level. > E.g., in migration_iteration_run() after qemu_savevm_state_iterate(). > Or we can just check the return value of qemu_savevm_state_iterate(), > which we have had ignored so far. Not very sure about the higher level, because other SaveStateEntry may cause errors that this feature don't need to care, I think we may only need it in ram_save. > [1] > >> >>> Another thing to mention about the "reasons" (though I see it more >>> like "events"): have you thought about adding a PRECOPY_NOTIFY_END? >>> It might help in some cases: >>> >>> - then you don't need to trickily export the migrate_postcopy() >>> since you'll notify that before postcopy starts >> I'm thinking probably we don't need to export migrate_postcopy even now. >> It's more like a sanity check, and not needed because now we have the >> notifier registered to the precopy specific callchain, which has ensured >> that >> it is invoked via precopy. > But postcopy will always start with precopy, no? Yes, but I think we could add the check in precopy_notify() > >>> - you'll have a solid point that you'll 100% guarantee that we'll >>> stop the free page hinting and don't need to worry about whether >>> there is chance the hinting will be running without an end [2]. >> Thanks, I think it makes sense. Plan to add PRECOPY_NOTIFY_END in >> ram_save_complete. > Yeah you can. > > Btw, if you're mostly adding the notifies only in RAM-only codes, then > you can consider add the "RAM" into the names of events too to be > clear. Sounds good, will try and see if the name would become too long :) > > My suggestion at [1] is precopy general, but you can still capture it > at the end of ram_save_iterate, then they are RAM-only again. Please > feel free to choose what fits more... OK, thanks. Best, Wei
On Wed, Nov 28, 2018 at 05:01:31PM +0800, Wei Wang wrote: > On 11/28/2018 01:26 PM, Peter Xu wrote: > > > > Ok thanks. Please just make sure you will capture all the error > > cases, e.g., I also see path like this (a few lines below): > > > > if (pages < 0) { > > qemu_file_set_error(f, pages); > > break; > > } > > > > It seems that you missed that one. > > I think that one should be fine. This notification is actually put at the > bottom of ram_save_iterate. All the above error will bail out to the "out:" > path and then go to call precopy_notify(PRECOPY_NOTIFY_ERR). Ok, maybe I was pointing to a wrong one. :) > > > > > I would even suggest that you capture the error with higher level. > > E.g., in migration_iteration_run() after qemu_savevm_state_iterate(). > > Or we can just check the return value of qemu_savevm_state_iterate(), > > which we have had ignored so far. > > Not very sure about the higher level, because other SaveStateEntry may cause > errors that this feature don't need to care, I think we may only need it in > ram_save. So what I am worrying here are corner cases where we might forget to stop the hinting. I'm fabricating one example sequence of events: (start migration) START_MIGRATION BEFORE_SYNC AFTER_SYNC ... BEFORE_SYNC AFTER_SYNC (some SaveStateEntry failed rather than RAM, then migration_detect_error returned MIG_THR_ERR_FATAL so we need to fail the migration, however when running the previous ram_save_iterate for RAM's specific SaveStateEntry we didn't see any error so no ERROR event detected) Then it seems the hinting will last forever. Considering that now I'm not sure whether this can be done ram-only, since even if you capture ram_save_complete() and at the same time you introduce PRECOPY_END you may still miss the PRECOPY_END event since AFAIU ram_save_complete() won't be called at all in this case. Could this happen? > > > > [1] > > > > > > > > > Another thing to mention about the "reasons" (though I see it more > > > > like "events"): have you thought about adding a PRECOPY_NOTIFY_END? > > > > It might help in some cases: > > > > > > > > - then you don't need to trickily export the migrate_postcopy() > > > > since you'll notify that before postcopy starts > > > I'm thinking probably we don't need to export migrate_postcopy even now. > > > It's more like a sanity check, and not needed because now we have the > > > notifier registered to the precopy specific callchain, which has ensured > > > that > > > it is invoked via precopy. > > But postcopy will always start with precopy, no? > > Yes, but I think we could add the check in precopy_notify() I'm not sure that's good. If the notifier could potentially have other user, they might still work with postcopy, and they might expect e.g. BEFORE_SYNC to be called for every sync, even if it's at the precopy stage of a postcopy. In that sense I still feel the PRECOPY_END is better (so contantly call it at the end of precopy, no matter whether there's another postcopy afterwards). It sounds like a cleaner interface. Or you can check it in the balloon specific callback and ignore the event if postcopy is on, but then we're going backward to need to export the API so it seems meaningless. Regards,
On 11/28/2018 05:32 PM, Peter Xu wrote: > > So what I am worrying here are corner cases where we might forget to > stop the hinting. I'm fabricating one example sequence of events: > > (start migration) > START_MIGRATION > BEFORE_SYNC > AFTER_SYNC > ... > BEFORE_SYNC > AFTER_SYNC > (some SaveStateEntry failed rather than RAM, then > migration_detect_error returned MIG_THR_ERR_FATAL so we need to > fail the migration, however when running the previous > ram_save_iterate for RAM's specific SaveStateEntry we didn't see > any error so no ERROR event detected) > > Then it seems the hinting will last forever. Considering that now I'm > not sure whether this can be done ram-only, since even if you capture > ram_save_complete() and at the same time you introduce PRECOPY_END you > may still miss the PRECOPY_END event since AFAIU ram_save_complete() > won't be called at all in this case. > > Could this happen? Thanks, indeed this case could happen if we add PRECOPY_END in ram_save_complete. How about putting PRECOPY_END in ram_save_cleanup? I think it would be called in any case. I'm also thinking probably we don't need PRECOPY_ERR when we have PRECOPY_END, and what do you think of the notifier names below: +typedef enum PrecopyNotifyReason { + PRECOPY_NOTIFY_RAM_SAVE_END = 0, + PRECOPY_NOTIFY_RAM_SAVE_START = 1, + PRECOPY_NOTIFY_RAM_SAVE_BEFORE_SYNC_BITMAP = 2, + PRECOPY_NOTIFY_RAM_SAVE_AFTER_SYNC_BITMAP = 3, + PRECOPY_NOTIFY_RAM_SAVE_MAX = 4, +} PrecopyNotifyReason; > >> >>> [1] >>> >>>>> Another thing to mention about the "reasons" (though I see it more >>>>> like "events"): have you thought about adding a PRECOPY_NOTIFY_END? >>>>> It might help in some cases: >>>>> >>>>> - then you don't need to trickily export the migrate_postcopy() >>>>> since you'll notify that before postcopy starts >>>> I'm thinking probably we don't need to export migrate_postcopy even now. >>>> It's more like a sanity check, and not needed because now we have the >>>> notifier registered to the precopy specific callchain, which has ensured >>>> that >>>> it is invoked via precopy. >>> But postcopy will always start with precopy, no? >> Yes, but I think we could add the check in precopy_notify() > I'm not sure that's good. If the notifier could potentially have > other user, they might still work with postcopy, and they might expect > e.g. BEFORE_SYNC to be called for every sync, even if it's at the > precopy stage of a postcopy. 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. > In that sense I still feel the > PRECOPY_END is better (so contantly call it at the end of precopy, no > matter whether there's another postcopy afterwards). It sounds like a > cleaner interface. Probably I still haven't got the point how PRECOPY_END could help above yet. Best, Wei
On Thu, Nov 29, 2018 at 11:40:57AM +0800, Wei Wang wrote: > On 11/28/2018 05:32 PM, Peter Xu wrote: > > > > So what I am worrying here are corner cases where we might forget to > > stop the hinting. I'm fabricating one example sequence of events: > > > > (start migration) > > START_MIGRATION > > BEFORE_SYNC > > AFTER_SYNC > > ... > > BEFORE_SYNC > > AFTER_SYNC > > (some SaveStateEntry failed rather than RAM, then > > migration_detect_error returned MIG_THR_ERR_FATAL so we need to > > fail the migration, however when running the previous > > ram_save_iterate for RAM's specific SaveStateEntry we didn't see > > any error so no ERROR event detected) > > > > Then it seems the hinting will last forever. Considering that now I'm > > not sure whether this can be done ram-only, since even if you capture > > ram_save_complete() and at the same time you introduce PRECOPY_END you > > may still miss the PRECOPY_END event since AFAIU ram_save_complete() > > won't be called at all in this case. > > > > Could this happen? > > Thanks, indeed this case could happen if we add PRECOPY_END in > ram_save_complete. > > How about putting PRECOPY_END in ram_save_cleanup? > I think it would be called in any case. Sounds good. > > I'm also thinking probably we don't need PRECOPY_ERR when we have > PRECOPY_END, > and what do you think of the notifier names below: > > +typedef enum PrecopyNotifyReason { > + PRECOPY_NOTIFY_RAM_SAVE_END = 0, > + PRECOPY_NOTIFY_RAM_SAVE_START = 1, > + PRECOPY_NOTIFY_RAM_SAVE_BEFORE_SYNC_BITMAP = 2, > + PRECOPY_NOTIFY_RAM_SAVE_AFTER_SYNC_BITMAP = 3, > + PRECOPY_NOTIFY_RAM_SAVE_MAX = 4, > +} PrecopyNotifyReason; (please see below [1]...) > > > > > > > > > > > [1] > > > > > > > > > > Another thing to mention about the "reasons" (though I see it more > > > > > > like "events"): have you thought about adding a PRECOPY_NOTIFY_END? > > > > > > It might help in some cases: > > > > > > > > > > > > - then you don't need to trickily export the migrate_postcopy() > > > > > > since you'll notify that before postcopy starts > > > > > I'm thinking probably we don't need to export migrate_postcopy even now. > > > > > It's more like a sanity check, and not needed because now we have the > > > > > notifier registered to the precopy specific callchain, which has ensured > > > > > that > > > > > it is invoked via precopy. > > > > But postcopy will always start with precopy, no? > > > Yes, but I think we could add the check in precopy_notify() > > I'm not sure that's good. If the notifier could potentially have > > other user, they might still work with postcopy, and they might expect > > e.g. BEFORE_SYNC to be called for every sync, even if it's at the > > precopy stage of a postcopy. > > 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... 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). 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. > > > > > In that sense I still feel the > > PRECOPY_END is better (so contantly call it at the end of precopy, no > > matter whether there's another postcopy afterwards). It sounds like a > > cleaner interface. > > Probably I still haven't got the point how PRECOPY_END could help above yet. Please have a look at above proposal. Thanks,
On Thu, Nov 29, 2018 at 01:10:14PM +0800, 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: > > > > > > So what I am worrying here are corner cases where we might forget to > > > stop the hinting. I'm fabricating one example sequence of events: > > > > > > (start migration) > > > START_MIGRATION > > > BEFORE_SYNC > > > AFTER_SYNC > > > ... > > > BEFORE_SYNC > > > AFTER_SYNC > > > (some SaveStateEntry failed rather than RAM, then > > > migration_detect_error returned MIG_THR_ERR_FATAL so we need to > > > fail the migration, however when running the previous > > > ram_save_iterate for RAM's specific SaveStateEntry we didn't see > > > any error so no ERROR event detected) > > > > > > Then it seems the hinting will last forever. Considering that now I'm > > > not sure whether this can be done ram-only, since even if you capture > > > ram_save_complete() and at the same time you introduce PRECOPY_END you > > > may still miss the PRECOPY_END event since AFAIU ram_save_complete() > > > won't be called at all in this case. > > > > > > Could this happen? > > > > Thanks, indeed this case could happen if we add PRECOPY_END in > > ram_save_complete. > > > > How about putting PRECOPY_END in ram_save_cleanup? > > I think it would be called in any case. > > Sounds good. > > > > > I'm also thinking probably we don't need PRECOPY_ERR when we have > > PRECOPY_END, > > and what do you think of the notifier names below: > > > > +typedef enum PrecopyNotifyReason { > > + PRECOPY_NOTIFY_RAM_SAVE_END = 0, > > + PRECOPY_NOTIFY_RAM_SAVE_START = 1, > > + PRECOPY_NOTIFY_RAM_SAVE_BEFORE_SYNC_BITMAP = 2, > > + PRECOPY_NOTIFY_RAM_SAVE_AFTER_SYNC_BITMAP = 3, > > + PRECOPY_NOTIFY_RAM_SAVE_MAX = 4, > > +} PrecopyNotifyReason; > > (please see below [1]...) > > > > > > > > > > > > > > > > > [1] > > > > > > > > > > > > Another thing to mention about the "reasons" (though I see it more > > > > > > > like "events"): have you thought about adding a PRECOPY_NOTIFY_END? > > > > > > > It might help in some cases: > > > > > > > > > > > > > > - then you don't need to trickily export the migrate_postcopy() > > > > > > > since you'll notify that before postcopy starts > > > > > > I'm thinking probably we don't need to export migrate_postcopy even now. > > > > > > It's more like a sanity check, and not needed because now we have the > > > > > > notifier registered to the precopy specific callchain, which has ensured > > > > > > that > > > > > > it is invoked via precopy. > > > > > But postcopy will always start with precopy, no? > > > > Yes, but I think we could add the check in precopy_notify() > > > I'm not sure that's good. If the notifier could potentially have > > > other user, they might still work with postcopy, and they might expect > > > e.g. BEFORE_SYNC to be called for every sync, even if it's at the > > > precopy stage of a postcopy. > > > > 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... > > 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). > > 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. Oh maybe we can remove all the RAM_ prefix to make it precopy general... typedef enum PrecopyNotifyReason { PRECOPY_NOTIFY_SETUP, PRECOPY_NOTIFY_BEFORE_SYNC, PRECOPY_NOTIFY_AFTER_SYNC, PRECOPY_NOTIFY_COMPLETE, PRECOPY_NOTIFY_CLEANUP, }; Then we can just hook everything with the corresponding names: SETUP: hooks with qemu_savevm_state_setup COMPLETE: hooks with qemu_savevm_state_complete_precopy CLEANUP: hooks with qemu_savevm_state_cleanup I'm not sure whether you'll need another hook in ram_state_reset in the future but for now I don't see it necessary since I don't thnk ram_list.version would change during migration for now, so ram_state_reset should only be called during setup. > > 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. > > > > > > > > > > In that sense I still feel the > > > PRECOPY_END is better (so contantly call it at the end of precopy, no > > > matter whether there's another postcopy afterwards). It sounds like a > > > cleaner interface. > > > > Probably I still haven't got the point how PRECOPY_END could help above yet. > > Please have a look at above proposal. Thanks, > > -- > Peter Xu > Regards,
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
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. Best, Wei
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,
On 11/30/2018 01:57 PM, Peter Xu wrote: >> >> >> Or you can introduce migration_bitmap_sync_precopy() and let precopy >> code call that one instead. Agree, thanks. > PS. I'm a bit unsure on why we need to sync bitmap in ram_init_bitmaps. > I feel like it can be removed... Seems it was added in early days to fix some bug: https://git.virtualopensystems.com/dev/qemu/commit/79536f4f16934d6759a1d67f0342b4e7ceb66671?w=1 Hopefully, Juan could share some details about that. Best, Wei
diff --git a/include/migration/misc.h b/include/migration/misc.h index 113320e..0bac623 100644 --- a/include/migration/misc.h +++ b/include/migration/misc.h @@ -19,6 +19,18 @@ /* migration/ram.c */ +typedef enum PrecopyNotifyReason { + PRECOPY_NOTIFY_ERR = 0, + PRECOPY_NOTIFY_START_ITERATION = 1, + PRECOPY_NOTIFY_BEFORE_SYNC_BITMAP = 2, + PRECOPY_NOTIFY_AFTER_SYNC_BITMAP = 3, + PRECOPY_NOTIFY_MAX = 4, +} PrecopyNotifyReason; + +void precopy_infrastructure_init(void); +void precopy_add_notifier(Notifier *n); +void precopy_remove_notifier(Notifier *n); + void ram_mig_init(void); void qemu_guest_free_page_hint(void *addr, size_t len); diff --git a/migration/ram.c b/migration/ram.c index 229b791..65b1223 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -292,6 +292,8 @@ struct RAMState { bool ram_bulk_stage; /* How many times we have dirty too many pages */ int dirty_rate_high_cnt; + /* ram save states used for notifiers */ + int ram_save_state; /* these variables are used for bitmap sync */ /* last time we did a full bitmap_sync */ int64_t time_last_bitmap_sync; @@ -328,6 +330,28 @@ typedef struct RAMState RAMState; static RAMState *ram_state; +static NotifierList precopy_notifier_list; + +void precopy_infrastructure_init(void) +{ + notifier_list_init(&precopy_notifier_list); +} + +void precopy_add_notifier(Notifier *n) +{ + notifier_list_add(&precopy_notifier_list, n); +} + +void precopy_remove_notifier(Notifier *n) +{ + notifier_remove(n); +} + +static void precopy_notify(PrecopyNotifyReason reason) +{ + notifier_list_notify(&precopy_notifier_list, &reason); +} + uint64_t ram_bytes_remaining(void) { return ram_state ? (ram_state->migration_dirty_pages * TARGET_PAGE_SIZE) : @@ -1642,6 +1666,8 @@ static void migration_bitmap_sync(RAMState *rs) int64_t end_time; uint64_t bytes_xfer_now; + precopy_notify(PRECOPY_NOTIFY_BEFORE_SYNC_BITMAP); + ram_counters.dirty_sync_count++; if (!rs->time_last_bitmap_sync) { @@ -1699,6 +1725,8 @@ static void migration_bitmap_sync(RAMState *rs) if (migrate_use_events()) { qapi_event_send_migration_pass(ram_counters.dirty_sync_count); } + + precopy_notify(PRECOPY_NOTIFY_AFTER_SYNC_BITMAP); } /** @@ -2555,6 +2583,8 @@ static void ram_state_reset(RAMState *rs) rs->last_page = 0; rs->last_version = ram_list.version; rs->ram_bulk_stage = true; + + precopy_notify(PRECOPY_NOTIFY_START_ITERATION); } #define MAX_WAIT 50 /* ms, half buffered_file limit */ @@ -3324,6 +3354,7 @@ out: ret = qemu_file_get_error(f); if (ret < 0) { + precopy_notify(PRECOPY_NOTIFY_ERR); return ret; } diff --git a/vl.c b/vl.c index fa25d1a..48ae0e8 100644 --- a/vl.c +++ b/vl.c @@ -3057,6 +3057,7 @@ int main(int argc, char **argv, char **envp) module_call_init(MODULE_INIT_OPTS); runstate_init(); + precopy_infrastructure_init(); postcopy_infrastructure_init(); monitor_init_globals();
This patch adds a notifier chain for the memory precopy. This enables various precopy optimizations to be invoked at specific places. Signed-off-by: Wei Wang <wei.w.wang@intel.com> CC: Dr. David Alan Gilbert <dgilbert@redhat.com> CC: Juan Quintela <quintela@redhat.com> CC: Michael S. Tsirkin <mst@redhat.com> CC: Peter Xu <peterx@redhat.com> --- include/migration/misc.h | 12 ++++++++++++ migration/ram.c | 31 +++++++++++++++++++++++++++++++ vl.c | 1 + 3 files changed, 44 insertions(+)