Re: [PATCH v2 2/2] overcommit: introduce mem-lock=on-fault

[Peter Xu]

On Wed, Dec 11, 2024 at 03:04:47AM +0300, Daniil Tatianin wrote:
> Locking the memory without MCL_ONFAULT instantly prefaults any mmaped
> anonymous memory with a write-fault, which introduces a lot of extra
> overhead in terms of memory usage when all you want to do is to prevent
> kcompactd from migrating and compacting QEMU pages. Add an option to
> only lock pages lazily as they're faulted by the process by using
> MCL_ONFAULT if asked.

Looks good but some nitpicks below..

> 
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> ---
>  include/sysemu/sysemu.h  |  1 +
>  migration/postcopy-ram.c |  4 ++--
>  qemu-options.hx          | 14 +++++++-----
>  system/globals.c         |  1 +
>  system/vl.c              | 46 ++++++++++++++++++++++++++++++++--------
>  5 files changed, 50 insertions(+), 16 deletions(-)
> 
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 7ec419ce13..b6519c3c1e 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -44,6 +44,7 @@ extern const char *keyboard_layout;
>  extern int old_param;
>  extern uint8_t *boot_splash_filedata;
>  extern bool enable_mlock;
> +extern bool enable_mlock_onfault;
>  extern bool enable_cpu_pm;
>  extern QEMUClockType rtc_clock;
>  
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 36ec6a3d75..8ff8c73a27 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -651,8 +651,8 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState 
> *mis)
>          mis->have_fault_thread = false;
>      }
>  
> -    if (enable_mlock) {
> -        if (os_mlock(false) < 0) {
> +    if (enable_mlock || enable_mlock_onfault) {
> +        if (os_mlock(enable_mlock_onfault) < 0) {
>              error_report("mlock: %s", strerror(errno));
>              /*
>               * It doesn't feel right to fail at this point, we have a valid
> diff --git a/qemu-options.hx b/qemu-options.hx
> index dacc9790a4..6c8360e62e 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4566,21 +4566,25 @@ SRST
>  ERST
>  
>  DEF("overcommit", HAS_ARG, QEMU_OPTION_overcommit,
> -    "-overcommit [mem-lock=on|off][cpu-pm=on|off]\n"
> +    "-overcommit [mem-lock=on|off|on-fault][cpu-pm=on|off]\n"
>      "                run qemu with overcommit hints\n"
> -    "                mem-lock=on|off controls memory lock support (default: 
> off)\n"
> +    "                mem-lock=on|off|on-fault controls memory lock support 
> (default: off)\n"
>      "                cpu-pm=on|off controls cpu power management (default: 
> off)\n",
>      QEMU_ARCH_ALL)
>  SRST
> -``-overcommit mem-lock=on|off``
> +``-overcommit mem-lock=on|off|on-fault``
>    \ 
>  ``-overcommit cpu-pm=on|off``
>      Run qemu with hints about host resource overcommit. The default is
>      to assume that host overcommits all resources.
>  
>      Locking qemu and guest memory can be enabled via ``mem-lock=on``
> -    (disabled by default). This works when host memory is not
> -    overcommitted and reduces the worst-case latency for guest.
> +    or ``mem-lock=on-fault`` (disabled by default). This works when
> +    host memory is not overcommitted and reduces the worst-case latency for
> +    guest. The on-fault option is better for reducing the memory footprint
> +    since it makes allocations lazy, but the pages still get locked in place
> +    once faulted by the guest or QEMU. Note that the two options are mutually
> +    exclusive.
>  
>      Guest ability to manage power state of host cpus (increasing latency
>      for other processes on the same host cpu, but decreasing latency for
> diff --git a/system/globals.c b/system/globals.c
> index 84ce943ac9..43501fe690 100644
> --- a/system/globals.c
> +++ b/system/globals.c
> @@ -35,6 +35,7 @@ enum vga_retrace_method vga_retrace_method = 
> VGA_RETRACE_DUMB;
>  int display_opengl;
>  const char* keyboard_layout;
>  bool enable_mlock;
> +bool enable_mlock_onfault;
>  bool enable_cpu_pm;
>  int autostart = 1;
>  int vga_interface_type = VGA_NONE;
> diff --git a/system/vl.c b/system/vl.c
> index 03819a80ef..4e2efd3ad4 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -347,7 +347,7 @@ static QemuOptsList qemu_overcommit_opts = {
>      .desc = {
>          {
>              .name = "mem-lock",
> -            .type = QEMU_OPT_BOOL,
> +            .type = QEMU_OPT_STRING,
>          },
>          {
>              .name = "cpu-pm",
> @@ -792,8 +792,8 @@ static QemuOptsList qemu_run_with_opts = {
>  
>  static void realtime_init(void)
>  {
> -    if (enable_mlock) {
> -        if (os_mlock(false) < 0) {
> +    if (enable_mlock || enable_mlock_onfault) {

IIUC it's still cleaner to make enable_mlock into an enum to be a
tri-state.  IOW, we could have two flags set now with the current patch
when with things like:

  -overcommit mem-lock=on -overcommit mem-lock=on-fault

> +        if (os_mlock(enable_mlock_onfault) < 0) {
>              error_report("locking memory failed");
>              exit(1);
>          }
> @@ -3532,14 +3532,42 @@ void qemu_init(int argc, char **argv)
>                  object_option_parse(optarg);
>                  break;
>              case QEMU_OPTION_overcommit:
> -                opts = qemu_opts_parse_noisily(qemu_find_opts("overcommit"),
> -                                               optarg, false);
> -                if (!opts) {
> +                {
> +                    const char *mem_lock_opt;
> +
> +                    opts = 
> qemu_opts_parse_noisily(qemu_find_opts("overcommit"),
> +                                                   optarg, false);
> +                    if (!opts) {
> +                        exit(1);
> +                    }
> +
> +                    enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", 
> enable_cpu_pm);
> +
> +                    mem_lock_opt = qemu_opt_get(opts, "mem-lock");
> +                    if (!mem_lock_opt) {
> +                        break;
> +                    }
> +
> +                    if (strcmp(mem_lock_opt, "on") == 0) {
> +                        enable_mlock = true;
> +                        break;
> +                    }
> +
> +                    if (strcmp(mem_lock_opt, "off") == 0) {
> +                        enable_mlock = false;
> +                        enable_mlock_onfault = false;
> +                        break;
> +                    }
> +
> +                    if (strcmp(mem_lock_opt, "on-fault") == 0) {
> +                        enable_mlock_onfault = true;
> +                        break;
> +                    }
> +
> +                    error_report("parameter 'mem-lock' expects one of "
> +                                 "'on', 'off', 'on-fault'");

Not the only one that open code this.. but still there're better references
like net_client_parse() or monitor_parse() that we could follow.  So would
be good to put it into a function.

Thanks,

>                      exit(1);
>                  }
> -                enable_mlock = qemu_opt_get_bool(opts, "mem-lock", 
> enable_mlock);
> -                enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", 
> enable_cpu_pm);
> -                break;
>              case QEMU_OPTION_compat:
>                  {
>                      CompatPolicy *opts_policy;
> -- 
> 2.34.1
> 

-- 
Peter Xu