* [PATCH v5 0/4] overcommit: introduce mem-lock-onfault
@ 2025-02-12 14:39 Daniil Tatianin
2025-02-12 14:39 ` [PATCH v5 1/4] os: add an ability to lock memory on_fault Daniil Tatianin
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Daniil Tatianin @ 2025-02-12 14:39 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Daniil Tatianin, Stefan Weil, Peter Xu, Fabiano Rosas,
Philippe Mathieu-Daudé, Peter Maydell, qemu-devel
Currently, passing mem-lock=on to QEMU causes memory usage to grow by
huge amounts:
no memlock:
$ ./qemu-system-x86_64 -overcommit mem-lock=off
$ ps -p $(pidof ./qemu-system-x86_64) -o rss=
45652
$ ./qemu-system-x86_64 -overcommit mem-lock=off -enable-kvm
$ ps -p $(pidof ./qemu-system-x86_64) -o rss=
39756
memlock:
$ ./qemu-system-x86_64 -overcommit mem-lock=on
$ ps -p $(pidof ./qemu-system-x86_64) -o rss=
1309876
$ ./qemu-system-x86_64 -overcommit mem-lock=on -enable-kvm
$ ps -p $(pidof ./qemu-system-x86_64) -o rss=
259956
This is caused by the fact that mlockall(2) automatically
write-faults every existing and future anonymous mappings in the
process right away.
One of the reasons to enable mem-lock is to protect a QEMU process'
pages from being compacted and migrated by kcompactd (which does so
by messing with a live process page tables causing thousands of TLB
flush IPIs per second) basically stealing all guest time while it's
active.
mem-lock=on helps against this (given compact_unevictable_allowed is 0),
but the memory overhead it introduces is an undesirable side effect,
which we can completely avoid by passing MCL_ONFAULT to mlockall, which
is what this series allows to do with a new option for mem-lock called
on-fault.
memlock-onfault:
$ ./qemu-system-x86_64 -overcommit mem-lock=on-fault
$ ps -p $(pidof ./qemu-system-x86_64) -o rss=
54004
$ ./qemu-system-x86_64 -overcommit mem-lock=on-fault -enable-kvm
$ ps -p $(pidof ./qemu-system-x86_64) -o rss=
47772
You may notice the memory usage is still slightly higher, in this case
by a few megabytes over the mem-lock=off case. I was able to trace this
down to a bug in the linux kernel with MCL_ONFAULT not being honored for
the early process heap (with brk(2) etc.) so it is still write-faulted in
this case, but it's still way less than it was with just the mem-lock=on.
Changes since v1:
- Don't make a separate mem-lock-onfault, add an on-fault option to mem-lock instead
Changes since v2:
- Move overcommit option parsing out of line
- Make enable_mlock an enum instead
Changes since v3:
- Rebase to latest master due to the recent sysemu -> system renames
Changes since v4:
- Fix compile errors under FreeBSD and MacOS
Daniil Tatianin (4):
os: add an ability to lock memory on_fault
system/vl: extract overcommit option parsing into a helper
system: introduce a new MlockState enum
overcommit: introduce mem-lock=on-fault
hw/virtio/virtio-mem.c | 2 +-
include/system/os-posix.h | 2 +-
include/system/os-win32.h | 3 ++-
include/system/system.h | 12 ++++++++-
meson.build | 6 +++++
migration/postcopy-ram.c | 4 +--
os-posix.c | 14 +++++++++--
qemu-options.hx | 14 +++++++----
system/globals.c | 12 ++++++++-
system/vl.c | 52 +++++++++++++++++++++++++++++++--------
10 files changed, 97 insertions(+), 24 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v5 1/4] os: add an ability to lock memory on_fault
2025-02-12 14:39 [PATCH v5 0/4] overcommit: introduce mem-lock-onfault Daniil Tatianin
@ 2025-02-12 14:39 ` Daniil Tatianin
2025-02-12 14:47 ` Vladimir Sementsov-Ogievskiy
` (3 more replies)
2025-02-12 14:39 ` [PATCH v5 2/4] system/vl: extract overcommit option parsing into a helper Daniil Tatianin
` (2 subsequent siblings)
3 siblings, 4 replies; 17+ messages in thread
From: Daniil Tatianin @ 2025-02-12 14:39 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Daniil Tatianin, Stefan Weil, Peter Xu, Fabiano Rosas,
Philippe Mathieu-Daudé, Peter Maydell, qemu-devel
This will be used in the following commits to make it possible to only
lock memory on fault instead of right away.
Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
include/system/os-posix.h | 2 +-
include/system/os-win32.h | 3 ++-
meson.build | 6 ++++++
migration/postcopy-ram.c | 2 +-
os-posix.c | 14 ++++++++++++--
system/vl.c | 2 +-
6 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/include/system/os-posix.h b/include/system/os-posix.h
index b881ac6c6f..ce5b3bccf8 100644
--- a/include/system/os-posix.h
+++ b/include/system/os-posix.h
@@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id);
void os_set_chroot(const char *path);
void os_setup_limits(void);
void os_setup_post(void);
-int os_mlock(void);
+int os_mlock(bool on_fault);
/**
* qemu_alloc_stack:
diff --git a/include/system/os-win32.h b/include/system/os-win32.h
index b82a5d3ad9..cd61d69e10 100644
--- a/include/system/os-win32.h
+++ b/include/system/os-win32.h
@@ -123,8 +123,9 @@ static inline bool is_daemonized(void)
return false;
}
-static inline int os_mlock(void)
+static inline int os_mlock(bool on_fault)
{
+ (void)on_fault;
return -ENOSYS;
}
diff --git a/meson.build b/meson.build
index 18cf9e2913..59953cbe6b 100644
--- a/meson.build
+++ b/meson.build
@@ -2885,6 +2885,12 @@ config_host_data.set('HAVE_MLOCKALL', cc.links(gnu_source_prefix + '''
return mlockall(MCL_FUTURE);
}'''))
+config_host_data.set('HAVE_MLOCK_ONFAULT', cc.links(gnu_source_prefix + '''
+ #include <sys/mman.h>
+ int main(void) {
+ return mlockall(MCL_FUTURE | MCL_ONFAULT);
+ }'''))
+
have_l2tpv3 = false
if get_option('l2tpv3').allowed() and have_system
have_l2tpv3 = cc.has_type('struct mmsghdr',
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 6a6da6ba7f..fc4d8a10df 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -652,7 +652,7 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
}
if (enable_mlock) {
- if (os_mlock() < 0) {
+ if (os_mlock(false) < 0) {
error_report("mlock: %s", strerror(errno));
/*
* It doesn't feel right to fail at this point, we have a valid
diff --git a/os-posix.c b/os-posix.c
index 9cce55ff2f..17b144c0a2 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -327,18 +327,28 @@ void os_set_line_buffering(void)
setvbuf(stdout, NULL, _IOLBF, 0);
}
-int os_mlock(void)
+int os_mlock(bool on_fault)
{
#ifdef HAVE_MLOCKALL
int ret = 0;
+ int flags = MCL_CURRENT | MCL_FUTURE;
- ret = mlockall(MCL_CURRENT | MCL_FUTURE);
+#ifdef HAVE_MLOCK_ONFAULT
+ if (on_fault) {
+ flags |= MCL_ONFAULT;
+ }
+#else
+ (void)on_fault;
+#endif
+
+ ret = mlockall(flags);
if (ret < 0) {
error_report("mlockall: %s", strerror(errno));
}
return ret;
#else
+ (void)on_fault;
return -ENOSYS;
#endif
}
diff --git a/system/vl.c b/system/vl.c
index 9c6942c6cf..e94fc7ea35 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -797,7 +797,7 @@ static QemuOptsList qemu_run_with_opts = {
static void realtime_init(void)
{
if (enable_mlock) {
- if (os_mlock() < 0) {
+ if (os_mlock(false) < 0) {
error_report("locking memory failed");
exit(1);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v5 2/4] system/vl: extract overcommit option parsing into a helper
2025-02-12 14:39 [PATCH v5 0/4] overcommit: introduce mem-lock-onfault Daniil Tatianin
2025-02-12 14:39 ` [PATCH v5 1/4] os: add an ability to lock memory on_fault Daniil Tatianin
@ 2025-02-12 14:39 ` Daniil Tatianin
2025-02-12 14:39 ` [PATCH v5 3/4] system: introduce a new MlockState enum Daniil Tatianin
2025-02-12 14:39 ` [PATCH v5 4/4] overcommit: introduce mem-lock=on-fault Daniil Tatianin
3 siblings, 0 replies; 17+ messages in thread
From: Daniil Tatianin @ 2025-02-12 14:39 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Daniil Tatianin, Stefan Weil, Peter Xu, Fabiano Rosas,
Philippe Mathieu-Daudé, Peter Maydell, qemu-devel,
Vladimir Sementsov-Ogievskiy
This will be extended in the future commits, let's move it out of line
right away so that it's easier to read.
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
system/vl.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/system/vl.c b/system/vl.c
index e94fc7ea35..72a40985f5 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -1875,6 +1875,19 @@ static void object_option_parse(const char *str)
visit_free(v);
}
+static void overcommit_parse(const char *str)
+{
+ QemuOpts *opts;
+
+ opts = qemu_opts_parse_noisily(qemu_find_opts("overcommit"),
+ str, false);
+ if (!opts) {
+ 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);
+}
+
/*
* Very early object creation, before the sandbox options have been activated.
*/
@@ -3575,13 +3588,7 @@ 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) {
- 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);
+ overcommit_parse(optarg);
break;
case QEMU_OPTION_compat:
{
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v5 3/4] system: introduce a new MlockState enum
2025-02-12 14:39 [PATCH v5 0/4] overcommit: introduce mem-lock-onfault Daniil Tatianin
2025-02-12 14:39 ` [PATCH v5 1/4] os: add an ability to lock memory on_fault Daniil Tatianin
2025-02-12 14:39 ` [PATCH v5 2/4] system/vl: extract overcommit option parsing into a helper Daniil Tatianin
@ 2025-02-12 14:39 ` Daniil Tatianin
2025-02-12 14:39 ` [PATCH v5 4/4] overcommit: introduce mem-lock=on-fault Daniil Tatianin
3 siblings, 0 replies; 17+ messages in thread
From: Daniil Tatianin @ 2025-02-12 14:39 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Daniil Tatianin, Stefan Weil, Peter Xu, Fabiano Rosas,
Philippe Mathieu-Daudé, Peter Maydell, qemu-devel,
Vladimir Sementsov-Ogievskiy
Replace the boolean value enable_mlock with an enum and add a helper to
decide whether we should be calling os_mlock.
This is a stepping stone towards introducing a new mlock mode, which
will be the third possible state of this enum.
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
hw/virtio/virtio-mem.c | 2 +-
include/system/system.h | 10 +++++++++-
migration/postcopy-ram.c | 2 +-
system/globals.c | 7 ++++++-
system/vl.c | 9 +++++++--
5 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index b1a003736b..7b140add76 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -991,7 +991,7 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp)
return;
}
- if (enable_mlock) {
+ if (should_mlock(mlock_state)) {
error_setg(errp, "Incompatible with mlock");
return;
}
diff --git a/include/system/system.h b/include/system/system.h
index 0cbb43ec30..dc7628357a 100644
--- a/include/system/system.h
+++ b/include/system/system.h
@@ -44,10 +44,18 @@ extern int display_opengl;
extern const char *keyboard_layout;
extern int old_param;
extern uint8_t *boot_splash_filedata;
-extern bool enable_mlock;
extern bool enable_cpu_pm;
extern QEMUClockType rtc_clock;
+typedef enum {
+ MLOCK_OFF = 0,
+ MLOCK_ON,
+} MlockState;
+
+bool should_mlock(MlockState);
+
+extern MlockState mlock_state;
+
#define MAX_OPTION_ROMS 16
typedef struct QEMUOptionRom {
const char *name;
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index fc4d8a10df..04068ee039 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -651,7 +651,7 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
mis->have_fault_thread = false;
}
- if (enable_mlock) {
+ if (should_mlock(mlock_state)) {
if (os_mlock(false) < 0) {
error_report("mlock: %s", strerror(errno));
/*
diff --git a/system/globals.c b/system/globals.c
index 4867c93ca6..adeff38348 100644
--- a/system/globals.c
+++ b/system/globals.c
@@ -31,10 +31,15 @@
#include "system/cpus.h"
#include "system/system.h"
+bool should_mlock(MlockState state)
+{
+ return state == MLOCK_ON;
+}
+
enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
int display_opengl;
const char* keyboard_layout;
-bool enable_mlock;
+MlockState mlock_state;
bool enable_cpu_pm;
int autostart = 1;
int vga_interface_type = VGA_NONE;
diff --git a/system/vl.c b/system/vl.c
index 72a40985f5..2895824c1a 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -796,7 +796,7 @@ static QemuOptsList qemu_run_with_opts = {
static void realtime_init(void)
{
- if (enable_mlock) {
+ if (should_mlock(mlock_state)) {
if (os_mlock(false) < 0) {
error_report("locking memory failed");
exit(1);
@@ -1878,13 +1878,18 @@ static void object_option_parse(const char *str)
static void overcommit_parse(const char *str)
{
QemuOpts *opts;
+ bool enable_mlock;
opts = qemu_opts_parse_noisily(qemu_find_opts("overcommit"),
str, false);
if (!opts) {
exit(1);
}
- enable_mlock = qemu_opt_get_bool(opts, "mem-lock", enable_mlock);
+
+ enable_mlock = qemu_opt_get_bool(opts, "mem-lock",
+ should_mlock(mlock_state));
+ mlock_state = enable_mlock ? MLOCK_ON : MLOCK_OFF;
+
enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", enable_cpu_pm);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v5 4/4] overcommit: introduce mem-lock=on-fault
2025-02-12 14:39 [PATCH v5 0/4] overcommit: introduce mem-lock-onfault Daniil Tatianin
` (2 preceding siblings ...)
2025-02-12 14:39 ` [PATCH v5 3/4] system: introduce a new MlockState enum Daniil Tatianin
@ 2025-02-12 14:39 ` Daniil Tatianin
3 siblings, 0 replies; 17+ messages in thread
From: Daniil Tatianin @ 2025-02-12 14:39 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Daniil Tatianin, Stefan Weil, Peter Xu, Fabiano Rosas,
Philippe Mathieu-Daudé, Peter Maydell, qemu-devel,
Vladimir Sementsov-Ogievskiy
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.
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
include/system/system.h | 2 ++
migration/postcopy-ram.c | 2 +-
qemu-options.hx | 14 +++++++++-----
system/globals.c | 7 ++++++-
system/vl.c | 34 +++++++++++++++++++++++++++-------
5 files changed, 45 insertions(+), 14 deletions(-)
diff --git a/include/system/system.h b/include/system/system.h
index dc7628357a..a7effe7dfd 100644
--- a/include/system/system.h
+++ b/include/system/system.h
@@ -50,9 +50,11 @@ extern QEMUClockType rtc_clock;
typedef enum {
MLOCK_OFF = 0,
MLOCK_ON,
+ MLOCK_ON_FAULT,
} MlockState;
bool should_mlock(MlockState);
+bool is_mlock_on_fault(MlockState);
extern MlockState mlock_state;
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 04068ee039..5d3edfcfec 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -652,7 +652,7 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
}
if (should_mlock(mlock_state)) {
- if (os_mlock(false) < 0) {
+ if (os_mlock(is_mlock_on_fault(mlock_state)) < 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 1b26ad53bd..61270e3206 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4632,21 +4632,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 adeff38348..316623bd20 100644
--- a/system/globals.c
+++ b/system/globals.c
@@ -33,7 +33,12 @@
bool should_mlock(MlockState state)
{
- return state == MLOCK_ON;
+ return state == MLOCK_ON || state == MLOCK_ON_FAULT;
+}
+
+bool is_mlock_on_fault(MlockState state)
+{
+ return state == MLOCK_ON_FAULT;
}
enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
diff --git a/system/vl.c b/system/vl.c
index 2895824c1a..3c0fa2ff64 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -351,7 +351,7 @@ static QemuOptsList qemu_overcommit_opts = {
.desc = {
{
.name = "mem-lock",
- .type = QEMU_OPT_BOOL,
+ .type = QEMU_OPT_STRING,
},
{
.name = "cpu-pm",
@@ -797,7 +797,7 @@ static QemuOptsList qemu_run_with_opts = {
static void realtime_init(void)
{
if (should_mlock(mlock_state)) {
- if (os_mlock(false) < 0) {
+ if (os_mlock(is_mlock_on_fault(mlock_state)) < 0) {
error_report("locking memory failed");
exit(1);
}
@@ -1878,7 +1878,7 @@ static void object_option_parse(const char *str)
static void overcommit_parse(const char *str)
{
QemuOpts *opts;
- bool enable_mlock;
+ const char *mem_lock_opt;
opts = qemu_opts_parse_noisily(qemu_find_opts("overcommit"),
str, false);
@@ -1886,11 +1886,31 @@ static void overcommit_parse(const char *str)
exit(1);
}
- enable_mlock = qemu_opt_get_bool(opts, "mem-lock",
- should_mlock(mlock_state));
- mlock_state = enable_mlock ? MLOCK_ON : MLOCK_OFF;
-
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) {
+ return;
+ }
+
+ if (strcmp(mem_lock_opt, "on") == 0) {
+ mlock_state = MLOCK_ON;
+ return;
+ }
+
+ if (strcmp(mem_lock_opt, "off") == 0) {
+ mlock_state = MLOCK_OFF;
+ return;
+ }
+
+ if (strcmp(mem_lock_opt, "on-fault") == 0) {
+ mlock_state = MLOCK_ON_FAULT;
+ return;
+ }
+
+ error_report("parameter 'mem-lock' expects one of "
+ "'on', 'off', 'on-fault'");
+ exit(1);
}
/*
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v5 1/4] os: add an ability to lock memory on_fault
2025-02-12 14:39 ` [PATCH v5 1/4] os: add an ability to lock memory on_fault Daniil Tatianin
@ 2025-02-12 14:47 ` Vladimir Sementsov-Ogievskiy
2025-02-12 14:48 ` Philippe Mathieu-Daudé
` (2 subsequent siblings)
3 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-02-12 14:47 UTC (permalink / raw)
To: Daniil Tatianin, Paolo Bonzini
Cc: Stefan Weil, Peter Xu, Fabiano Rosas, Philippe Mathieu-Daudé,
Peter Maydell, qemu-devel
On 12.02.25 17:39, Daniil Tatianin wrote:
> This will be used in the following commits to make it possible to only
> lock memory on fault instead of right away.
>
> Signed-off-by: Daniil Tatianin<d-tatianin@yandex-team.ru>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 1/4] os: add an ability to lock memory on_fault
2025-02-12 14:39 ` [PATCH v5 1/4] os: add an ability to lock memory on_fault Daniil Tatianin
2025-02-12 14:47 ` Vladimir Sementsov-Ogievskiy
@ 2025-02-12 14:48 ` Philippe Mathieu-Daudé
2025-02-12 14:51 ` Daniil Tatianin
2025-02-12 15:23 ` Peter Xu
2025-02-12 15:30 ` Daniel P. Berrangé
3 siblings, 1 reply; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-02-12 14:48 UTC (permalink / raw)
To: Daniil Tatianin, Paolo Bonzini
Cc: Stefan Weil, Peter Xu, Fabiano Rosas, Peter Maydell, qemu-devel
Hi Daniil,
On 12/2/25 15:39, Daniil Tatianin wrote:
> This will be used in the following commits to make it possible to only
> lock memory on fault instead of right away.
>
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> ---
> include/system/os-posix.h | 2 +-
> include/system/os-win32.h | 3 ++-
> meson.build | 6 ++++++
> migration/postcopy-ram.c | 2 +-
> os-posix.c | 14 ++++++++++++--
> system/vl.c | 2 +-
> 6 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/include/system/os-posix.h b/include/system/os-posix.h
> index b881ac6c6f..ce5b3bccf8 100644
> --- a/include/system/os-posix.h
> +++ b/include/system/os-posix.h
> @@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id);
> void os_set_chroot(const char *path);
> void os_setup_limits(void);
> void os_setup_post(void);
> -int os_mlock(void);
> +int os_mlock(bool on_fault);
If we need to support more flag, is your plan to add more arguments?
Otherwise, why not use a 'int flags' argument, and have the callers
pass MCL_ONFAULT?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 1/4] os: add an ability to lock memory on_fault
2025-02-12 14:48 ` Philippe Mathieu-Daudé
@ 2025-02-12 14:51 ` Daniil Tatianin
2025-02-12 14:55 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 17+ messages in thread
From: Daniil Tatianin @ 2025-02-12 14:51 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Paolo Bonzini
Cc: Stefan Weil, Peter Xu, Fabiano Rosas, Peter Maydell, qemu-devel
On 2/12/25 5:48 PM, Philippe Mathieu-Daudé wrote:
> Hi Daniil,
>
> On 12/2/25 15:39, Daniil Tatianin wrote:
>> This will be used in the following commits to make it possible to only
>> lock memory on fault instead of right away.
>>
>> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
>> ---
>> include/system/os-posix.h | 2 +-
>> include/system/os-win32.h | 3 ++-
>> meson.build | 6 ++++++
>> migration/postcopy-ram.c | 2 +-
>> os-posix.c | 14 ++++++++++++--
>> system/vl.c | 2 +-
>> 6 files changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/system/os-posix.h b/include/system/os-posix.h
>> index b881ac6c6f..ce5b3bccf8 100644
>> --- a/include/system/os-posix.h
>> +++ b/include/system/os-posix.h
>> @@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id);
>> void os_set_chroot(const char *path);
>> void os_setup_limits(void);
>> void os_setup_post(void);
>> -int os_mlock(void);
>> +int os_mlock(bool on_fault);
>
> If we need to support more flag, is your plan to add more arguments?
> Otherwise, why not use a 'int flags' argument, and have the callers
> pass MCL_ONFAULT?
Hi!
I chose this approach because MCL_ONFAULT is a POSIX/linux-specific
flag, and this function is called in places that are platform-agnostic,
thus a bool flag seemed more fitting.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 1/4] os: add an ability to lock memory on_fault
2025-02-12 14:51 ` Daniil Tatianin
@ 2025-02-12 14:55 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-02-12 14:55 UTC (permalink / raw)
To: Daniil Tatianin, Paolo Bonzini
Cc: Stefan Weil, Peter Xu, Fabiano Rosas, Peter Maydell, qemu-devel
On 12/2/25 15:51, Daniil Tatianin wrote:
> On 2/12/25 5:48 PM, Philippe Mathieu-Daudé wrote:
>
>> Hi Daniil,
>>
>> On 12/2/25 15:39, Daniil Tatianin wrote:
>>> This will be used in the following commits to make it possible to only
>>> lock memory on fault instead of right away.
>>>
>>> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
>>> ---
>>> include/system/os-posix.h | 2 +-
>>> include/system/os-win32.h | 3 ++-
>>> meson.build | 6 ++++++
>>> migration/postcopy-ram.c | 2 +-
>>> os-posix.c | 14 ++++++++++++--
>>> system/vl.c | 2 +-
>>> 6 files changed, 23 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/system/os-posix.h b/include/system/os-posix.h
>>> index b881ac6c6f..ce5b3bccf8 100644
>>> --- a/include/system/os-posix.h
>>> +++ b/include/system/os-posix.h
>>> @@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id);
>>> void os_set_chroot(const char *path);
>>> void os_setup_limits(void);
>>> void os_setup_post(void);
>>> -int os_mlock(void);
>>> +int os_mlock(bool on_fault);
>>
>> If we need to support more flag, is your plan to add more arguments?
>> Otherwise, why not use a 'int flags' argument, and have the callers
>> pass MCL_ONFAULT?
>
> Hi!
>
> I chose this approach because MCL_ONFAULT is a POSIX/linux-specific
> flag, and this function is called in places that are platform-agnostic,
> thus a bool flag seemed more fitting.
OK then.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 1/4] os: add an ability to lock memory on_fault
2025-02-12 14:39 ` [PATCH v5 1/4] os: add an ability to lock memory on_fault Daniil Tatianin
2025-02-12 14:47 ` Vladimir Sementsov-Ogievskiy
2025-02-12 14:48 ` Philippe Mathieu-Daudé
@ 2025-02-12 15:23 ` Peter Xu
2025-02-12 15:27 ` Daniil Tatianin
2025-02-12 15:46 ` Philippe Mathieu-Daudé
2025-02-12 15:30 ` Daniel P. Berrangé
3 siblings, 2 replies; 17+ messages in thread
From: Peter Xu @ 2025-02-12 15:23 UTC (permalink / raw)
To: Daniil Tatianin
Cc: Paolo Bonzini, Stefan Weil, Fabiano Rosas,
Philippe Mathieu-Daudé, Peter Maydell, qemu-devel
On Wed, Feb 12, 2025 at 05:39:17PM +0300, Daniil Tatianin wrote:
> This will be used in the following commits to make it possible to only
> lock memory on fault instead of right away.
>
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> ---
> include/system/os-posix.h | 2 +-
> include/system/os-win32.h | 3 ++-
> meson.build | 6 ++++++
> migration/postcopy-ram.c | 2 +-
> os-posix.c | 14 ++++++++++++--
> system/vl.c | 2 +-
> 6 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/include/system/os-posix.h b/include/system/os-posix.h
> index b881ac6c6f..ce5b3bccf8 100644
> --- a/include/system/os-posix.h
> +++ b/include/system/os-posix.h
> @@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id);
> void os_set_chroot(const char *path);
> void os_setup_limits(void);
> void os_setup_post(void);
> -int os_mlock(void);
> +int os_mlock(bool on_fault);
>
> /**
> * qemu_alloc_stack:
> diff --git a/include/system/os-win32.h b/include/system/os-win32.h
> index b82a5d3ad9..cd61d69e10 100644
> --- a/include/system/os-win32.h
> +++ b/include/system/os-win32.h
> @@ -123,8 +123,9 @@ static inline bool is_daemonized(void)
> return false;
> }
>
> -static inline int os_mlock(void)
> +static inline int os_mlock(bool on_fault)
> {
> + (void)on_fault;
> return -ENOSYS;
> }
>
> diff --git a/meson.build b/meson.build
> index 18cf9e2913..59953cbe6b 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2885,6 +2885,12 @@ config_host_data.set('HAVE_MLOCKALL', cc.links(gnu_source_prefix + '''
> return mlockall(MCL_FUTURE);
> }'''))
>
> +config_host_data.set('HAVE_MLOCK_ONFAULT', cc.links(gnu_source_prefix + '''
> + #include <sys/mman.h>
> + int main(void) {
> + return mlockall(MCL_FUTURE | MCL_ONFAULT);
> + }'''))
> +
> have_l2tpv3 = false
> if get_option('l2tpv3').allowed() and have_system
> have_l2tpv3 = cc.has_type('struct mmsghdr',
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 6a6da6ba7f..fc4d8a10df 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -652,7 +652,7 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
> }
>
> if (enable_mlock) {
> - if (os_mlock() < 0) {
> + if (os_mlock(false) < 0) {
> error_report("mlock: %s", strerror(errno));
> /*
> * It doesn't feel right to fail at this point, we have a valid
> diff --git a/os-posix.c b/os-posix.c
> index 9cce55ff2f..17b144c0a2 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -327,18 +327,28 @@ void os_set_line_buffering(void)
> setvbuf(stdout, NULL, _IOLBF, 0);
> }
>
> -int os_mlock(void)
> +int os_mlock(bool on_fault)
> {
> #ifdef HAVE_MLOCKALL
> int ret = 0;
> + int flags = MCL_CURRENT | MCL_FUTURE;
>
> - ret = mlockall(MCL_CURRENT | MCL_FUTURE);
> +#ifdef HAVE_MLOCK_ONFAULT
> + if (on_fault) {
> + flags |= MCL_ONFAULT;
> + }
> +#else
> + (void)on_fault;
> +#endif
IIUC we should fail this when not supported.
Would you mind I squash this in?
diff --git a/os-posix.c b/os-posix.c
index 17b144c0a2..52925c23d3 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -333,13 +333,14 @@ int os_mlock(bool on_fault)
int ret = 0;
int flags = MCL_CURRENT | MCL_FUTURE;
-#ifdef HAVE_MLOCK_ONFAULT
if (on_fault) {
+#ifdef HAVE_MLOCK_ONFAULT
flags |= MCL_ONFAULT;
- }
#else
- (void)on_fault;
+ error_report("mlockall: on_fault not supported");
+ return -EINVAL;
#endif
+ }
ret = mlockall(flags);
if (ret < 0) {
> +
> + ret = mlockall(flags);
> if (ret < 0) {
> error_report("mlockall: %s", strerror(errno));
> }
>
> return ret;
> #else
> + (void)on_fault;
> return -ENOSYS;
> #endif
> }
> diff --git a/system/vl.c b/system/vl.c
> index 9c6942c6cf..e94fc7ea35 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -797,7 +797,7 @@ static QemuOptsList qemu_run_with_opts = {
> static void realtime_init(void)
> {
> if (enable_mlock) {
> - if (os_mlock() < 0) {
> + if (os_mlock(false) < 0) {
> error_report("locking memory failed");
> exit(1);
> }
> --
> 2.34.1
>
--
Peter Xu
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v5 1/4] os: add an ability to lock memory on_fault
2025-02-12 15:23 ` Peter Xu
@ 2025-02-12 15:27 ` Daniil Tatianin
2025-02-12 15:46 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 17+ messages in thread
From: Daniil Tatianin @ 2025-02-12 15:27 UTC (permalink / raw)
To: Peter Xu
Cc: Paolo Bonzini, Stefan Weil, Fabiano Rosas,
Philippe Mathieu-Daudé, Peter Maydell, qemu-devel
On 2/12/25 6:23 PM, Peter Xu wrote:
> On Wed, Feb 12, 2025 at 05:39:17PM +0300, Daniil Tatianin wrote:
>> This will be used in the following commits to make it possible to only
>> lock memory on fault instead of right away.
>>
>> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
>> ---
>> include/system/os-posix.h | 2 +-
>> include/system/os-win32.h | 3 ++-
>> meson.build | 6 ++++++
>> migration/postcopy-ram.c | 2 +-
>> os-posix.c | 14 ++++++++++++--
>> system/vl.c | 2 +-
>> 6 files changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/system/os-posix.h b/include/system/os-posix.h
>> index b881ac6c6f..ce5b3bccf8 100644
>> --- a/include/system/os-posix.h
>> +++ b/include/system/os-posix.h
>> @@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id);
>> void os_set_chroot(const char *path);
>> void os_setup_limits(void);
>> void os_setup_post(void);
>> -int os_mlock(void);
>> +int os_mlock(bool on_fault);
>>
>> /**
>> * qemu_alloc_stack:
>> diff --git a/include/system/os-win32.h b/include/system/os-win32.h
>> index b82a5d3ad9..cd61d69e10 100644
>> --- a/include/system/os-win32.h
>> +++ b/include/system/os-win32.h
>> @@ -123,8 +123,9 @@ static inline bool is_daemonized(void)
>> return false;
>> }
>>
>> -static inline int os_mlock(void)
>> +static inline int os_mlock(bool on_fault)
>> {
>> + (void)on_fault;
>> return -ENOSYS;
>> }
>>
>> diff --git a/meson.build b/meson.build
>> index 18cf9e2913..59953cbe6b 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -2885,6 +2885,12 @@ config_host_data.set('HAVE_MLOCKALL', cc.links(gnu_source_prefix + '''
>> return mlockall(MCL_FUTURE);
>> }'''))
>>
>> +config_host_data.set('HAVE_MLOCK_ONFAULT', cc.links(gnu_source_prefix + '''
>> + #include <sys/mman.h>
>> + int main(void) {
>> + return mlockall(MCL_FUTURE | MCL_ONFAULT);
>> + }'''))
>> +
>> have_l2tpv3 = false
>> if get_option('l2tpv3').allowed() and have_system
>> have_l2tpv3 = cc.has_type('struct mmsghdr',
>> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
>> index 6a6da6ba7f..fc4d8a10df 100644
>> --- a/migration/postcopy-ram.c
>> +++ b/migration/postcopy-ram.c
>> @@ -652,7 +652,7 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
>> }
>>
>> if (enable_mlock) {
>> - if (os_mlock() < 0) {
>> + if (os_mlock(false) < 0) {
>> error_report("mlock: %s", strerror(errno));
>> /*
>> * It doesn't feel right to fail at this point, we have a valid
>> diff --git a/os-posix.c b/os-posix.c
>> index 9cce55ff2f..17b144c0a2 100644
>> --- a/os-posix.c
>> +++ b/os-posix.c
>> @@ -327,18 +327,28 @@ void os_set_line_buffering(void)
>> setvbuf(stdout, NULL, _IOLBF, 0);
>> }
>>
>> -int os_mlock(void)
>> +int os_mlock(bool on_fault)
>> {
>> #ifdef HAVE_MLOCKALL
>> int ret = 0;
>> + int flags = MCL_CURRENT | MCL_FUTURE;
>>
>> - ret = mlockall(MCL_CURRENT | MCL_FUTURE);
>> +#ifdef HAVE_MLOCK_ONFAULT
>> + if (on_fault) {
>> + flags |= MCL_ONFAULT;
>> + }
>> +#else
>> + (void)on_fault;
>> +#endif
> IIUC we should fail this when not supported.
>
> Would you mind I squash this in?
Sure, go ahead. Thanks!
>
> diff --git a/os-posix.c b/os-posix.c
> index 17b144c0a2..52925c23d3 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -333,13 +333,14 @@ int os_mlock(bool on_fault)
> int ret = 0;
> int flags = MCL_CURRENT | MCL_FUTURE;
>
> -#ifdef HAVE_MLOCK_ONFAULT
> if (on_fault) {
> +#ifdef HAVE_MLOCK_ONFAULT
> flags |= MCL_ONFAULT;
> - }
> #else
> - (void)on_fault;
> + error_report("mlockall: on_fault not supported");
> + return -EINVAL;
> #endif
> + }
>
> ret = mlockall(flags);
> if (ret < 0) {
>
>
>> +
>> + ret = mlockall(flags);
>> if (ret < 0) {
>> error_report("mlockall: %s", strerror(errno));
>> }
>>
>> return ret;
>> #else
>> + (void)on_fault;
>> return -ENOSYS;
>> #endif
>> }
>> diff --git a/system/vl.c b/system/vl.c
>> index 9c6942c6cf..e94fc7ea35 100644
>> --- a/system/vl.c
>> +++ b/system/vl.c
>> @@ -797,7 +797,7 @@ static QemuOptsList qemu_run_with_opts = {
>> static void realtime_init(void)
>> {
>> if (enable_mlock) {
>> - if (os_mlock() < 0) {
>> + if (os_mlock(false) < 0) {
>> error_report("locking memory failed");
>> exit(1);
>> }
>> --
>> 2.34.1
>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 1/4] os: add an ability to lock memory on_fault
2025-02-12 14:39 ` [PATCH v5 1/4] os: add an ability to lock memory on_fault Daniil Tatianin
` (2 preceding siblings ...)
2025-02-12 15:23 ` Peter Xu
@ 2025-02-12 15:30 ` Daniel P. Berrangé
2025-02-12 15:41 ` Peter Xu
2025-02-12 16:17 ` Daniil Tatianin
3 siblings, 2 replies; 17+ messages in thread
From: Daniel P. Berrangé @ 2025-02-12 15:30 UTC (permalink / raw)
To: Daniil Tatianin
Cc: Paolo Bonzini, Stefan Weil, Peter Xu, Fabiano Rosas,
Philippe Mathieu-Daudé, Peter Maydell, qemu-devel
On Wed, Feb 12, 2025 at 05:39:17PM +0300, Daniil Tatianin wrote:
> This will be used in the following commits to make it possible to only
> lock memory on fault instead of right away.
>
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> ---
> include/system/os-posix.h | 2 +-
> include/system/os-win32.h | 3 ++-
> meson.build | 6 ++++++
> migration/postcopy-ram.c | 2 +-
> os-posix.c | 14 ++++++++++++--
> system/vl.c | 2 +-
> 6 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/include/system/os-posix.h b/include/system/os-posix.h
> index b881ac6c6f..ce5b3bccf8 100644
> --- a/include/system/os-posix.h
> +++ b/include/system/os-posix.h
> @@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id);
> void os_set_chroot(const char *path);
> void os_setup_limits(void);
> void os_setup_post(void);
> -int os_mlock(void);
> +int os_mlock(bool on_fault);
>
> /**
> * qemu_alloc_stack:
> diff --git a/include/system/os-win32.h b/include/system/os-win32.h
> index b82a5d3ad9..cd61d69e10 100644
> --- a/include/system/os-win32.h
> +++ b/include/system/os-win32.h
> @@ -123,8 +123,9 @@ static inline bool is_daemonized(void)
> return false;
> }
>
> -static inline int os_mlock(void)
> +static inline int os_mlock(bool on_fault)
> {
> + (void)on_fault;
Is this really needed ? Our compiler flags don't enable warnings
about unused variables.
If they did, this would not be the way to hide them. Instead you
would use the "G_GNUC_UNUSED" annotation against the parameter.
eg
static inline int os_mlock(bool on_fault G_GNUC_UNUSED)
> return -ENOSYS;
> }
>
> diff --git a/os-posix.c b/os-posix.c
> index 9cce55ff2f..17b144c0a2 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -327,18 +327,28 @@ void os_set_line_buffering(void)
> setvbuf(stdout, NULL, _IOLBF, 0);
> }
>
> -int os_mlock(void)
> +int os_mlock(bool on_fault)
> {
> #ifdef HAVE_MLOCKALL
> int ret = 0;
> + int flags = MCL_CURRENT | MCL_FUTURE;
>
> - ret = mlockall(MCL_CURRENT | MCL_FUTURE);
> +#ifdef HAVE_MLOCK_ONFAULT
> + if (on_fault) {
> + flags |= MCL_ONFAULT;
> + }
> +#else
> + (void)on_fault;
> +#endif
> +
> + ret = mlockall(flags);
> if (ret < 0) {
> error_report("mlockall: %s", strerror(errno));
> }
>
> return ret;
> #else
> + (void)on_fault;
> return -ENOSYS;
> #endif
Again casting to (void) should not be required AFAIK.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 1/4] os: add an ability to lock memory on_fault
2025-02-12 15:30 ` Daniel P. Berrangé
@ 2025-02-12 15:41 ` Peter Xu
2025-02-12 16:17 ` Daniil Tatianin
1 sibling, 0 replies; 17+ messages in thread
From: Peter Xu @ 2025-02-12 15:41 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Daniil Tatianin, Paolo Bonzini, Stefan Weil, Fabiano Rosas,
Philippe Mathieu-Daudé, Peter Maydell, qemu-devel
On Wed, Feb 12, 2025 at 03:30:21PM +0000, Daniel P. Berrangé wrote:
> > diff --git a/include/system/os-win32.h b/include/system/os-win32.h
> > index b82a5d3ad9..cd61d69e10 100644
> > --- a/include/system/os-win32.h
> > +++ b/include/system/os-win32.h
> > @@ -123,8 +123,9 @@ static inline bool is_daemonized(void)
> > return false;
> > }
> >
> > -static inline int os_mlock(void)
> > +static inline int os_mlock(bool on_fault)
> > {
> > + (void)on_fault;
>
> Is this really needed ? Our compiler flags don't enable warnings
> about unused variables.
>
> If they did, this would not be the way to hide them. Instead you
> would use the "G_GNUC_UNUSED" annotation against the parameter.
> eg
>
> static inline int os_mlock(bool on_fault G_GNUC_UNUSED)
To be on the safe side.. I'll try to keep the marker if no one disagrees.
So that's:
diff --git a/include/system/os-win32.h b/include/system/os-win32.h
index cd61d69e10..bc623061d8 100644
--- a/include/system/os-win32.h
+++ b/include/system/os-win32.h
@@ -123,9 +123,8 @@ static inline bool is_daemonized(void)
return false;
}
-static inline int os_mlock(bool on_fault)
+static inline int os_mlock(bool on_fault G_GNUC_UNUSED)
{
- (void)on_fault;
return -ENOSYS;
}
>
> > return -ENOSYS;
> > }
--
Peter Xu
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v5 1/4] os: add an ability to lock memory on_fault
2025-02-12 15:23 ` Peter Xu
2025-02-12 15:27 ` Daniil Tatianin
@ 2025-02-12 15:46 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-02-12 15:46 UTC (permalink / raw)
To: Peter Xu, Daniil Tatianin
Cc: Paolo Bonzini, Stefan Weil, Fabiano Rosas, Peter Maydell,
qemu-devel
On 12/2/25 16:23, Peter Xu wrote:
> On Wed, Feb 12, 2025 at 05:39:17PM +0300, Daniil Tatianin wrote:
>> This will be used in the following commits to make it possible to only
>> lock memory on fault instead of right away.
>>
>> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
>> ---
>> include/system/os-posix.h | 2 +-
>> include/system/os-win32.h | 3 ++-
>> meson.build | 6 ++++++
>> migration/postcopy-ram.c | 2 +-
>> os-posix.c | 14 ++++++++++++--
>> system/vl.c | 2 +-
>> 6 files changed, 23 insertions(+), 6 deletions(-)
>> diff --git a/os-posix.c b/os-posix.c
>> index 9cce55ff2f..17b144c0a2 100644
>> --- a/os-posix.c
>> +++ b/os-posix.c
>> @@ -327,18 +327,28 @@ void os_set_line_buffering(void)
>> setvbuf(stdout, NULL, _IOLBF, 0);
>> }
>>
>> -int os_mlock(void)
>> +int os_mlock(bool on_fault)
>> {
>> #ifdef HAVE_MLOCKALL
>> int ret = 0;
>> + int flags = MCL_CURRENT | MCL_FUTURE;
>>
>> - ret = mlockall(MCL_CURRENT | MCL_FUTURE);
>> +#ifdef HAVE_MLOCK_ONFAULT
>> + if (on_fault) {
>> + flags |= MCL_ONFAULT;
>> + }
>> +#else
>> + (void)on_fault;
>> +#endif
>
> IIUC we should fail this when not supported.
>
> Would you mind I squash this in?
>
> diff --git a/os-posix.c b/os-posix.c
> index 17b144c0a2..52925c23d3 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -333,13 +333,14 @@ int os_mlock(bool on_fault)
> int ret = 0;
> int flags = MCL_CURRENT | MCL_FUTURE;
>
> -#ifdef HAVE_MLOCK_ONFAULT
> if (on_fault) {
> +#ifdef HAVE_MLOCK_ONFAULT
> flags |= MCL_ONFAULT;
> - }
> #else
> - (void)on_fault;
> + error_report("mlockall: on_fault not supported");
> + return -EINVAL;
Good point!
> #endif
> + }
>
> ret = mlockall(flags);
> if (ret < 0) {
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 1/4] os: add an ability to lock memory on_fault
2025-02-12 15:30 ` Daniel P. Berrangé
2025-02-12 15:41 ` Peter Xu
@ 2025-02-12 16:17 ` Daniil Tatianin
2025-02-12 16:42 ` Peter Xu
1 sibling, 1 reply; 17+ messages in thread
From: Daniil Tatianin @ 2025-02-12 16:17 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Paolo Bonzini, Stefan Weil, Peter Xu, Fabiano Rosas,
Philippe Mathieu-Daudé, Peter Maydell, qemu-devel
On 2/12/25 6:30 PM, Daniel P. Berrangé wrote:
> On Wed, Feb 12, 2025 at 05:39:17PM +0300, Daniil Tatianin wrote:
>> This will be used in the following commits to make it possible to only
>> lock memory on fault instead of right away.
>>
>> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
>> ---
>> include/system/os-posix.h | 2 +-
>> include/system/os-win32.h | 3 ++-
>> meson.build | 6 ++++++
>> migration/postcopy-ram.c | 2 +-
>> os-posix.c | 14 ++++++++++++--
>> system/vl.c | 2 +-
>> 6 files changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/system/os-posix.h b/include/system/os-posix.h
>> index b881ac6c6f..ce5b3bccf8 100644
>> --- a/include/system/os-posix.h
>> +++ b/include/system/os-posix.h
>> @@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id);
>> void os_set_chroot(const char *path);
>> void os_setup_limits(void);
>> void os_setup_post(void);
>> -int os_mlock(void);
>> +int os_mlock(bool on_fault);
>>
>> /**
>> * qemu_alloc_stack:
>> diff --git a/include/system/os-win32.h b/include/system/os-win32.h
>> index b82a5d3ad9..cd61d69e10 100644
>> --- a/include/system/os-win32.h
>> +++ b/include/system/os-win32.h
>> @@ -123,8 +123,9 @@ static inline bool is_daemonized(void)
>> return false;
>> }
>>
>> -static inline int os_mlock(void)
>> +static inline int os_mlock(bool on_fault)
>> {
>> + (void)on_fault;
> Is this really needed ? Our compiler flags don't enable warnings
> about unused variables.
Hmm, I was not aware of that, thank you.
Peter, do you want me to resend, or can you squash remove this as well?
>
> If they did, this would not be the way to hide them. Instead you
> would use the "G_GNUC_UNUSED" annotation against the parameter.
> eg
>
> static inline int os_mlock(bool on_fault G_GNUC_UNUSED)
>
>> return -ENOSYS;
>> }
>>
>> diff --git a/os-posix.c b/os-posix.c
>> index 9cce55ff2f..17b144c0a2 100644
>> --- a/os-posix.c
>> +++ b/os-posix.c
>> @@ -327,18 +327,28 @@ void os_set_line_buffering(void)
>> setvbuf(stdout, NULL, _IOLBF, 0);
>> }
>>
>> -int os_mlock(void)
>> +int os_mlock(bool on_fault)
>> {
>> #ifdef HAVE_MLOCKALL
>> int ret = 0;
>> + int flags = MCL_CURRENT | MCL_FUTURE;
>>
>> - ret = mlockall(MCL_CURRENT | MCL_FUTURE);
>> +#ifdef HAVE_MLOCK_ONFAULT
>> + if (on_fault) {
>> + flags |= MCL_ONFAULT;
>> + }
>> +#else
>> + (void)on_fault;
>> +#endif
>> +
>> + ret = mlockall(flags);
>> if (ret < 0) {
>> error_report("mlockall: %s", strerror(errno));
>> }
>>
>> return ret;
>> #else
>> + (void)on_fault;
>> return -ENOSYS;
>> #endif
> Again casting to (void) should not be required AFAIK.
>
>
> With regards,
> Daniel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 1/4] os: add an ability to lock memory on_fault
2025-02-12 16:17 ` Daniil Tatianin
@ 2025-02-12 16:42 ` Peter Xu
2025-02-12 16:46 ` Daniil Tatianin
0 siblings, 1 reply; 17+ messages in thread
From: Peter Xu @ 2025-02-12 16:42 UTC (permalink / raw)
To: Daniil Tatianin
Cc: Daniel P. Berrangé, Paolo Bonzini, Stefan Weil,
Fabiano Rosas, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel
On Wed, Feb 12, 2025 at 07:17:45PM +0300, Daniil Tatianin wrote:
> > > -static inline int os_mlock(void)
> > > +static inline int os_mlock(bool on_fault)
> > > {
> > > + (void)on_fault;
> > Is this really needed ? Our compiler flags don't enable warnings
> > about unused variables.
>
> Hmm, I was not aware of that, thank you.
>
> Peter, do you want me to resend, or can you squash remove this as well?
I'll do, no worry.
--
Peter Xu
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 1/4] os: add an ability to lock memory on_fault
2025-02-12 16:42 ` Peter Xu
@ 2025-02-12 16:46 ` Daniil Tatianin
0 siblings, 0 replies; 17+ messages in thread
From: Daniil Tatianin @ 2025-02-12 16:46 UTC (permalink / raw)
To: Peter Xu
Cc: Daniel P. Berrangé, Paolo Bonzini, Stefan Weil,
Fabiano Rosas, Philippe Mathieu-Daudé, Peter Maydell,
qemu-devel
On 2/12/25 7:42 PM, Peter Xu wrote:
> On Wed, Feb 12, 2025 at 07:17:45PM +0300, Daniil Tatianin wrote:
>>>> -static inline int os_mlock(void)
>>>> +static inline int os_mlock(bool on_fault)
>>>> {
>>>> + (void)on_fault;
>>> Is this really needed ? Our compiler flags don't enable warnings
>>> about unused variables.
>> Hmm, I was not aware of that, thank you.
>>
>> Peter, do you want me to resend, or can you squash remove this as well?
> I'll do, no worry.
Much appreciated!
>
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2025-02-12 16:47 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-12 14:39 [PATCH v5 0/4] overcommit: introduce mem-lock-onfault Daniil Tatianin
2025-02-12 14:39 ` [PATCH v5 1/4] os: add an ability to lock memory on_fault Daniil Tatianin
2025-02-12 14:47 ` Vladimir Sementsov-Ogievskiy
2025-02-12 14:48 ` Philippe Mathieu-Daudé
2025-02-12 14:51 ` Daniil Tatianin
2025-02-12 14:55 ` Philippe Mathieu-Daudé
2025-02-12 15:23 ` Peter Xu
2025-02-12 15:27 ` Daniil Tatianin
2025-02-12 15:46 ` Philippe Mathieu-Daudé
2025-02-12 15:30 ` Daniel P. Berrangé
2025-02-12 15:41 ` Peter Xu
2025-02-12 16:17 ` Daniil Tatianin
2025-02-12 16:42 ` Peter Xu
2025-02-12 16:46 ` Daniil Tatianin
2025-02-12 14:39 ` [PATCH v5 2/4] system/vl: extract overcommit option parsing into a helper Daniil Tatianin
2025-02-12 14:39 ` [PATCH v5 3/4] system: introduce a new MlockState enum Daniil Tatianin
2025-02-12 14:39 ` [PATCH v5 4/4] overcommit: introduce mem-lock=on-fault Daniil Tatianin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.