Re: [Qemu-devel] [PATCH v7 41/42] Disable mlock around incoming postcopy

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <address@hidden> wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
>
> Userfault doesn't work with mlock; mlock is designed to nail down pages
> so they don't move, userfault is designed to tell you when they're not
> there.
>
> munlock the pages we userfault protect before postcopy.
> mlock everything again at the end if mlock is enabled.
>
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> Reviewed-by: David Gibson <address@hidden>
> ---
>  include/sysemu/sysemu.h  |  1 +
>  migration/postcopy-ram.c | 24 ++++++++++++++++++++++++
>  2 files changed, 25 insertions(+)
>
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 1af2ea0..c1f3da4 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -171,6 +171,7 @@ extern int boot_menu;
>  extern bool boot_strict;
>  extern uint8_t *boot_splash_filedata;
>  extern size_t boot_splash_filedata_size;
> +extern bool enable_mlock;
>  extern uint8_t qemu_extra_params_fw[2];
>  extern QEMUClockType rtc_clock;
>  extern const char *mem_path;
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 7eb1fb9..be7e5f2 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -85,6 +85,11 @@ static bool ufd_version_check(int ufd)
>      return true;
>  }
>  
> +/*
> + * Note: This has the side effect of munlock'ing all of RAM, that's
> + * normally fine since if the postcopy succeeds it gets turned back on at the
> + * end.
> + */
>  bool postcopy_ram_supported_by_host(void)
>  {
>      long pagesize = getpagesize();
> @@ -113,6 +118,15 @@ bool postcopy_ram_supported_by_host(void)
>      }
>  
>      /*
> +     * userfault and mlock don't go together; we'll put it back later if
> +     * it was enabled.
> +     */
> +    if (munlockall()) {
> +        error_report("%s: munlockall: %s", __func__,  strerror(errno));


why is this not proteced by enable_mlock?

> +        return -1;
> +    }
> +
> +    /*
>       *  We need to check that the ops we need are supported on anon memory
>       *  To do that we need to register a chunk and see the flags that
>       *  are returned.
> @@ -303,6 +317,16 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState 
> *mis)
>          mis->have_fault_thread = false;
>      }
>  
> +    if (enable_mlock) {
> +        if (os_mlock() < 0) {
> +            error_report("mlock: %s", strerror(errno));
> +            /*
> +             * It doesn't feel right to fail at this point, we have a valid
> +             * VM state.
> +             */

realtime_init() exit in case of os_mlock() fails, so current code is:

- we start qemu with mlock requset
- we mlock memory
- we start postcopy
- we munlock memory
- we mlock memory

I wmill really, really preffer having a check if memory is mlocked, and
it that case, just abort migration altogether.  Or better still, wait to
enable mlock *until* we have finished postcopy, no?

Later, Juan.

> +        }
> +    }
> +
>      postcopy_state_set(mis, POSTCOPY_INCOMING_END);
>      migrate_send_rp_shut(mis, qemu_file_get_error(mis->file) != 0);