[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain fo
From: |
Peter Xu |
Subject: |
Re: [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy |
Date: |
Tue, 27 Nov 2018 15:38:02 +0800 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
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 <address@hidden>
> CC: Dr. David Alan Gilbert <address@hidden>
> CC: Juan Quintela <address@hidden>
> CC: Michael S. Tsirkin <address@hidden>
> CC: Peter Xu <address@hidden>
> ---
> 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,
--
Peter Xu
- Re: [Qemu-devel] [PATCH v9 4/8] migration: API to clear bits of guest free pages from the dirty bitmap, (continued)
[Qemu-devel] [PATCH v9 3/8] migration: use bitmap_mutex in migration_bitmap_clear_dirty, Wei Wang, 2018/11/15
[Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy, Wei Wang, 2018/11/15
- Re: [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy,
Peter Xu <=
- Re: [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy, Wei Wang, 2018/11/27
- Re: [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy, Peter Xu, 2018/11/28
- Re: [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy, Wei Wang, 2018/11/28
- Re: [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy, Peter Xu, 2018/11/28
- Re: [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy, Wei Wang, 2018/11/28
- Re: [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy, Peter Xu, 2018/11/29
- Re: [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy, Peter Xu, 2018/11/29
- Re: [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy, Wei Wang, 2018/11/29
- Re: [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy, Wei Wang, 2018/11/30
- Re: [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy, Peter Xu, 2018/11/30