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

[Daniil Tatianin]

On 12/12/24 12:52 AM, Peter Xu wrote:

> 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

Oh yeah good catch, that's a problem.

> > +        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,

Yeah I'm not a fan of this solution to parsing, but I wasn't able to 
find better examples straight away so went with this. Will take a look 
at the places you've mentioned.

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