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-tatia...@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
