* [PATCH v4 2/5] futex: Create set_robust_list2
From: André Almeida @ 2025-02-25 18:35 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Arnd Bergmann, sonicadvance1
Cc: linux-kernel, kernel-dev, linux-api, Vinicius Peixoto,
Sebastian Andrzej Siewior, André Almeida
In-Reply-To: <20250225183531.682556-1-andrealmeid@igalia.com>
Create a new robust_list() syscall. The current syscall can't be
expanded to cover the following use case, so a new one is needed. This
new syscall allows users to set multiple robust lists per process and to
have either 32bit or 64bit pointers in the list.
* Interface
This is the proposed interface:
long set_robust_list2(void *head, int index, unsigned int flags)
`head` is the head of the userspace struct robust_list_head, just as old
set_robust_list(). It needs to be a void pointer since it can point to a
normal robust_list_head or a compat_robust_list_head.
`flags` can be used for defining the list type:
enum robust_list_type {
ROBUST_LIST_32BIT,
ROBUST_LIST_64BIT,
};
`index` is the index in the internal robust_list's linked list (the
naming starts to get confusing, I reckon). If `index == -1`, that means
that user wants to set a new robust_list, and the kernel will append it
in the end of the list, assign a new index and return this index to the
user. If `index >= 0`, that means that user wants to re-set `*head` of
an already existing list (similarly to what happens when you call
set_robust_list() twice with different `*head`).
If `index` is out of range, or it points to a non-existing robust_list,
or if the internal list is full, an error is returned.
Unaligned `head` addresses are refused by the kernel with -EINVAL.
User cannot remove lists.
* Implementation
The old syscall's set/get_robust_list() are converted to use the linked
list as well. When using only the old syscalls user shouldn't any
difference as the internal code will handle the linked list insertion as
usual. When mixing old and new interfaces users should be aware that one
of the elements of the list was created by another syscall and they
should have special care handling this element index.
On exit, the linked list is parsed and all robust lists regardless of
which interface it was used to create them are handled.
Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
include/linux/futex.h | 5 +-
include/linux/sched.h | 5 +-
include/uapi/asm-generic/unistd.h | 4 +-
include/uapi/linux/futex.h | 24 +++++++
kernel/futex/core.c | 111 ++++++++++++++++++++++++------
kernel/futex/futex.h | 5 ++
kernel/futex/syscalls.c | 81 ++++++++++++++++++++--
7 files changed, 205 insertions(+), 30 deletions(-)
diff --git a/include/linux/futex.h b/include/linux/futex.h
index 8217b5ebdd9c..39335f21aea6 100644
--- a/include/linux/futex.h
+++ b/include/linux/futex.h
@@ -72,10 +72,11 @@ enum {
static inline void futex_init_task(struct task_struct *tsk)
{
- tsk->robust_list = NULL;
+ tsk->robust_list_index = -1;
#ifdef CONFIG_COMPAT
- tsk->compat_robust_list = NULL;
+ tsk->compat_robust_list_index = -1;
#endif
+ INIT_LIST_HEAD(&tsk->robust_list2);
INIT_LIST_HEAD(&tsk->pi_state_list);
tsk->pi_state_cache = NULL;
tsk->futex_state = FUTEX_STATE_OK;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 29e500d8d19d..903c1aedbe07 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1297,10 +1297,11 @@ struct task_struct {
u32 rmid;
#endif
#ifdef CONFIG_FUTEX
- struct robust_list_head __user *robust_list;
+ int robust_list_index;
#ifdef CONFIG_COMPAT
- struct robust_list_head32 __user *compat_robust_list;
+ int compat_robust_list_index;
#endif
+ struct list_head robust_list2;
struct list_head pi_state_list;
struct futex_pi_state *pi_state_cache;
struct mutex futex_exit_mutex;
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 88dc393c2bca..477cce02ed72 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -850,8 +850,10 @@ __SYSCALL(__NR_listxattrat, sys_listxattrat)
#define __NR_removexattrat 466
__SYSCALL(__NR_removexattrat, sys_removexattrat)
+#define __NR_set_robust_list2 467
+
#undef __NR_syscalls
-#define __NR_syscalls 467
+#define __NR_syscalls 468
/*
* 32 bit systems traditionally used different
diff --git a/include/uapi/linux/futex.h b/include/uapi/linux/futex.h
index d2ee625ea189..13903a278b71 100644
--- a/include/uapi/linux/futex.h
+++ b/include/uapi/linux/futex.h
@@ -146,6 +146,30 @@ struct robust_list_head {
struct robust_list __user *list_op_pending;
};
+#define ROBUST_LISTS_PER_TASK 10
+
+enum robust_list2_type {
+ ROBUST_LIST_32BIT,
+ ROBUST_LIST_64BIT,
+};
+
+#define ROBUST_LIST_TYPE_MASK (ROBUST_LIST_32BIT | ROBUST_LIST_64BIT)
+
+/*
+ * This is an entry of a linked list of robust lists.
+ *
+ * @head: can point to a 64bit list or a 32bit list
+ * @list_type: determine the size of the futex pointers in the list
+ * @index: the index of this entry in the list
+ * @list: linked list element
+ */
+struct robust_list2_entry {
+ void __user *head;
+ enum robust_list2_type list_type;
+ unsigned int index;
+ struct list_head list;
+};
+
/*
* Are there any waiters for this robust futex:
*/
diff --git a/kernel/futex/core.c b/kernel/futex/core.c
index 3d81a53c114c..07a7e5e9bc8d 100644
--- a/kernel/futex/core.c
+++ b/kernel/futex/core.c
@@ -776,9 +776,9 @@ static inline int fetch_robust_entry(struct robust_list __user **entry,
*
* We silently return on any sign of list-walking problem.
*/
-static void exit_robust_list64(struct task_struct *curr)
+static void exit_robust_list64(struct task_struct *curr,
+ struct robust_list_head __user *head)
{
- struct robust_list_head __user *head = curr->robust_list;
struct robust_list __user *entry, *next_entry, *pending;
unsigned int limit = ROBUST_LIST_LIMIT, pi, pip;
unsigned int next_pi;
@@ -838,7 +838,8 @@ static void exit_robust_list64(struct task_struct *curr)
}
}
#else
-static void exit_robust_list64(struct task_struct *curr)
+static void exit_robust_list64(struct task_struct *curr,
+ struct robust_list_head __user *head)
{
pr_warn("32bit kernel should not allow ROBUST_LIST_64BIT");
}
@@ -875,9 +876,9 @@ fetch_robust_entry32(u32 *uentry, struct robust_list __user **entry,
*
* We silently return on any sign of list-walking problem.
*/
-static void exit_robust_list32(struct task_struct *curr)
+static void exit_robust_list32(struct task_struct *curr,
+ struct robust_list_head32 __user *head)
{
- struct robust_list_head32 __user *head = curr->compat_robust_list;
struct robust_list __user *entry, *next_entry, *pending;
unsigned int limit = ROBUST_LIST_LIMIT, pi, pip;
unsigned int next_pi;
@@ -943,6 +944,70 @@ static void exit_robust_list32(struct task_struct *curr)
}
}
+long do_set_robust_list2(struct robust_list_head __user *head,
+ int index, unsigned int type)
+{
+ struct list_head *list2 = ¤t->robust_list2;
+ struct robust_list2_entry *prev, *new = NULL;
+
+ if (index == -1) {
+ if (list_empty(list2)) {
+ index = 0;
+ } else {
+ prev = list_last_entry(list2, struct robust_list2_entry, list);
+ index = prev->index + 1;
+ }
+
+ if (index >= ROBUST_LISTS_PER_TASK)
+ return -EINVAL;
+
+ new = kmalloc(sizeof(struct robust_list2_entry), GFP_KERNEL);
+ if (!new)
+ return -ENOMEM;
+
+ list_add_tail(&new->list, list2);
+ new->index = index;
+
+ } else if (index >= 0) {
+ struct robust_list2_entry *curr;
+
+ if (list_empty(list2))
+ return -ENOENT;
+
+ list_for_each_entry(curr, list2, list) {
+ if (index == curr->index) {
+ new = curr;
+ break;
+ }
+ }
+
+ if (!new)
+ return -ENOENT;
+ }
+
+ BUG_ON(!new);
+ new->head = head;
+ new->list_type = type;
+
+ return index;
+}
+
+struct robust_list_head __user *get_robust_list2(int index, struct task_struct *task)
+{
+ struct list_head *list2 = &task->robust_list2;
+ struct robust_list2_entry *curr;
+
+ if (list_empty(list2) || index == -1)
+ return NULL;
+
+ list_for_each_entry(curr, list2, list) {
+ if (index == curr->index)
+ return curr->head;
+ }
+
+ return NULL;
+}
+
#ifdef CONFIG_FUTEX_PI
/*
@@ -1024,24 +1089,28 @@ static inline void exit_pi_state_list(struct task_struct *curr) { }
static void futex_cleanup(struct task_struct *tsk)
{
-#ifdef CONFIG_64BIT
- if (unlikely(tsk->robust_list)) {
- exit_robust_list64(tsk);
- tsk->robust_list = NULL;
- }
-#else
- if (unlikely(tsk->robust_list)) {
- exit_robust_list32(tsk);
- tsk->robust_list = NULL;
- }
-#endif
+ struct robust_list2_entry *curr, *n;
+ struct list_head *list2 = &tsk->robust_list2;
-#ifdef CONFIG_COMPAT
- if (unlikely(tsk->compat_robust_list)) {
- exit_robust_list32(tsk);
- tsk->compat_robust_list = NULL;
+ /*
+ * Walk through the linked list, parsing robust lists and freeing the
+ * allocated lists
+ */
+ if (unlikely(!list_empty(list2))) {
+ list_for_each_entry_safe(curr, n, list2, list) {
+ if (curr->head != NULL) {
+ if (curr->list_type == ROBUST_LIST_64BIT)
+ exit_robust_list64(tsk, curr->head);
+ else if (curr->list_type == ROBUST_LIST_32BIT)
+ exit_robust_list32(tsk, curr->head);
+ curr->head = NULL;
+ }
+ list_del_init(&curr->list);
+ kfree(curr);
+ }
}
-#endif
+
+ tsk->robust_list_index = -1;
if (unlikely(!list_empty(&tsk->pi_state_list)))
exit_pi_state_list(tsk);
diff --git a/kernel/futex/futex.h b/kernel/futex/futex.h
index 6b2f4c7eb720..b8c20deb5552 100644
--- a/kernel/futex/futex.h
+++ b/kernel/futex/futex.h
@@ -409,6 +409,11 @@ extern int __futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
extern int futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
ktime_t *abs_time, u32 bitset);
+extern long do_set_robust_list2(struct robust_list_head __user *head,
+ int index, unsigned int type);
+
+extern struct robust_list_head __user *get_robust_list2(int index, struct task_struct *task);
+
/**
* struct futex_vector - Auxiliary struct for futex_waitv()
* @w: Userspace provided data
diff --git a/kernel/futex/syscalls.c b/kernel/futex/syscalls.c
index dba193dfd216..56ee1123cbd8 100644
--- a/kernel/futex/syscalls.c
+++ b/kernel/futex/syscalls.c
@@ -20,6 +20,18 @@
* the list. There can only be one such pending lock.
*/
+#ifdef CONFIG_64BIT
+static inline int robust_list_native_type(void)
+{
+ return ROBUST_LIST_64BIT;
+}
+#else
+static inline int robust_list_native_type(void)
+{
+ return ROBUST_LIST_32BIT;
+}
+#endif
+
/**
* sys_set_robust_list() - Set the robust-futex list head of a task
* @head: pointer to the list-head
@@ -28,17 +40,63 @@
SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head,
size_t, len)
{
+ unsigned int type = robust_list_native_type();
+ int ret;
+
/*
* The kernel knows only one size for now:
*/
if (unlikely(len != sizeof(*head)))
return -EINVAL;
- current->robust_list = head;
+ ret = do_set_robust_list2(head, current->robust_list_index, type);
+ if (ret < 0)
+ return ret;
+
+ current->robust_list_index = ret;
return 0;
}
+#define ROBUST_LIST_FLAGS ROBUST_LIST_TYPE_MASK
+
+/*
+ * sys_set_robust_list2()
+ *
+ * When index == -1, create a new list for user. When index >= 0, try to find
+ * the corresponding list and re-set the head there.
+ *
+ * Return values:
+ * >= 0: success, index of the robust list
+ * -EINVAL: invalid flags, invalid index
+ * -ENOENT: requested index no where to be found
+ * -ENOMEM: error allocating new list
+ * -ESRCH: too many allocated lists
+ */
+SYSCALL_DEFINE3(set_robust_list2, struct robust_list_head __user *, head,
+ int, index, unsigned int, flags)
+{
+ unsigned int type;
+
+ type = flags & ROBUST_LIST_TYPE_MASK;
+
+ if (index < -1 || index >= ROBUST_LISTS_PER_TASK)
+ return -EINVAL;
+
+ if ((flags & ~ROBUST_LIST_FLAGS) != 0)
+ return -EINVAL;
+
+ if (((uintptr_t) head % sizeof(u32)) != 0)
+ return -EINVAL;
+
+#ifndef CONFIG_64BIT
+ if (type == ROBUST_LIST_64BIT)
+ return -EINVAL;
+#endif
+
+ return do_set_robust_list2(head, index, type);
+}
+
/**
* sys_get_robust_list() - Get the robust-futex list head of a task
* @pid: pid of the process [zero for current task]
@@ -52,6 +110,7 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
struct robust_list_head __user *head;
unsigned long ret;
struct task_struct *p;
+ int index;
rcu_read_lock();
@@ -68,9 +127,11 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
goto err_unlock;
- head = p->robust_list;
+ index = p->robust_list_index;
rcu_read_unlock();
+ head = get_robust_list2(index, p);
+
if (put_user(sizeof(*head), len_ptr))
return -EFAULT;
return put_user(head, head_ptr);
@@ -443,10 +504,19 @@ COMPAT_SYSCALL_DEFINE2(set_robust_list,
struct robust_list_head32 __user *, head,
compat_size_t, len)
{
+ unsigned int type = ROBUST_LIST_32BIT;
+ int ret;
+
if (unlikely(len != sizeof(*head)))
return -EINVAL;
- current->compat_robust_list = head;
+ ret = do_set_robust_list2((struct robust_list_head __user *) head,
+ current->robust_list_index, type);
+ if (ret < 0)
+ return ret;
+
+ current->robust_list_index = ret;
+
return 0;
}
@@ -458,6 +528,7 @@ COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
struct robust_list_head32 __user *head;
unsigned long ret;
struct task_struct *p;
+ int index;
rcu_read_lock();
@@ -474,9 +545,11 @@ COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
goto err_unlock;
- head = p->compat_robust_list;
+ index = p->compat_robust_list_index;
rcu_read_unlock();
+ head = (struct robust_list_head32 __user *) get_robust_list2(index, p);
+
if (put_user(sizeof(*head), len_ptr))
return -EFAULT;
return put_user(ptr_to_compat(head), head_ptr);
--
2.48.1
^ permalink raw reply related
* [PATCH v4 0/5] futex: Create set_robust_list2
From: André Almeida @ 2025-02-25 18:35 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Arnd Bergmann, sonicadvance1
Cc: linux-kernel, kernel-dev, linux-api, Vinicius Peixoto,
Sebastian Andrzej Siewior, André Almeida
This patch adds a new robust_list() syscall. The current syscall
can't be expanded to cover the following use case, so a new one is
needed. This new syscall allows users to set multiple robust lists per
process and to have either 32bit or 64bit pointers in the list.
* Use case
FEX-Emu[1] is an application that runs x86 and x86-64 binaries on an
AArch64 Linux host. One of the tasks of FEX-Emu is to translate syscalls
from one platform to another. Existing set_robust_list() can't be easily
translated because of two limitations:
1) x86 apps can have 32bit pointers robust lists. For a x86-64 kernel
this is not a problem, because of the compat entry point. But there's
no such compat entry point for AArch64, so the kernel would do the
pointer arithmetic wrongly. Is also unviable to userspace to keep
track every addition/removal to the robust list and keep a 64bit
version of it somewhere else to feed the kernel. Thus, the new
interface has an option of telling the kernel if the list is filled
with 32bit or 64bit pointers.
2) Apps can set just one robust list (in theory, x86-64 can set two if
they also use the compat entry point). That means that when a x86 app
asks FEX-Emu to call set_robust_list(), FEX have two options: to
overwrite their own robust list pointer and make the app robust, or
to ignore the app robust list and keep the emulator robust. The new
interface allows for multiple robust lists per application, solving
this.
* Interface
This is the proposed interface:
long set_robust_list2(void *head, int index, unsigned int flags)
`head` is the head of the userspace struct robust_list_head, just as old
set_robust_list(). It needs to be a void pointer since it can point to a normal
robust_list_head or a compat_robust_list_head.
`flags` can be used for defining the list type:
enum robust_list_type {
ROBUST_LIST_32BIT,
ROBUST_LIST_64BIT,
};
`index` is the index in the internal robust_list's linked list (the naming
starts to get confusing, I reckon). If `index == -1`, that means that user wants
to set a new robust_list, and the kernel will append it in the end of the list,
assign a new index and return this index to the user. If `index >= 0`, that
means that user wants to re-set `*head` of an already existing list (similarly
to what happens when you call set_robust_list() twice with different `*head`).
If `index` is out of range, or it points to a non-existing robust_list, or if
the internal list is full, an error is returned.
* Implementation
The implementation re-uses most of the existing robust list interface as
possible. The new task_struct member `struct list_head robust_list2` is just a
linked list where new lists are appended as the user requests more lists, and by
futex_cleanup(), the kernel walks through the internal list feeding
exit_robust_list() with the robust_list's.
This implementation supports up to 10 lists (defined at ROBUST_LISTS_PER_TASK),
but it was an arbitrary number for this RFC. For the described use case above, 4
should be enough, I'm not sure which should be the limit.
It doesn't support list removal (should it support?). It doesn't have a proper
get_robust_list2() yet as well, but I can add it in a next revision. We could
also have a generic robust_list() syscall that can be used to set/get and be
controlled by flags.
The new interface has a `unsigned int flags` argument, making it
extensible for future use cases as well.
It refuses unaligned `head` addresses. It doesn't have a limit for elements in a
single list (like ROBUST_LIST_LIMIT), it destroys the list as it is parsed to be
safe against circular lists.
* Testing
This patcheset has a selftest patch that expands this one:
https://lore.kernel.org/lkml/20250212131123.37431-1-andrealmeid@igalia.com/
Also, FEX-Emu added support for this interface to validate it:
https://github.com/FEX-Emu/FEX/pull/3966
Feedback is very welcomed!
Thanks,
André
[1] https://github.com/FEX-Emu/FEX
Changelog:
- Refuse unaligned head pointers
- Ignore ROBUST_LIST_LIMIT for lists created with this interface and make it
robust against circular lists
- Fix a get_robust_list() syscall bug for getting the list from another thread
- Adapt selftest to use the new interface
v3: https://lore.kernel.org/lkml/20241217174958.477692-1-andrealmeid@igalia.com/
- Old syscall set_robust_list() adds new head to the internal linked list of
robust lists pointers, instead of having a field just for them. Remove
tsk->robust_list and use only tsk->robust_list2
v2: https://lore.kernel.org/lkml/20241101162147.284993-1-andrealmeid@igalia.com/
- Added a patch to properly deal with exit_robust_list() in 64bit vs 32bit
- Wired-up syscall for all archs
- Added more of the cover letter to the commit message
v1: https://lore.kernel.org/lkml/20241024145735.162090-1-andrealmeid@igalia.com/
André Almeida (5):
futex: Use explicit sizes for compat_exit_robust_list
futex: Create set_robust_list2
futex: Wire up set_robust_list2 syscall
futex: Remove the limit of elements for sys_set_robust_list2 lists
selftests: futex: Expand robust list test for the new interface
arch/alpha/kernel/syscalls/syscall.tbl | 1 +
arch/arm/tools/syscall.tbl | 1 +
arch/m68k/kernel/syscalls/syscall.tbl | 1 +
arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 1 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 1 +
arch/parisc/kernel/syscalls/syscall.tbl | 1 +
arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
arch/s390/kernel/syscalls/syscall.tbl | 1 +
arch/sh/kernel/syscalls/syscall.tbl | 1 +
arch/sparc/kernel/syscalls/syscall.tbl | 1 +
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
include/linux/compat.h | 12 +-
include/linux/futex.h | 16 +-
include/linux/sched.h | 5 +-
include/uapi/asm-generic/unistd.h | 4 +-
include/uapi/linux/futex.h | 24 +++
kernel/futex/core.c | 165 ++++++++++++++----
kernel/futex/futex.h | 5 +
kernel/futex/syscalls.c | 85 ++++++++-
kernel/sys_ni.c | 1 +
scripts/syscall.tbl | 1 +
.../selftests/futex/functional/robust_list.c | 160 ++++++++++++++++-
26 files changed, 436 insertions(+), 57 deletions(-)
--
2.48.1
^ permalink raw reply
* [PATCH v4 5/5] selftests: futex: Expand robust list test for the new interface
From: André Almeida @ 2025-02-25 18:35 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Arnd Bergmann, sonicadvance1
Cc: linux-kernel, kernel-dev, linux-api, Vinicius Peixoto,
Sebastian Andrzej Siewior, André Almeida
In-Reply-To: <20250225183531.682556-1-andrealmeid@igalia.com>
Expand the current robust list test for the new set_robust_list2
syscall. Create an option to make it possible to run the same tests
using the new syscall, and also add two new relevant test: test long
lists (bigger than ROBUST_LIST_LIMIT) and for unaligned addresses.
Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
.../selftests/futex/functional/robust_list.c | 160 +++++++++++++++++-
1 file changed, 156 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/futex/functional/robust_list.c b/tools/testing/selftests/futex/functional/robust_list.c
index 42690b2440fd..201acbeeac5a 100644
--- a/tools/testing/selftests/futex/functional/robust_list.c
+++ b/tools/testing/selftests/futex/functional/robust_list.c
@@ -35,16 +35,45 @@
#include <stddef.h>
#include <sys/mman.h>
#include <sys/wait.h>
+#include <stdint.h>
#define STACK_SIZE (1024 * 1024)
#define FUTEX_TIMEOUT 3
+#define SYS_set_robust_list2 467
+
+enum robust_list2_type {
+ ROBUST_LIST_32BIT,
+ ROBUST_LIST_64BIT,
+};
+
static pthread_barrier_t barrier, barrier2;
+bool robust2 = false;
+
int set_robust_list(struct robust_list_head *head, size_t len)
{
- return syscall(SYS_set_robust_list, head, len);
+ int ret, flags;
+
+ if (!robust2) {
+ return syscall(SYS_set_robust_list, head, len);
+ }
+
+ if (sizeof(head) == 8)
+ flags = ROBUST_LIST_64BIT;
+ else
+ flags = ROBUST_LIST_32BIT;
+
+ /*
+ * We act as we have just one list here. We try to use the first slot,
+ * but if it hasn't been alocated yet we allocate it.
+ */
+ ret = syscall(SYS_set_robust_list2, head, 0, flags);
+ if (ret == -1 && errno == ENOENT)
+ ret = syscall(SYS_set_robust_list2, head, -1, flags);
+
+ return ret;
}
int get_robust_list(int pid, struct robust_list_head **head, size_t *len_ptr)
@@ -246,6 +275,11 @@ static void test_set_robust_list_invalid_size(void)
size_t head_size = sizeof(struct robust_list_head);
int ret;
+ if (robust2) {
+ ksft_test_result_skip("This test is only for old robust interface\n");
+ return;
+ }
+
ret = set_robust_list(&head, head_size);
ASSERT_EQ(ret, 0);
@@ -321,6 +355,11 @@ static void test_get_robust_list_child(void)
struct robust_list_head head, *get_head;
size_t len_ptr;
+ if (robust2) {
+ ksft_test_result_skip("Not implemented in the new robust interface\n");
+ return;
+ }
+
ret = pthread_barrier_init(&barrier, NULL, 2);
ret = pthread_barrier_init(&barrier2, NULL, 2);
ASSERT_EQ(ret, 0);
@@ -332,7 +371,7 @@ static void test_get_robust_list_child(void)
ret = get_robust_list(tid, &get_head, &len_ptr);
ASSERT_EQ(ret, 0);
- ASSERT_EQ(&head, get_head);
+ ASSERT_EQ(get_head, &head);
pthread_barrier_wait(&barrier2);
@@ -507,11 +546,119 @@ static void test_circular_list(void)
ksft_test_result_pass("%s\n", __func__);
}
+#define ROBUST_LIST_LIMIT 2048
+#define CHILD_LIST_LIMIT (ROBUST_LIST_LIMIT + 10)
+
+static int child_robust_list_limit(void *arg)
+{
+ struct lock_struct *locks;
+ struct robust_list *list;
+ struct robust_list_head head;
+ int ret, i;
+
+ locks = (struct lock_struct *) arg;
+
+ ret = set_list(&head);
+ if (ret)
+ ksft_test_result_fail("set_list error\n");
+
+ /*
+ * Create a very long list of locks
+ */
+ head.list.next = &locks[0].list;
+
+ list = head.list.next;
+ for (i = 0; i < CHILD_LIST_LIMIT - 1; i++) {
+ list->next = &locks[i+1].list;
+ list = list->next;
+ }
+ list->next = &head.list;
+
+ /*
+ * Grab the lock in the last one, and die without releasing it
+ */
+ mutex_lock(&locks[CHILD_LIST_LIMIT], &head, false);
+ pthread_barrier_wait(&barrier);
+
+ sleep(1);
+
+ return 0;
+}
+
+/*
+ * The old robust list used to have a limit of 2048 items from the kernel side.
+ * After this limit the kernel stops walking the list and ignore the other
+ * futexes, causing deadlocks.
+ *
+ * For the new interface, test if we can wait for a list of more than 2048
+ * elements.
+ */
+static void test_robust_list_limit(void)
+{
+ struct lock_struct locks[CHILD_LIST_LIMIT + 1];
+ _Atomic(unsigned int) *futex = &locks[CHILD_LIST_LIMIT].futex;
+ struct robust_list_head head;
+ int ret;
+
+ if (!robust2) {
+ ksft_test_result_skip("This test is only for new robust interface\n");
+ return;
+ }
+
+ *futex = 0;
+
+ ret = set_list(&head);
+ ASSERT_EQ(ret, 0);
+
+ ret = pthread_barrier_init(&barrier, NULL, 2);
+ ASSERT_EQ(ret, 0);
+
+ create_child(child_robust_list_limit, locks);
+
+ /*
+ * After the child thread creates the very long list of locks, wait on
+ * the last one.
+ */
+ pthread_barrier_wait(&barrier);
+ ret = mutex_lock(&locks[CHILD_LIST_LIMIT], &head, false);
+
+ if (ret != 0)
+ printf("futex wait returned %d\n", errno);
+ ASSERT_EQ(ret, 0);
+
+ ASSERT_TRUE(*futex | FUTEX_OWNER_DIED);
+
+ wait(NULL);
+ pthread_barrier_destroy(&barrier);
+
+ ksft_test_result_pass("%s\n", __func__);
+}
+
+/*
+ * The kernel should refuse an unaligned head pointer
+ */
+static void test_unaligned_address(void)
+{
+ struct robust_list_head head, *h;
+ int ret;
+
+ if (!robust2) {
+ ksft_test_result_skip("This test is only for new robust interface\n");
+ return;
+ }
+
+ h = (struct robust_list_head *) ((uintptr_t) &head + 1);
+ ret = set_list(h);
+ ASSERT_EQ(ret, -1);
+ ASSERT_EQ(errno, EINVAL);
+}
+
void usage(char *prog)
{
printf("Usage: %s\n", prog);
printf(" -c Use color\n");
printf(" -h Display this help message\n");
+ printf(" -n Use robust2 syscall\n");
printf(" -v L Verbosity level: %d=QUIET %d=CRITICAL %d=INFO\n",
VQUIET, VCRITICAL, VINFO);
}
@@ -520,7 +667,7 @@ int main(int argc, char *argv[])
{
int c;
- while ((c = getopt(argc, argv, "cht:v:")) != -1) {
+ while ((c = getopt(argc, argv, "chnt:v:")) != -1) {
switch (c) {
case 'c':
log_color(1);
@@ -531,6 +678,9 @@ int main(int argc, char *argv[])
case 'v':
log_verbosity(atoi(optarg));
break;
+ case 'n':
+ robust2 = true;
+ break;
default:
usage(basename(argv[0]));
exit(1);
@@ -538,7 +688,7 @@ int main(int argc, char *argv[])
}
ksft_print_header();
- ksft_set_plan(7);
+ ksft_set_plan(8);
test_robustness();
@@ -548,6 +698,8 @@ int main(int argc, char *argv[])
test_set_list_op_pending();
test_robust_list_multiple_elements();
test_circular_list();
+ test_robust_list_limit();
+ test_unaligned_address();
ksft_print_cnts();
return 0;
--
2.48.1
^ permalink raw reply related
* [PATCH v4 1/5] futex: Use explicit sizes for compat_exit_robust_list
From: André Almeida @ 2025-02-25 18:35 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Arnd Bergmann, sonicadvance1
Cc: linux-kernel, kernel-dev, linux-api, Vinicius Peixoto,
Sebastian Andrzej Siewior, André Almeida
In-Reply-To: <20250225183531.682556-1-andrealmeid@igalia.com>
There are two functions for handling robust lists during the task
exit: exit_robust_list() and compat_exit_robust_list(). The first one
handles either 64bit or 32bit lists, depending if it's a 64bit or 32bit
kernel. The compat_exit_robust_list() only exists in 64bit kernels that
supports 32bit syscalls, and handles 32bit lists.
For the new syscall set_robust_list2(), 64bit kernels need to be able to
handle 32bit lists despite having or not support for 32bit syscalls, so
make compat_exit_robust_list() exist regardless of compat_ config.
Also, use explicitly sizing, otherwise in a 32bit kernel both
exit_robust_list() and compat_exit_robust_list() would be the exactly
same function, with none of them dealing with 64bit robust lists.
Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
include/linux/compat.h | 12 +----------
include/linux/futex.h | 11 +++++++++++
include/linux/sched.h | 2 +-
kernel/futex/core.c | 44 ++++++++++++++++++++++++++---------------
kernel/futex/syscalls.c | 4 ++--
5 files changed, 43 insertions(+), 30 deletions(-)
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 56cebaff0c91..968a9135ff48 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -385,16 +385,6 @@ struct compat_ifconf {
compat_caddr_t ifcbuf;
};
-struct compat_robust_list {
- compat_uptr_t next;
-};
-
-struct compat_robust_list_head {
- struct compat_robust_list list;
- compat_long_t futex_offset;
- compat_uptr_t list_op_pending;
-};
-
#ifdef CONFIG_COMPAT_OLD_SIGACTION
struct compat_old_sigaction {
compat_uptr_t sa_handler;
@@ -672,7 +662,7 @@ asmlinkage long compat_sys_waitid(int, compat_pid_t,
struct compat_siginfo __user *, int,
struct compat_rusage __user *);
asmlinkage long
-compat_sys_set_robust_list(struct compat_robust_list_head __user *head,
+compat_sys_set_robust_list(struct robust_list_head32 __user *head,
compat_size_t len);
asmlinkage long
compat_sys_get_robust_list(int pid, compat_uptr_t __user *head_ptr,
diff --git a/include/linux/futex.h b/include/linux/futex.h
index b70df27d7e85..8217b5ebdd9c 100644
--- a/include/linux/futex.h
+++ b/include/linux/futex.h
@@ -53,6 +53,17 @@ union futex_key {
#define FUTEX_KEY_INIT (union futex_key) { .both = { .ptr = 0ULL } }
#ifdef CONFIG_FUTEX
+
+struct robust_list32 {
+ u32 next;
+};
+
+struct robust_list_head32 {
+ struct robust_list32 list;
+ s32 futex_offset;
+ u32 list_op_pending;
+};
+
enum {
FUTEX_STATE_OK,
FUTEX_STATE_EXITING,
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 9632e3318e0d..29e500d8d19d 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1299,7 +1299,7 @@ struct task_struct {
#ifdef CONFIG_FUTEX
struct robust_list_head __user *robust_list;
#ifdef CONFIG_COMPAT
- struct compat_robust_list_head __user *compat_robust_list;
+ struct robust_list_head32 __user *compat_robust_list;
#endif
struct list_head pi_state_list;
struct futex_pi_state *pi_state_cache;
diff --git a/kernel/futex/core.c b/kernel/futex/core.c
index 3db8567f5a44..3d81a53c114c 100644
--- a/kernel/futex/core.c
+++ b/kernel/futex/core.c
@@ -769,13 +769,14 @@ static inline int fetch_robust_entry(struct robust_list __user **entry,
return 0;
}
+#ifdef CONFIG_64BIT
/*
* Walk curr->robust_list (very carefully, it's a userspace list!)
* and mark any locks found there dead, and notify any waiters.
*
* We silently return on any sign of list-walking problem.
*/
-static void exit_robust_list(struct task_struct *curr)
+static void exit_robust_list64(struct task_struct *curr)
{
struct robust_list_head __user *head = curr->robust_list;
struct robust_list __user *entry, *next_entry, *pending;
@@ -836,8 +837,13 @@ static void exit_robust_list(struct task_struct *curr)
curr, pip, HANDLE_DEATH_PENDING);
}
}
+#else
+static void exit_robust_list64(struct task_struct *curr)
+{
+ pr_warn("32bit kernel should not allow ROBUST_LIST_64BIT");
+}
+#endif
-#ifdef CONFIG_COMPAT
static void __user *futex_uaddr(struct robust_list __user *entry,
compat_long_t futex_offset)
{
@@ -851,13 +857,13 @@ static void __user *futex_uaddr(struct robust_list __user *entry,
* Fetch a robust-list pointer. Bit 0 signals PI futexes:
*/
static inline int
-compat_fetch_robust_entry(compat_uptr_t *uentry, struct robust_list __user **entry,
- compat_uptr_t __user *head, unsigned int *pi)
+fetch_robust_entry32(u32 *uentry, struct robust_list __user **entry,
+ u32 __user *head, unsigned int *pi)
{
if (get_user(*uentry, head))
return -EFAULT;
- *entry = compat_ptr((*uentry) & ~1);
+ *entry = (void __user *)(unsigned long)((*uentry) & ~1);
*pi = (unsigned int)(*uentry) & 1;
return 0;
@@ -869,21 +875,21 @@ compat_fetch_robust_entry(compat_uptr_t *uentry, struct robust_list __user **ent
*
* We silently return on any sign of list-walking problem.
*/
-static void compat_exit_robust_list(struct task_struct *curr)
+static void exit_robust_list32(struct task_struct *curr)
{
- struct compat_robust_list_head __user *head = curr->compat_robust_list;
+ struct robust_list_head32 __user *head = curr->compat_robust_list;
struct robust_list __user *entry, *next_entry, *pending;
unsigned int limit = ROBUST_LIST_LIMIT, pi, pip;
unsigned int next_pi;
- compat_uptr_t uentry, next_uentry, upending;
- compat_long_t futex_offset;
+ u32 uentry, next_uentry, upending;
+ s32 futex_offset;
int rc;
/*
* Fetch the list head (which was registered earlier, via
* sys_set_robust_list()):
*/
- if (compat_fetch_robust_entry(&uentry, &entry, &head->list.next, &pi))
+ if (fetch_robust_entry32((u32 *)&uentry, &entry, (u32 *)&head->list.next, &pi))
return;
/*
* Fetch the relative futex offset:
@@ -894,7 +900,7 @@ static void compat_exit_robust_list(struct task_struct *curr)
* Fetch any possibly pending lock-add first, and handle it
* if it exists:
*/
- if (compat_fetch_robust_entry(&upending, &pending,
+ if (fetch_robust_entry32(&upending, &pending,
&head->list_op_pending, &pip))
return;
@@ -904,8 +910,8 @@ static void compat_exit_robust_list(struct task_struct *curr)
* Fetch the next entry in the list before calling
* handle_futex_death:
*/
- rc = compat_fetch_robust_entry(&next_uentry, &next_entry,
- (compat_uptr_t __user *)&entry->next, &next_pi);
+ rc = fetch_robust_entry32(&next_uentry, &next_entry,
+ (u32 __user *)&entry->next, &next_pi);
/*
* A pending lock might already be on the list, so
* dont process it twice:
@@ -936,7 +942,6 @@ static void compat_exit_robust_list(struct task_struct *curr)
handle_futex_death(uaddr, curr, pip, HANDLE_DEATH_PENDING);
}
}
-#endif
#ifdef CONFIG_FUTEX_PI
@@ -1019,14 +1024,21 @@ static inline void exit_pi_state_list(struct task_struct *curr) { }
static void futex_cleanup(struct task_struct *tsk)
{
+#ifdef CONFIG_64BIT
if (unlikely(tsk->robust_list)) {
- exit_robust_list(tsk);
+ exit_robust_list64(tsk);
tsk->robust_list = NULL;
}
+#else
+ if (unlikely(tsk->robust_list)) {
+ exit_robust_list32(tsk);
+ tsk->robust_list = NULL;
+ }
+#endif
#ifdef CONFIG_COMPAT
if (unlikely(tsk->compat_robust_list)) {
- compat_exit_robust_list(tsk);
+ exit_robust_list32(tsk);
tsk->compat_robust_list = NULL;
}
#endif
diff --git a/kernel/futex/syscalls.c b/kernel/futex/syscalls.c
index 4b6da9116aa6..dba193dfd216 100644
--- a/kernel/futex/syscalls.c
+++ b/kernel/futex/syscalls.c
@@ -440,7 +440,7 @@ SYSCALL_DEFINE4(futex_requeue,
#ifdef CONFIG_COMPAT
COMPAT_SYSCALL_DEFINE2(set_robust_list,
- struct compat_robust_list_head __user *, head,
+ struct robust_list_head32 __user *, head,
compat_size_t, len)
{
if (unlikely(len != sizeof(*head)))
@@ -455,7 +455,7 @@ COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
compat_uptr_t __user *, head_ptr,
compat_size_t __user *, len_ptr)
{
- struct compat_robust_list_head __user *head;
+ struct robust_list_head32 __user *head;
unsigned long ret;
struct task_struct *p;
--
2.48.1
^ permalink raw reply related
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: David Hildenbrand @ 2025-02-25 16:48 UTC (permalink / raw)
To: Lorenzo Stoakes, Vlastimil Babka
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Paul E . McKenney, Jann Horn, linux-mm,
linux-kernel, Shuah Khan, linux-kselftest, linux-api,
John Hubbard, Juan Yescas, Kalesh Singh
In-Reply-To: <3102ab3b-67b6-4047-9db7-e83c3b9c1c53@lucifer.local>
>> As for compatibility with VM_LOCKONFAULT, do we need a new
>> MADV_GUARD_INSTALL_LOCKED or can we say MADV_GUARD_INSTALL is new enough
>> that it can be just retrofitted (like you retrofit file backed mappings)?
>> AFAIU the only risk would be breaking somebody that already relies on a
>> failure for VM_LOCKONFAULT, and it's unlikely there's such a somebody now.
>>
>>
>
> Hmm yeah I suppose. I guess just to be consistent with the other _LOCKED
> variants? (which seem to be... undocumented at least in man pages :P, and yes I
> realise this is me semi-volunteering to do that obviously...).
>
> But on the other hand, we could also expand this if you and I see also Dave feel
> this makes sense and wouldn't be confusing.
Just my 2 cents: one thing that came to mind: an existing library would
have to be updated to use the _LOCKED variant if the app would be using
mlockall(future), which is a bit unfortunate -- and if it could be
avoided, it would be great.
But yeah, devil is in the detail ...
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: Lorenzo Stoakes @ 2025-02-25 16:37 UTC (permalink / raw)
To: Vlastimil Babka
Cc: David Hildenbrand, Andrew Morton, Suren Baghdasaryan,
Liam R . Howlett, Matthew Wilcox, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Shuah Khan, linux-kselftest, linux-api,
John Hubbard, Juan Yescas, Kalesh Singh
In-Reply-To: <e0954e13-2c7d-447c-ba86-19875c74bc3b@suse.cz>
On Tue, Feb 25, 2025 at 04:54:22PM +0100, Vlastimil Babka wrote:
> On 2/18/25 18:28, Lorenzo Stoakes wrote:
> > On Tue, Feb 18, 2025 at 06:25:35PM +0100, David Hildenbrand wrote:
> >>
> >> > > >
> >> > > > It fails because it tries to 'touch' the memory, but 'touching' guard
> >> > > > region memory causes a segfault. This kind of breaks the idea of
> >> > > > mlock()'ing guard regions.
> >> > > >
> >> > > > I think adding workarounds to make this possible in any way is not really
> >> > > > worth it (and would probably be pretty gross).
> >> > > >
> >> > > > We already document that 'mlock()ing lightweight guard regions will fail'
> >> > > > as per man page so this is all in line with that.
> >> > >
> >> > > Right, and I claim that supporting VM_LOCKONFAULT might likely be as easy as
> >> > > allowing install/remove of guard regions when that flag is set.
> >> >
> >> > We already allow this flag! VM_LOCKED and VM_HUGETLB are the only flags we
> >> > disallow.
> >>
> >>
> >> See mlock2();
> >>
> >> SYSCALL_DEFINE3(mlock2, unsigned long, start, size_t, len, int, flags)
> >> {
> >> vm_flags_t vm_flags = VM_LOCKED;
> >>
> >> if (flags & ~MLOCK_ONFAULT)
> >> return -EINVAL;
> >>
> >> if (flags & MLOCK_ONFAULT)
> >> vm_flags |= VM_LOCKONFAULT;
> >>
> >> return do_mlock(start, len, vm_flags);
> >> }
> >>
> >>
> >> VM_LOCKONFAULT always as VM_LOCKED set as well.
> >
> > OK cool, that makes sense.
> >
> > As with much kernel stuff, I knew this in the past. Then I forgot. Then I knew
> > again, then... :P if only somebody would write it down in a book...
> >
> > Yeah then that makes sense to check explicitly for (VM_LOCKED | VM_LOCKONFAULT)
> > in any MADV_GUARD_INSTALL_LOCKED variant as obviously this would be passively
> > excluded right now.
>
> Sorry for the late reply. So AFAIU from your conversations, guards can't be
> compatible with VM_LOCKED, which means e.g. any attempts of glibc to use
> guards for stacks will soon discover that mlockall() users exist and are
> broken by this, and the attempts will fail? That's a bummer.
>
Yeah damn, this pushes up the priority on this.
Yeah unfortunately we cannot support this with guard regions being installed
_after_ the mlockall() but can for before.
Let me write this on my eternal whiteboard of doom (TM), because that ups the
priority on this. I want to have a good think and see if it might after all be
possible to find a way to make things work here for sake of this case.
This thing is already shipped now, so it is inevitably going to be an add-on.
I will try some experiments when I get a sec.
Thanks very much for bringing this point up! This is pretty key.
> As for compatibility with VM_LOCKONFAULT, do we need a new
> MADV_GUARD_INSTALL_LOCKED or can we say MADV_GUARD_INSTALL is new enough
> that it can be just retrofitted (like you retrofit file backed mappings)?
> AFAIU the only risk would be breaking somebody that already relies on a
> failure for VM_LOCKONFAULT, and it's unlikely there's such a somebody now.
>
>
Hmm yeah I suppose. I guess just to be consistent with the other _LOCKED
variants? (which seem to be... undocumented at least in man pages :P, and yes I
realise this is me semi-volunteering to do that obviously...).
But on the other hand, we could also expand this if you and I see also Dave feel
this makes sense and wouldn't be confusing.
Agreed entirely that it'd be very very odd for a user to rely on that so I think
we'll be fine.
I shall return to this topic later, in the form of a series, probably!
Cheers!
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: David Hildenbrand @ 2025-02-25 16:31 UTC (permalink / raw)
To: Vlastimil Babka, Lorenzo Stoakes
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Paul E . McKenney, Jann Horn, linux-mm,
linux-kernel, Shuah Khan, linux-kselftest, linux-api,
John Hubbard, Juan Yescas, Kalesh Singh
In-Reply-To: <e0954e13-2c7d-447c-ba86-19875c74bc3b@suse.cz>
On 25.02.25 16:54, Vlastimil Babka wrote:
> On 2/18/25 18:28, Lorenzo Stoakes wrote:
>> On Tue, Feb 18, 2025 at 06:25:35PM +0100, David Hildenbrand wrote:
>>>
>>>>>>
>>>>>> It fails because it tries to 'touch' the memory, but 'touching' guard
>>>>>> region memory causes a segfault. This kind of breaks the idea of
>>>>>> mlock()'ing guard regions.
>>>>>>
>>>>>> I think adding workarounds to make this possible in any way is not really
>>>>>> worth it (and would probably be pretty gross).
>>>>>>
>>>>>> We already document that 'mlock()ing lightweight guard regions will fail'
>>>>>> as per man page so this is all in line with that.
>>>>>
>>>>> Right, and I claim that supporting VM_LOCKONFAULT might likely be as easy as
>>>>> allowing install/remove of guard regions when that flag is set.
>>>>
>>>> We already allow this flag! VM_LOCKED and VM_HUGETLB are the only flags we
>>>> disallow.
>>>
>>>
>>> See mlock2();
>>>
>>> SYSCALL_DEFINE3(mlock2, unsigned long, start, size_t, len, int, flags)
>>> {
>>> vm_flags_t vm_flags = VM_LOCKED;
>>>
>>> if (flags & ~MLOCK_ONFAULT)
>>> return -EINVAL;
>>>
>>> if (flags & MLOCK_ONFAULT)
>>> vm_flags |= VM_LOCKONFAULT;
>>>
>>> return do_mlock(start, len, vm_flags);
>>> }
>>>
>>>
>>> VM_LOCKONFAULT always as VM_LOCKED set as well.
>>
>> OK cool, that makes sense.
>>
>> As with much kernel stuff, I knew this in the past. Then I forgot. Then I knew
>> again, then... :P if only somebody would write it down in a book...
>>
>> Yeah then that makes sense to check explicitly for (VM_LOCKED | VM_LOCKONFAULT)
>> in any MADV_GUARD_INSTALL_LOCKED variant as obviously this would be passively
>> excluded right now.
>
> Sorry for the late reply. So AFAIU from your conversations, guards can't be
> compatible with VM_LOCKED, which means e.g. any attempts of glibc to use
> guards for stacks will soon discover that mlockall() users exist and are
> broken by this, and the attempts will fail? That's a bummer.
>
> As for compatibility with VM_LOCKONFAULT, do we need a new
> MADV_GUARD_INSTALL_LOCKED or can we say MADV_GUARD_INSTALL is new enough
> that it can be just retrofitted (like you retrofit file backed mappings)?
> AFAIU the only risk would be breaking somebody that already relies on a
> failure for VM_LOCKONFAULT, and it's unlikely there's such a somebody now.
Exactly my thinking I didn't have time to phrase during that
conversation. IMHO, we were careful with MADV_DONTNEED because it was
... around for a little bit longer :)
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Darrick J. Wong @ 2025-02-25 15:59 UTC (permalink / raw)
To: Christian Brauner
Cc: Arnd Bergmann, Amir Goldstein, Andrey Albershteyn,
Richard Henderson, Matt Turner, Russell King, Catalin Marinas,
Will Deacon, Geert Uytterhoeven, Michal Simek,
Thomas Bogendoerfer, James E . J . Bottomley, Helge Deller,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, Naveen N Rao, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
David S . Miller, Andreas Larsson, Andy Lutomirski,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Chris Zankel, Max Filippov, Alexander Viro,
Jan Kara, Mickaël Salaün, Günther Noack,
linux-alpha, linux-kernel, linux-arm-kernel, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-fsdevel, linux-security-module, linux-api,
Linux-Arch, linux-xfs, Pali Rohár, Theodore Ts'o
In-Reply-To: <20250225-testfahrt-seilwinde-64e6f44c01ce@brauner>
On Tue, Feb 25, 2025 at 12:24:08PM +0100, Christian Brauner wrote:
> On Tue, Feb 25, 2025 at 11:40:51AM +0100, Arnd Bergmann wrote:
> > On Tue, Feb 25, 2025, at 11:22, Christian Brauner wrote:
> > > On Tue, Feb 25, 2025 at 09:02:04AM +0100, Arnd Bergmann wrote:
> > >> On Mon, Feb 24, 2025, at 12:32, Christian Brauner wrote:
> > >>
> > >> The ioctl interface relies on the existing behavior, see
> > >> 0a6eab8bd4e0 ("vfs: support FS_XFLAG_COWEXTSIZE and get/set of
> > >> CoW extent size hint") for how it was previously extended
> > >> with an optional flag/word. I think that is fine for the syscall
> > >> as well, but should be properly documented since it is different
> > >> from how most syscalls work.
> > >
> > > If we're doing a new system call I see no reason to limit us to a
> > > pre-existing structure or structure layout.
> >
> > Obviously we could create a new structure, but I also see no
> > reason to do so. The existing ioctl interface was added in
> > in 2002 as part of linux-2.5.35 with 16 bytes of padding, half
> > of which have been used so far.
> >
> > If this structure works for another 23 years before we run out
> > of spare bytes, I think that's good enough. Building in an
> > incompatible way to handle potential future contents would
> > just make it harder to use for any userspace that wants to
> > use the new syscalls but still needs a fallback to the
> > ioctl version.
>
> The fact that this structure has existed since the dawn of time doesn't
> mean it needs to be retained when adding a completely new system call.
>
> People won't mix both. They either switch to the new interface because
> they want to get around the limitations of the old interface or they
> keep using the old interface and the associated workarounds.
>
> In another thread they keep arguing about new extensions for Windows
> that are going to be added to the ioctl interface and how to make it fit
> into this. That just shows that it's very hard to predict from the
> amount of past changes how many future changes are going to happen. And
> if an interface is easy to extend it might well invite new changes that
> people didn't want to or couldn't make using the old interface.
Agreed, I don't think it's hard to enlarge struct fsxattr in the
existing ioctl interface; either we figure out how to make the kernel
fill out the "missing" bytes with an internal getfsxattr call, or we
make it return some errno if we would be truncating real output due to
struct size limits and leave a note in the manpage that "EL3HLT means
use a bigger structure definition"
Then both interfaces can plod along for another 30 years. :)
--D
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: Vlastimil Babka @ 2025-02-25 15:54 UTC (permalink / raw)
To: Lorenzo Stoakes, David Hildenbrand
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Paul E . McKenney, Jann Horn, linux-mm,
linux-kernel, Shuah Khan, linux-kselftest, linux-api,
John Hubbard, Juan Yescas, Kalesh Singh
In-Reply-To: <c0e079bd-a840-4240-93ae-0ee2755d425a@lucifer.local>
On 2/18/25 18:28, Lorenzo Stoakes wrote:
> On Tue, Feb 18, 2025 at 06:25:35PM +0100, David Hildenbrand wrote:
>>
>> > > >
>> > > > It fails because it tries to 'touch' the memory, but 'touching' guard
>> > > > region memory causes a segfault. This kind of breaks the idea of
>> > > > mlock()'ing guard regions.
>> > > >
>> > > > I think adding workarounds to make this possible in any way is not really
>> > > > worth it (and would probably be pretty gross).
>> > > >
>> > > > We already document that 'mlock()ing lightweight guard regions will fail'
>> > > > as per man page so this is all in line with that.
>> > >
>> > > Right, and I claim that supporting VM_LOCKONFAULT might likely be as easy as
>> > > allowing install/remove of guard regions when that flag is set.
>> >
>> > We already allow this flag! VM_LOCKED and VM_HUGETLB are the only flags we
>> > disallow.
>>
>>
>> See mlock2();
>>
>> SYSCALL_DEFINE3(mlock2, unsigned long, start, size_t, len, int, flags)
>> {
>> vm_flags_t vm_flags = VM_LOCKED;
>>
>> if (flags & ~MLOCK_ONFAULT)
>> return -EINVAL;
>>
>> if (flags & MLOCK_ONFAULT)
>> vm_flags |= VM_LOCKONFAULT;
>>
>> return do_mlock(start, len, vm_flags);
>> }
>>
>>
>> VM_LOCKONFAULT always as VM_LOCKED set as well.
>
> OK cool, that makes sense.
>
> As with much kernel stuff, I knew this in the past. Then I forgot. Then I knew
> again, then... :P if only somebody would write it down in a book...
>
> Yeah then that makes sense to check explicitly for (VM_LOCKED | VM_LOCKONFAULT)
> in any MADV_GUARD_INSTALL_LOCKED variant as obviously this would be passively
> excluded right now.
Sorry for the late reply. So AFAIU from your conversations, guards can't be
compatible with VM_LOCKED, which means e.g. any attempts of glibc to use
guards for stacks will soon discover that mlockall() users exist and are
broken by this, and the attempts will fail? That's a bummer.
As for compatibility with VM_LOCKONFAULT, do we need a new
MADV_GUARD_INSTALL_LOCKED or can we say MADV_GUARD_INSTALL is new enough
that it can be just retrofitted (like you retrofit file backed mappings)?
AFAIU the only risk would be breaking somebody that already relies on a
failure for VM_LOCKONFAULT, and it's unlikely there's such a somebody now.
^ permalink raw reply
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Christian Brauner @ 2025-02-25 11:24 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Amir Goldstein, Andrey Albershteyn, Darrick J. Wong,
Richard Henderson, Matt Turner, Russell King, Catalin Marinas,
Will Deacon, Geert Uytterhoeven, Michal Simek,
Thomas Bogendoerfer, James E . J . Bottomley, Helge Deller,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, Naveen N Rao, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
David S . Miller, Andreas Larsson, Andy Lutomirski,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Chris Zankel, Max Filippov, Alexander Viro,
Jan Kara, Mickaël Salaün, Günther Noack,
linux-alpha, linux-kernel, linux-arm-kernel, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-fsdevel, linux-security-module, linux-api,
Linux-Arch, linux-xfs, Pali Rohár, Theodore Ts'o
In-Reply-To: <3c860dc0-ba8d-4324-b286-c160b7d8d2c4@app.fastmail.com>
On Tue, Feb 25, 2025 at 11:40:51AM +0100, Arnd Bergmann wrote:
> On Tue, Feb 25, 2025, at 11:22, Christian Brauner wrote:
> > On Tue, Feb 25, 2025 at 09:02:04AM +0100, Arnd Bergmann wrote:
> >> On Mon, Feb 24, 2025, at 12:32, Christian Brauner wrote:
> >>
> >> The ioctl interface relies on the existing behavior, see
> >> 0a6eab8bd4e0 ("vfs: support FS_XFLAG_COWEXTSIZE and get/set of
> >> CoW extent size hint") for how it was previously extended
> >> with an optional flag/word. I think that is fine for the syscall
> >> as well, but should be properly documented since it is different
> >> from how most syscalls work.
> >
> > If we're doing a new system call I see no reason to limit us to a
> > pre-existing structure or structure layout.
>
> Obviously we could create a new structure, but I also see no
> reason to do so. The existing ioctl interface was added in
> in 2002 as part of linux-2.5.35 with 16 bytes of padding, half
> of which have been used so far.
>
> If this structure works for another 23 years before we run out
> of spare bytes, I think that's good enough. Building in an
> incompatible way to handle potential future contents would
> just make it harder to use for any userspace that wants to
> use the new syscalls but still needs a fallback to the
> ioctl version.
The fact that this structure has existed since the dawn of time doesn't
mean it needs to be retained when adding a completely new system call.
People won't mix both. They either switch to the new interface because
they want to get around the limitations of the old interface or they
keep using the old interface and the associated workarounds.
In another thread they keep arguing about new extensions for Windows
that are going to be added to the ioctl interface and how to make it fit
into this. That just shows that it's very hard to predict from the
amount of past changes how many future changes are going to happen. And
if an interface is easy to extend it might well invite new changes that
people didn't want to or couldn't make using the old interface.
^ permalink raw reply
* Re: [PATCH v2 17/20] fs/proc/task_mmu: remove per-page mapcount dependency for PM_MMAP_EXCLUSIVE (CONFIG_NO_PAGE_MAPCOUNT)
From: David Hildenbrand @ 2025-02-25 10:56 UTC (permalink / raw)
To: linux-kernel
Cc: linux-doc, cgroups, linux-mm, linux-fsdevel, linux-api,
Andrew Morton, Matthew Wilcox (Oracle), Tejun Heo, Zefan Li,
Johannes Weiner, Michal Koutný, Jonathan Corbet,
Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Muchun Song, Liam R. Howlett, Lorenzo Stoakes,
Vlastimil Babka, Jann Horn
In-Reply-To: <20250224165603.1434404-18-david@redhat.com>
On 24.02.25 17:55, David Hildenbrand wrote:
> Let's implement an alternative when per-page mapcounts in large folios are
> no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
>
> PM_MMAP_EXCLUSIVE will now be set if folio_likely_mapped_shared() is
> true -- when the folio is considered "mapped shared", including when
> it once was "mapped shared" but no longer is, as documented.
>
> This might result in and under-indication of "exclusively mapped", which
> is considered better than over-indicating it: under-estimating the USS
> (Unique Set Size) is better than over-estimating it.
>
> As an alternative, we could simply remove that flag with
> CONFIG_NO_PAGE_MAPCOUNT completely, but there might be value to it. So,
> let's keep it like that and document the behavior.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> Documentation/admin-guide/mm/pagemap.rst | 9 +++++++++
> fs/proc/task_mmu.c | 11 +++++++++--
> 2 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst
> index 49590306c61a0..131c86574c39a 100644
> --- a/Documentation/admin-guide/mm/pagemap.rst
> +++ b/Documentation/admin-guide/mm/pagemap.rst
> @@ -37,6 +37,15 @@ There are four components to pagemap:
> precisely which pages are mapped (or in swap) and comparing mapped
> pages between processes.
>
> + Note that in some kernel configurations, all pages part of a larger
> + allocation (e.g., THP) might be considered "mapped shared" if the large
> + allocation is considered "mapped shared": if not all pages are exclusive to
> + the same process. Further, some kernel configurations might consider larger
> + allocations "mapped shared", if they were at one point considered
> + "mapped shared", even if they would now be considered "exclusively mapped".
> + Consequently, in these kernel configurations, bit 56 might be set although
> + the page is actually "exclusively mapped"
I rewrote this yet another time to maybe make it clearer ...
+ Traditionally, bit 56 indicates that a page is mapped exactly once and bit
+ 56 is clear when a page is mapped multiple times, even when mapped in the
+ same process multiple times. In some kernel configurations, the semantics
+ for pages part of a larger allocation (e.g., THP) differ: bit 56 is set if
+ all pages part of the corresponding large allocation are *certainly* mapped
+ in the same process, even if the page is mapped multiple times in that
+ process. Bit 56 is clear when any page page of the larger allocation
+ is *maybe* mapped in a different process. In some cases, a large allocation
+ might be treated as "maybe mapped by multiple processes" even though this
+ is no longer the case.
(talking about "process" is not completely correct, it's actually "MMs"; but
that might add more confusion here)
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Arnd Bergmann @ 2025-02-25 10:40 UTC (permalink / raw)
To: Christian Brauner
Cc: Amir Goldstein, Andrey Albershteyn, Darrick J. Wong,
Richard Henderson, Matt Turner, Russell King, Catalin Marinas,
Will Deacon, Geert Uytterhoeven, Michal Simek,
Thomas Bogendoerfer, James E . J . Bottomley, Helge Deller,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, Naveen N Rao, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
David S . Miller, Andreas Larsson, Andy Lutomirski,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Chris Zankel, Max Filippov, Alexander Viro,
Jan Kara, Mickaël Salaün, Günther Noack,
linux-alpha, linux-kernel, linux-arm-kernel, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-fsdevel, linux-security-module, linux-api,
Linux-Arch, linux-xfs, Pali Rohár, Theodore Ts'o
In-Reply-To: <20250225-strom-kopflos-32062347cd13@brauner>
On Tue, Feb 25, 2025, at 11:22, Christian Brauner wrote:
> On Tue, Feb 25, 2025 at 09:02:04AM +0100, Arnd Bergmann wrote:
>> On Mon, Feb 24, 2025, at 12:32, Christian Brauner wrote:
>>
>> The ioctl interface relies on the existing behavior, see
>> 0a6eab8bd4e0 ("vfs: support FS_XFLAG_COWEXTSIZE and get/set of
>> CoW extent size hint") for how it was previously extended
>> with an optional flag/word. I think that is fine for the syscall
>> as well, but should be properly documented since it is different
>> from how most syscalls work.
>
> If we're doing a new system call I see no reason to limit us to a
> pre-existing structure or structure layout.
Obviously we could create a new structure, but I also see no
reason to do so. The existing ioctl interface was added in
in 2002 as part of linux-2.5.35 with 16 bytes of padding, half
of which have been used so far.
If this structure works for another 23 years before we run out
of spare bytes, I think that's good enough. Building in an
incompatible way to handle potential future contents would
just make it harder to use for any userspace that wants to
use the new syscalls but still needs a fallback to the
ioctl version.
Arnd
^ permalink raw reply
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Christian Brauner @ 2025-02-25 10:22 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Amir Goldstein, Andrey Albershteyn, Darrick J. Wong,
Richard Henderson, Matt Turner, Russell King, Catalin Marinas,
Will Deacon, Geert Uytterhoeven, Michal Simek,
Thomas Bogendoerfer, James E . J . Bottomley, Helge Deller,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, Naveen N Rao, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
David S . Miller, Andreas Larsson, Andy Lutomirski,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Chris Zankel, Max Filippov, Alexander Viro,
Jan Kara, Mickaël Salaün, Günther Noack,
linux-alpha, linux-kernel, linux-arm-kernel, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-fsdevel, linux-security-module, linux-api,
Linux-Arch, linux-xfs, Pali Rohár, Theodore Ts'o
In-Reply-To: <6b51ffa2-9d67-4466-865e-e703c1243352@app.fastmail.com>
On Tue, Feb 25, 2025 at 09:02:04AM +0100, Arnd Bergmann wrote:
> On Mon, Feb 24, 2025, at 12:32, Christian Brauner wrote:
> > On Fri, Feb 21, 2025 at 08:15:24PM +0100, Amir Goldstein wrote:
> >> On Fri, Feb 21, 2025 at 7:13 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> >> > > @@ -23,6 +23,9 @@
> >> > > #include <linux/rw_hint.h>
> >> > > #include <linux/seq_file.h>
> >> > > #include <linux/debugfs.h>
> >> > > +#include <linux/syscalls.h>
> >> > > +#include <linux/fileattr.h>
> >> > > +#include <linux/namei.h>
> >> > > #include <trace/events/writeback.h>
> >> > > #define CREATE_TRACE_POINTS
> >> > > #include <trace/events/timestamp.h>
> >> > > @@ -2953,3 +2956,75 @@ umode_t mode_strip_sgid(struct mnt_idmap *idmap,
> >> > > return mode & ~S_ISGID;
> >> > > }
> >> > > EXPORT_SYMBOL(mode_strip_sgid);
> >> > > +
> >> > > +SYSCALL_DEFINE4(getfsxattrat, int, dfd, const char __user *, filename,
> >> > > + struct fsxattr __user *, fsx, unsigned int, at_flags)
> >> >
> >> > Should the kernel require userspace to pass the size of the fsx buffer?
> >> > That way we avoid needing to rev the interface when we decide to grow
> >> > the structure.
> >
> > Please version the struct by size as we do for clone3(),
> > mount_setattr(), listmount()'s struct mnt_id_req, sched_setattr(), all
> > the new xattrat*() system calls and a host of others. So laying out the
> > struct 64bit and passing a size alongside it.
> >
> > This is all handled by copy_struct_from_user() and copy_struct_to_user()
> > so nothing to reinvent. And it's easy to copy from existing system
> > calls.
>
> I don't think that works in this case, because 'struct fsxattr'
> is an existing structure that is defined with a fixed size of
> 28 bytes. If we ever need more than 8 extra bytes, then the
> existing ioctl commands are also broken.
>
> Replacing fsxattr with an extensible structure of the same contents
> would work, but I feel that just adds more complication for little
> gain.
>
> On the other hand, there is an open question about how unknown
> flags and fields in this structure. FS_IOC_FSSETXATTR/FS_IOC_FSGETXATTR
> treats them as optional and just ignores anything it doesn't
> understand, while copy_struct_from_user() would treat any unknown
> but set bytes as -E2BIG.
>
> The ioctl interface relies on the existing behavior, see
> 0a6eab8bd4e0 ("vfs: support FS_XFLAG_COWEXTSIZE and get/set of
> CoW extent size hint") for how it was previously extended
> with an optional flag/word. I think that is fine for the syscall
> as well, but should be properly documented since it is different
> from how most syscalls work.
If we're doing a new system call I see no reason to limit us to a
pre-existing structure or structure layout.
^ permalink raw reply
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Arnd Bergmann @ 2025-02-25 8:02 UTC (permalink / raw)
To: Christian Brauner
Cc: Amir Goldstein, Andrey Albershteyn, Darrick J. Wong,
Richard Henderson, Matt Turner, Russell King, Catalin Marinas,
Will Deacon, Geert Uytterhoeven, Michal Simek,
Thomas Bogendoerfer, James E . J . Bottomley, Helge Deller,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, Naveen N Rao, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
David S . Miller, Andreas Larsson, Andy Lutomirski,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Chris Zankel, Max Filippov, Alexander Viro,
Jan Kara, Mickaël Salaün, Günther Noack,
linux-alpha, linux-kernel, linux-arm-kernel, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-fsdevel, linux-security-module, linux-api,
Linux-Arch, linux-xfs, Pali Rohár, Theodore Ts'o
In-Reply-To: <20250224-klinke-hochdekoriert-3f6be89005a8@brauner>
On Mon, Feb 24, 2025, at 12:32, Christian Brauner wrote:
> On Fri, Feb 21, 2025 at 08:15:24PM +0100, Amir Goldstein wrote:
>> On Fri, Feb 21, 2025 at 7:13 PM Darrick J. Wong <djwong@kernel.org> wrote:
>> > > @@ -23,6 +23,9 @@
>> > > #include <linux/rw_hint.h>
>> > > #include <linux/seq_file.h>
>> > > #include <linux/debugfs.h>
>> > > +#include <linux/syscalls.h>
>> > > +#include <linux/fileattr.h>
>> > > +#include <linux/namei.h>
>> > > #include <trace/events/writeback.h>
>> > > #define CREATE_TRACE_POINTS
>> > > #include <trace/events/timestamp.h>
>> > > @@ -2953,3 +2956,75 @@ umode_t mode_strip_sgid(struct mnt_idmap *idmap,
>> > > return mode & ~S_ISGID;
>> > > }
>> > > EXPORT_SYMBOL(mode_strip_sgid);
>> > > +
>> > > +SYSCALL_DEFINE4(getfsxattrat, int, dfd, const char __user *, filename,
>> > > + struct fsxattr __user *, fsx, unsigned int, at_flags)
>> >
>> > Should the kernel require userspace to pass the size of the fsx buffer?
>> > That way we avoid needing to rev the interface when we decide to grow
>> > the structure.
>
> Please version the struct by size as we do for clone3(),
> mount_setattr(), listmount()'s struct mnt_id_req, sched_setattr(), all
> the new xattrat*() system calls and a host of others. So laying out the
> struct 64bit and passing a size alongside it.
>
> This is all handled by copy_struct_from_user() and copy_struct_to_user()
> so nothing to reinvent. And it's easy to copy from existing system
> calls.
I don't think that works in this case, because 'struct fsxattr'
is an existing structure that is defined with a fixed size of
28 bytes. If we ever need more than 8 extra bytes, then the
existing ioctl commands are also broken.
Replacing fsxattr with an extensible structure of the same contents
would work, but I feel that just adds more complication for little
gain.
On the other hand, there is an open question about how unknown
flags and fields in this structure. FS_IOC_FSSETXATTR/FS_IOC_FSGETXATTR
treats them as optional and just ignores anything it doesn't
understand, while copy_struct_from_user() would treat any unknown
but set bytes as -E2BIG.
The ioctl interface relies on the existing behavior, see
0a6eab8bd4e0 ("vfs: support FS_XFLAG_COWEXTSIZE and get/set of
CoW extent size hint") for how it was previously extended
with an optional flag/word. I think that is fine for the syscall
as well, but should be properly documented since it is different
from how most syscalls work.
Arnd
^ permalink raw reply
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Paul Moore @ 2025-02-25 2:37 UTC (permalink / raw)
To: Andrey Albershteyn
Cc: Mickaël Salaün, Richard Henderson, Matt Turner,
Russell King, Catalin Marinas, Will Deacon, Geert Uytterhoeven,
Michal Simek, Thomas Bogendoerfer, James E.J. Bottomley,
Helge Deller, Madhavan Srinivasan, Michael Ellerman,
Nicholas Piggin, Christophe Leroy, Naveen N Rao, Heiko Carstens,
Vasily Gorbik, Alexander Gordeev, Christian Borntraeger,
Sven Schnelle, Yoshinori Sato, Rich Felker,
John Paul Adrian Glaubitz, David S. Miller, Andreas Larsson,
Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Chris Zankel, Max Filippov,
Alexander Viro, Christian Brauner, Jan Kara, Günther Noack,
Arnd Bergmann, linux-alpha, linux-kernel, linux-arm-kernel,
linux-m68k, linux-mips, linux-parisc, linuxppc-dev, linux-s390,
linux-sh, sparclinux, linux-fsdevel, linux-security-module,
linux-api, linux-arch, linux-xfs
In-Reply-To: <jvo6uj7ro5czlo5ukw3vtf5mpqgrbuksqq4j63s2i6gwrjpz4m@kghpcqyi7gwb>
On Mon, Feb 24, 2025 at 11:00 AM Andrey Albershteyn <aalbersh@redhat.com> wrote:
> On 2025-02-21 16:08:33, Mickaël Salaün wrote:
> > It looks security checks are missing. With IOCTL commands, file
> > permissions are checked at open time, but with these syscalls the path
> > is only resolved but no specific access seems to be checked (except
> > inode_owner_or_capable via vfs_fileattr_set).
...
> > On Tue, Feb 11, 2025 at 06:22:47PM +0100, Andrey Albershteyn wrote:
...
> > > +SYSCALL_DEFINE4(setfsxattrat, int, dfd, const char __user *, filename,
> > > + struct fsxattr __user *, fsx, unsigned int, at_flags)
> > > +{
> > > + CLASS(fd, dir)(dfd);
> > > + struct fileattr fa;
> > > + struct path filepath;
> > > + int error;
> > > + unsigned int lookup_flags = 0;
> > > +
> > > + if ((at_flags & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0)
> > > + return -EINVAL;
> > > +
> > > + if (at_flags & AT_SYMLINK_FOLLOW)
> > > + lookup_flags |= LOOKUP_FOLLOW;
> > > +
> > > + if (at_flags & AT_EMPTY_PATH)
> > > + lookup_flags |= LOOKUP_EMPTY;
> > > +
> > > + if (fd_empty(dir))
> > > + return -EBADF;
> > > +
> > > + if (copy_fsxattr_from_user(&fa, fsx))
> > > + return -EFAULT;
> > > +
> > > + error = user_path_at(dfd, filename, lookup_flags, &filepath);
> > > + if (error)
> > > + return error;
> > > +
> > > + error = mnt_want_write(filepath.mnt);
> > > + if (!error) {
> >
> > security_inode_setattr() should probably be called too.
>
> Aren't those checks for something different - inode attributes
> ATTR_*?
> (sorry, the naming can't be more confusing)
>
> Looking into security_inode_setattr() it seems to expect struct
> iattr, which works with inode attributes (mode, time, uid/gid...).
> These new syscalls work with filesystem inode extended flags/attributes
> FS_XFLAG_* in fsxattr->fsx_xflags. Let me know if I missing
> something here
A valid point. While these are two different operations, with
different structs/types, I suspect that most LSMs will consider them
to be roughly equivalent from an access control perspective, which is
why I felt the existing security_inode_{set,get}attr() hooks seemed
appropriate. However, there likely is value in keeping the ATTR and
FSX operations separate; those LSMs that wish to treat them the same
can easily do so in their respective LSM callbacks.
With all this in mind, I think it probably makes sense to create two
new LSM hooks, security_inode_{get,set}fsxattr(). The get hook should
probably be placed inside vfs_fileattr_get() just before the call to
the inode's fileattr_get() method, and the set hook should probably be
placed inside vfs_fileattr_set(), inside the inode lock and after a
successful call to fileattr_set_prepare().
Does that sound better to everyone?
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v2 16/20] fs/proc/page: remove per-page mapcount dependency for /proc/kpagecount (CONFIG_NO_PAGE_MAPCOUNT)
From: David Hildenbrand @ 2025-02-24 21:53 UTC (permalink / raw)
To: Zi Yan
Cc: linux-kernel, linux-doc, cgroups, linux-mm, linux-fsdevel,
linux-api, Andrew Morton, Matthew Wilcox (Oracle), Tejun Heo,
Zefan Li, Johannes Weiner, Michal Koutný, Jonathan Corbet,
Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Muchun Song, Liam R. Howlett, Lorenzo Stoakes,
Vlastimil Babka, Jann Horn
In-Reply-To: <1FAD9E31-3D11-4759-9363-4B76BE96002A@nvidia.com>
On 24.02.25 22:44, Zi Yan wrote:
> On 24 Feb 2025, at 16:42, David Hildenbrand wrote:
>
>> On 24.02.25 22:23, Zi Yan wrote:
>>> On 24 Feb 2025, at 16:15, David Hildenbrand wrote:
>>>
>>>> On 24.02.25 22:10, Zi Yan wrote:
>>>>> On 24 Feb 2025, at 16:02, David Hildenbrand wrote:
>>>>>
>>>>>> On 24.02.25 21:40, Zi Yan wrote:
>>>>>>> On Mon Feb 24, 2025 at 11:55 AM EST, David Hildenbrand wrote:
>>>>>>>> Let's implement an alternative when per-page mapcounts in large folios
>>>>>>>> are no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
>>>>>>>>
>>>>>>>> For large folios, we'll return the per-page average mapcount within the
>>>>>>>> folio, except when the average is 0 but the folio is mapped: then we
>>>>>>>> return 1.
>>>>>>>>
>>>>>>>> For hugetlb folios and for large folios that are fully mapped
>>>>>>>> into all address spaces, there is no change.
>>>>>>>>
>>>>>>>> As an alternative, we could simply return 0 for non-hugetlb large folios,
>>>>>>>> or disable this legacy interface with CONFIG_NO_PAGE_MAPCOUNT.
>>>>>>>>
>>>>>>>> But the information exposed by this interface can still be valuable, and
>>>>>>>> frequently we deal with fully-mapped large folios where the average
>>>>>>>> corresponds to the actual page mapcount. So we'll leave it like this for
>>>>>>>> now and document the new behavior.
>>>>>>>>
>>>>>>>> Note: this interface is likely not very relevant for performance. If
>>>>>>>> ever required, we could try doing a rather expensive rmap walk to collect
>>>>>>>> precisely how often this folio page is mapped.
>>>>>>>>
>>>>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>>>>> ---
>>>>>>>> Documentation/admin-guide/mm/pagemap.rst | 7 +++++-
>>>>>>>> fs/proc/internal.h | 31 ++++++++++++++++++++++++
>>>>>>>> fs/proc/page.c | 19 ++++++++++++---
>>>>>>>> 3 files changed, 53 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst
>>>>>>>> index caba0f52dd36c..49590306c61a0 100644
>>>>>>>> --- a/Documentation/admin-guide/mm/pagemap.rst
>>>>>>>> +++ b/Documentation/admin-guide/mm/pagemap.rst
>>>>>>>> @@ -42,7 +42,12 @@ There are four components to pagemap:
>>>>>>>> skip over unmapped regions.
>>>>>>>> * ``/proc/kpagecount``. This file contains a 64-bit count of the number of
>>>>>>>> - times each page is mapped, indexed by PFN.
>>>>>>>> + times each page is mapped, indexed by PFN. Some kernel configurations do
>>>>>>>> + not track the precise number of times a page part of a larger allocation
>>>>>>>> + (e.g., THP) is mapped. In these configurations, the average number of
>>>>>>>> + mappings per page in this larger allocation is returned instead. However,
>>>>>>>> + if any page of the large allocation is mapped, the returned value will
>>>>>>>> + be at least 1.
>>>>>>>> The page-types tool in the tools/mm directory can be used to query the
>>>>>>>> number of times a page is mapped.
>>>>>>>> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
>>>>>>>> index 1695509370b88..16aa1fd260771 100644
>>>>>>>> --- a/fs/proc/internal.h
>>>>>>>> +++ b/fs/proc/internal.h
>>>>>>>> @@ -174,6 +174,37 @@ static inline int folio_precise_page_mapcount(struct folio *folio,
>>>>>>>> return mapcount;
>>>>>>>> }
>>>>>>>> +/**
>>>>>>>> + * folio_average_page_mapcount() - Average number of mappings per page in this
>>>>>>>> + * folio
>>>>>>>> + * @folio: The folio.
>>>>>>>> + *
>>>>>>>> + * The average number of present user page table entries that reference each
>>>>>>>> + * page in this folio as tracked via the RMAP: either referenced directly
>>>>>>>> + * (PTE) or as part of a larger area that covers this page (e.g., PMD).
>>>>>>>> + *
>>>>>>>> + * Returns: The average number of mappings per page in this folio. 0 for
>>>>>>>> + * folios that are not mapped to user space or are not tracked via the RMAP
>>>>>>>> + * (e.g., shared zeropage).
>>>>>>>> + */
>>>>>>>> +static inline int folio_average_page_mapcount(struct folio *folio)
>>>>>>>> +{
>>>>>>>> + int mapcount, entire_mapcount;
>>>>>>>> + unsigned int adjust;
>>>>>>>> +
>>>>>>>> + if (!folio_test_large(folio))
>>>>>>>> + return atomic_read(&folio->_mapcount) + 1;
>>>>>>>> +
>>>>>>>> + mapcount = folio_large_mapcount(folio);
>>>>>>>> + entire_mapcount = folio_entire_mapcount(folio);
>>>>>>>> + if (mapcount <= entire_mapcount)
>>>>>>>> + return entire_mapcount;
>>>>>>>> + mapcount -= entire_mapcount;
>>>>>>>> +
>>>>>>>> + adjust = folio_large_nr_pages(folio) / 2;
>>>>>>
>>>>>> Thanks for the review!
>>>>>>
>>>>>>>
>>>>>>> Is there any reason for choosing this adjust number? A comment might be
>>>>>>> helpful in case people want to change it later, either with some reasoning
>>>>>>> or just saying it is chosen empirically.
>>>>>>
>>>>>> We're dividing by folio_large_nr_pages(folio) (shifting by folio_large_order(folio)), so this is not a magic number at all.
>>>>>>
>>>>>> So this should be "ordinary" rounding.
>>>>>
>>>>> I thought the rounding would be (mapcount + 511) / 512.
>>>>
>>>> Yes, that's "rounding up".
>>>>
>>>>> But
>>>>> that means if one subpage is mapped, the average will be 1.
>>>>> Your rounding means if at least half of the subpages is mapped,
>>>>> the average will be 1. Others might think 1/3 is mapped,
>>>>> the average will be 1. That is why I think adjust looks like
>>>>> a magic number.
>>>>
>>>> I think all callers could tolerate (or benefit) from folio_average_page_mapcount() returning at least 1 in case any page is mapped.
>>>>
>>>> There was a reason why I decided to round to the nearest integer instead.
>>>>
>>>> Let me think about this once more, I went back and forth a couple of times on this.
>>>
>>> Sure. Your current choice might be good enough for now. My intend of
>>> adding a comment here is just to let people know the adjust can be
>>> changed in the future. :)
>>
>> The following will make the callers easier to read, while keeping
>> the rounding to the next integer for the other cases untouched.
>>
>> +/**
>> + * folio_average_page_mapcount() - Average number of mappings per page in this
>> + * folio
>> + * @folio: The folio.
>> + *
>> + * The average number of present user page table entries that reference each
>> + * page in this folio as tracked via the RMAP: either referenced directly
>> + * (PTE) or as part of a larger area that covers this page (e.g., PMD).
>> + *
>> + * The average is calculated by rounding to the nearest integer; however,
>> + * if at least a single page is mapped, the average will be at least 1.
>> + *
>> + * Returns: The average number of mappings per page in this folio.
>> + */
>> +static inline int folio_average_page_mapcount(struct folio *folio)
>> +{
>> + int mapcount, entire_mapcount, avg;
>> +
>> + if (!folio_test_large(folio))
>> + return atomic_read(&folio->_mapcount) + 1;
>> +
>> + mapcount = folio_large_mapcount(folio);
>> + if (unlikely(mapcount <= 0))
>> + return 0;
>> + entire_mapcount = folio_entire_mapcount(folio);
>> + if (mapcount <= entire_mapcount)
>> + return entire_mapcount;
>> + mapcount -= entire_mapcount;
>> +
>> + /* Round to closest integer ... */
>> + avg = (mapcount + folio_large_nr_pages(folio) / 2) >> folio_large_order(folio);
>> + avg += entire_mapcount;
>> + /* ... but return at least 1. */
>> + return max_t(int, avg, 1);
>> +}
>
> LGTM. Thanks.
Thanks! BTW, I think I chose the "round to closest integer" primarily
to make the PSS estimate a bit better. But that is indeed something
that can be adjusted easily later.
BTW, as commented in the cover letter, being able to calculate the
avg without the entire_mapcount could clean this function up quite a bit
(and make it completely atomic), but that will require more work.
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 16/20] fs/proc/page: remove per-page mapcount dependency for /proc/kpagecount (CONFIG_NO_PAGE_MAPCOUNT)
From: Zi Yan @ 2025-02-24 21:44 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-doc, cgroups, linux-mm, linux-fsdevel,
linux-api, Andrew Morton, Matthew Wilcox (Oracle), Tejun Heo,
Zefan Li, Johannes Weiner, Michal Koutný, Jonathan Corbet,
Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Muchun Song, Liam R. Howlett, Lorenzo Stoakes,
Vlastimil Babka, Jann Horn
In-Reply-To: <3f6b7e66-3412-4af2-97d9-6d31d6373079@redhat.com>
On 24 Feb 2025, at 16:42, David Hildenbrand wrote:
> On 24.02.25 22:23, Zi Yan wrote:
>> On 24 Feb 2025, at 16:15, David Hildenbrand wrote:
>>
>>> On 24.02.25 22:10, Zi Yan wrote:
>>>> On 24 Feb 2025, at 16:02, David Hildenbrand wrote:
>>>>
>>>>> On 24.02.25 21:40, Zi Yan wrote:
>>>>>> On Mon Feb 24, 2025 at 11:55 AM EST, David Hildenbrand wrote:
>>>>>>> Let's implement an alternative when per-page mapcounts in large folios
>>>>>>> are no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
>>>>>>>
>>>>>>> For large folios, we'll return the per-page average mapcount within the
>>>>>>> folio, except when the average is 0 but the folio is mapped: then we
>>>>>>> return 1.
>>>>>>>
>>>>>>> For hugetlb folios and for large folios that are fully mapped
>>>>>>> into all address spaces, there is no change.
>>>>>>>
>>>>>>> As an alternative, we could simply return 0 for non-hugetlb large folios,
>>>>>>> or disable this legacy interface with CONFIG_NO_PAGE_MAPCOUNT.
>>>>>>>
>>>>>>> But the information exposed by this interface can still be valuable, and
>>>>>>> frequently we deal with fully-mapped large folios where the average
>>>>>>> corresponds to the actual page mapcount. So we'll leave it like this for
>>>>>>> now and document the new behavior.
>>>>>>>
>>>>>>> Note: this interface is likely not very relevant for performance. If
>>>>>>> ever required, we could try doing a rather expensive rmap walk to collect
>>>>>>> precisely how often this folio page is mapped.
>>>>>>>
>>>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>>>> ---
>>>>>>> Documentation/admin-guide/mm/pagemap.rst | 7 +++++-
>>>>>>> fs/proc/internal.h | 31 ++++++++++++++++++++++++
>>>>>>> fs/proc/page.c | 19 ++++++++++++---
>>>>>>> 3 files changed, 53 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst
>>>>>>> index caba0f52dd36c..49590306c61a0 100644
>>>>>>> --- a/Documentation/admin-guide/mm/pagemap.rst
>>>>>>> +++ b/Documentation/admin-guide/mm/pagemap.rst
>>>>>>> @@ -42,7 +42,12 @@ There are four components to pagemap:
>>>>>>> skip over unmapped regions.
>>>>>>> * ``/proc/kpagecount``. This file contains a 64-bit count of the number of
>>>>>>> - times each page is mapped, indexed by PFN.
>>>>>>> + times each page is mapped, indexed by PFN. Some kernel configurations do
>>>>>>> + not track the precise number of times a page part of a larger allocation
>>>>>>> + (e.g., THP) is mapped. In these configurations, the average number of
>>>>>>> + mappings per page in this larger allocation is returned instead. However,
>>>>>>> + if any page of the large allocation is mapped, the returned value will
>>>>>>> + be at least 1.
>>>>>>> The page-types tool in the tools/mm directory can be used to query the
>>>>>>> number of times a page is mapped.
>>>>>>> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
>>>>>>> index 1695509370b88..16aa1fd260771 100644
>>>>>>> --- a/fs/proc/internal.h
>>>>>>> +++ b/fs/proc/internal.h
>>>>>>> @@ -174,6 +174,37 @@ static inline int folio_precise_page_mapcount(struct folio *folio,
>>>>>>> return mapcount;
>>>>>>> }
>>>>>>> +/**
>>>>>>> + * folio_average_page_mapcount() - Average number of mappings per page in this
>>>>>>> + * folio
>>>>>>> + * @folio: The folio.
>>>>>>> + *
>>>>>>> + * The average number of present user page table entries that reference each
>>>>>>> + * page in this folio as tracked via the RMAP: either referenced directly
>>>>>>> + * (PTE) or as part of a larger area that covers this page (e.g., PMD).
>>>>>>> + *
>>>>>>> + * Returns: The average number of mappings per page in this folio. 0 for
>>>>>>> + * folios that are not mapped to user space or are not tracked via the RMAP
>>>>>>> + * (e.g., shared zeropage).
>>>>>>> + */
>>>>>>> +static inline int folio_average_page_mapcount(struct folio *folio)
>>>>>>> +{
>>>>>>> + int mapcount, entire_mapcount;
>>>>>>> + unsigned int adjust;
>>>>>>> +
>>>>>>> + if (!folio_test_large(folio))
>>>>>>> + return atomic_read(&folio->_mapcount) + 1;
>>>>>>> +
>>>>>>> + mapcount = folio_large_mapcount(folio);
>>>>>>> + entire_mapcount = folio_entire_mapcount(folio);
>>>>>>> + if (mapcount <= entire_mapcount)
>>>>>>> + return entire_mapcount;
>>>>>>> + mapcount -= entire_mapcount;
>>>>>>> +
>>>>>>> + adjust = folio_large_nr_pages(folio) / 2;
>>>>>
>>>>> Thanks for the review!
>>>>>
>>>>>>
>>>>>> Is there any reason for choosing this adjust number? A comment might be
>>>>>> helpful in case people want to change it later, either with some reasoning
>>>>>> or just saying it is chosen empirically.
>>>>>
>>>>> We're dividing by folio_large_nr_pages(folio) (shifting by folio_large_order(folio)), so this is not a magic number at all.
>>>>>
>>>>> So this should be "ordinary" rounding.
>>>>
>>>> I thought the rounding would be (mapcount + 511) / 512.
>>>
>>> Yes, that's "rounding up".
>>>
>>>> But
>>>> that means if one subpage is mapped, the average will be 1.
>>>> Your rounding means if at least half of the subpages is mapped,
>>>> the average will be 1. Others might think 1/3 is mapped,
>>>> the average will be 1. That is why I think adjust looks like
>>>> a magic number.
>>>
>>> I think all callers could tolerate (or benefit) from folio_average_page_mapcount() returning at least 1 in case any page is mapped.
>>>
>>> There was a reason why I decided to round to the nearest integer instead.
>>>
>>> Let me think about this once more, I went back and forth a couple of times on this.
>>
>> Sure. Your current choice might be good enough for now. My intend of
>> adding a comment here is just to let people know the adjust can be
>> changed in the future. :)
>
> The following will make the callers easier to read, while keeping
> the rounding to the next integer for the other cases untouched.
>
> +/**
> + * folio_average_page_mapcount() - Average number of mappings per page in this
> + * folio
> + * @folio: The folio.
> + *
> + * The average number of present user page table entries that reference each
> + * page in this folio as tracked via the RMAP: either referenced directly
> + * (PTE) or as part of a larger area that covers this page (e.g., PMD).
> + *
> + * The average is calculated by rounding to the nearest integer; however,
> + * if at least a single page is mapped, the average will be at least 1.
> + *
> + * Returns: The average number of mappings per page in this folio.
> + */
> +static inline int folio_average_page_mapcount(struct folio *folio)
> +{
> + int mapcount, entire_mapcount, avg;
> +
> + if (!folio_test_large(folio))
> + return atomic_read(&folio->_mapcount) + 1;
> +
> + mapcount = folio_large_mapcount(folio);
> + if (unlikely(mapcount <= 0))
> + return 0;
> + entire_mapcount = folio_entire_mapcount(folio);
> + if (mapcount <= entire_mapcount)
> + return entire_mapcount;
> + mapcount -= entire_mapcount;
> +
> + /* Round to closest integer ... */
> + avg = (mapcount + folio_large_nr_pages(folio) / 2) >> folio_large_order(folio);
> + avg += entire_mapcount;
> + /* ... but return at least 1. */
> + return max_t(int, avg, 1);
> +}
LGTM. Thanks.
Best Regards,
Yan, Zi
^ permalink raw reply
* Re: [PATCH v2 16/20] fs/proc/page: remove per-page mapcount dependency for /proc/kpagecount (CONFIG_NO_PAGE_MAPCOUNT)
From: David Hildenbrand @ 2025-02-24 21:42 UTC (permalink / raw)
To: Zi Yan
Cc: linux-kernel, linux-doc, cgroups, linux-mm, linux-fsdevel,
linux-api, Andrew Morton, Matthew Wilcox (Oracle), Tejun Heo,
Zefan Li, Johannes Weiner, Michal Koutný, Jonathan Corbet,
Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Muchun Song, Liam R. Howlett, Lorenzo Stoakes,
Vlastimil Babka, Jann Horn
In-Reply-To: <30C2A030-7438-4298-87D8-287BED1EA473@nvidia.com>
On 24.02.25 22:23, Zi Yan wrote:
> On 24 Feb 2025, at 16:15, David Hildenbrand wrote:
>
>> On 24.02.25 22:10, Zi Yan wrote:
>>> On 24 Feb 2025, at 16:02, David Hildenbrand wrote:
>>>
>>>> On 24.02.25 21:40, Zi Yan wrote:
>>>>> On Mon Feb 24, 2025 at 11:55 AM EST, David Hildenbrand wrote:
>>>>>> Let's implement an alternative when per-page mapcounts in large folios
>>>>>> are no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
>>>>>>
>>>>>> For large folios, we'll return the per-page average mapcount within the
>>>>>> folio, except when the average is 0 but the folio is mapped: then we
>>>>>> return 1.
>>>>>>
>>>>>> For hugetlb folios and for large folios that are fully mapped
>>>>>> into all address spaces, there is no change.
>>>>>>
>>>>>> As an alternative, we could simply return 0 for non-hugetlb large folios,
>>>>>> or disable this legacy interface with CONFIG_NO_PAGE_MAPCOUNT.
>>>>>>
>>>>>> But the information exposed by this interface can still be valuable, and
>>>>>> frequently we deal with fully-mapped large folios where the average
>>>>>> corresponds to the actual page mapcount. So we'll leave it like this for
>>>>>> now and document the new behavior.
>>>>>>
>>>>>> Note: this interface is likely not very relevant for performance. If
>>>>>> ever required, we could try doing a rather expensive rmap walk to collect
>>>>>> precisely how often this folio page is mapped.
>>>>>>
>>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>>> ---
>>>>>> Documentation/admin-guide/mm/pagemap.rst | 7 +++++-
>>>>>> fs/proc/internal.h | 31 ++++++++++++++++++++++++
>>>>>> fs/proc/page.c | 19 ++++++++++++---
>>>>>> 3 files changed, 53 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst
>>>>>> index caba0f52dd36c..49590306c61a0 100644
>>>>>> --- a/Documentation/admin-guide/mm/pagemap.rst
>>>>>> +++ b/Documentation/admin-guide/mm/pagemap.rst
>>>>>> @@ -42,7 +42,12 @@ There are four components to pagemap:
>>>>>> skip over unmapped regions.
>>>>>> * ``/proc/kpagecount``. This file contains a 64-bit count of the number of
>>>>>> - times each page is mapped, indexed by PFN.
>>>>>> + times each page is mapped, indexed by PFN. Some kernel configurations do
>>>>>> + not track the precise number of times a page part of a larger allocation
>>>>>> + (e.g., THP) is mapped. In these configurations, the average number of
>>>>>> + mappings per page in this larger allocation is returned instead. However,
>>>>>> + if any page of the large allocation is mapped, the returned value will
>>>>>> + be at least 1.
>>>>>> The page-types tool in the tools/mm directory can be used to query the
>>>>>> number of times a page is mapped.
>>>>>> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
>>>>>> index 1695509370b88..16aa1fd260771 100644
>>>>>> --- a/fs/proc/internal.h
>>>>>> +++ b/fs/proc/internal.h
>>>>>> @@ -174,6 +174,37 @@ static inline int folio_precise_page_mapcount(struct folio *folio,
>>>>>> return mapcount;
>>>>>> }
>>>>>> +/**
>>>>>> + * folio_average_page_mapcount() - Average number of mappings per page in this
>>>>>> + * folio
>>>>>> + * @folio: The folio.
>>>>>> + *
>>>>>> + * The average number of present user page table entries that reference each
>>>>>> + * page in this folio as tracked via the RMAP: either referenced directly
>>>>>> + * (PTE) or as part of a larger area that covers this page (e.g., PMD).
>>>>>> + *
>>>>>> + * Returns: The average number of mappings per page in this folio. 0 for
>>>>>> + * folios that are not mapped to user space or are not tracked via the RMAP
>>>>>> + * (e.g., shared zeropage).
>>>>>> + */
>>>>>> +static inline int folio_average_page_mapcount(struct folio *folio)
>>>>>> +{
>>>>>> + int mapcount, entire_mapcount;
>>>>>> + unsigned int adjust;
>>>>>> +
>>>>>> + if (!folio_test_large(folio))
>>>>>> + return atomic_read(&folio->_mapcount) + 1;
>>>>>> +
>>>>>> + mapcount = folio_large_mapcount(folio);
>>>>>> + entire_mapcount = folio_entire_mapcount(folio);
>>>>>> + if (mapcount <= entire_mapcount)
>>>>>> + return entire_mapcount;
>>>>>> + mapcount -= entire_mapcount;
>>>>>> +
>>>>>> + adjust = folio_large_nr_pages(folio) / 2;
>>>>
>>>> Thanks for the review!
>>>>
>>>>>
>>>>> Is there any reason for choosing this adjust number? A comment might be
>>>>> helpful in case people want to change it later, either with some reasoning
>>>>> or just saying it is chosen empirically.
>>>>
>>>> We're dividing by folio_large_nr_pages(folio) (shifting by folio_large_order(folio)), so this is not a magic number at all.
>>>>
>>>> So this should be "ordinary" rounding.
>>>
>>> I thought the rounding would be (mapcount + 511) / 512.
>>
>> Yes, that's "rounding up".
>>
>>> But
>>> that means if one subpage is mapped, the average will be 1.
>>> Your rounding means if at least half of the subpages is mapped,
>>> the average will be 1. Others might think 1/3 is mapped,
>>> the average will be 1. That is why I think adjust looks like
>>> a magic number.
>>
>> I think all callers could tolerate (or benefit) from folio_average_page_mapcount() returning at least 1 in case any page is mapped.
>>
>> There was a reason why I decided to round to the nearest integer instead.
>>
>> Let me think about this once more, I went back and forth a couple of times on this.
>
> Sure. Your current choice might be good enough for now. My intend of
> adding a comment here is just to let people know the adjust can be
> changed in the future. :)
The following will make the callers easier to read, while keeping
the rounding to the next integer for the other cases untouched.
+/**
+ * folio_average_page_mapcount() - Average number of mappings per page in this
+ * folio
+ * @folio: The folio.
+ *
+ * The average number of present user page table entries that reference each
+ * page in this folio as tracked via the RMAP: either referenced directly
+ * (PTE) or as part of a larger area that covers this page (e.g., PMD).
+ *
+ * The average is calculated by rounding to the nearest integer; however,
+ * if at least a single page is mapped, the average will be at least 1.
+ *
+ * Returns: The average number of mappings per page in this folio.
+ */
+static inline int folio_average_page_mapcount(struct folio *folio)
+{
+ int mapcount, entire_mapcount, avg;
+
+ if (!folio_test_large(folio))
+ return atomic_read(&folio->_mapcount) + 1;
+
+ mapcount = folio_large_mapcount(folio);
+ if (unlikely(mapcount <= 0))
+ return 0;
+ entire_mapcount = folio_entire_mapcount(folio);
+ if (mapcount <= entire_mapcount)
+ return entire_mapcount;
+ mapcount -= entire_mapcount;
+
+ /* Round to closest integer ... */
+ avg = (mapcount + folio_large_nr_pages(folio) / 2) >> folio_large_order(folio);
+ avg += entire_mapcount;
+ /* ... but return at least 1. */
+ return max_t(int, avg, 1);
+}
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 16/20] fs/proc/page: remove per-page mapcount dependency for /proc/kpagecount (CONFIG_NO_PAGE_MAPCOUNT)
From: Zi Yan @ 2025-02-24 21:23 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-doc, cgroups, linux-mm, linux-fsdevel,
linux-api, Andrew Morton, Matthew Wilcox (Oracle), Tejun Heo,
Zefan Li, Johannes Weiner, Michal Koutný, Jonathan Corbet,
Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Muchun Song, Liam R. Howlett, Lorenzo Stoakes,
Vlastimil Babka, Jann Horn
In-Reply-To: <567b02b0-3e39-4e3c-ba41-1bc59217a421@redhat.com>
On 24 Feb 2025, at 16:15, David Hildenbrand wrote:
> On 24.02.25 22:10, Zi Yan wrote:
>> On 24 Feb 2025, at 16:02, David Hildenbrand wrote:
>>
>>> On 24.02.25 21:40, Zi Yan wrote:
>>>> On Mon Feb 24, 2025 at 11:55 AM EST, David Hildenbrand wrote:
>>>>> Let's implement an alternative when per-page mapcounts in large folios
>>>>> are no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
>>>>>
>>>>> For large folios, we'll return the per-page average mapcount within the
>>>>> folio, except when the average is 0 but the folio is mapped: then we
>>>>> return 1.
>>>>>
>>>>> For hugetlb folios and for large folios that are fully mapped
>>>>> into all address spaces, there is no change.
>>>>>
>>>>> As an alternative, we could simply return 0 for non-hugetlb large folios,
>>>>> or disable this legacy interface with CONFIG_NO_PAGE_MAPCOUNT.
>>>>>
>>>>> But the information exposed by this interface can still be valuable, and
>>>>> frequently we deal with fully-mapped large folios where the average
>>>>> corresponds to the actual page mapcount. So we'll leave it like this for
>>>>> now and document the new behavior.
>>>>>
>>>>> Note: this interface is likely not very relevant for performance. If
>>>>> ever required, we could try doing a rather expensive rmap walk to collect
>>>>> precisely how often this folio page is mapped.
>>>>>
>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>> ---
>>>>> Documentation/admin-guide/mm/pagemap.rst | 7 +++++-
>>>>> fs/proc/internal.h | 31 ++++++++++++++++++++++++
>>>>> fs/proc/page.c | 19 ++++++++++++---
>>>>> 3 files changed, 53 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst
>>>>> index caba0f52dd36c..49590306c61a0 100644
>>>>> --- a/Documentation/admin-guide/mm/pagemap.rst
>>>>> +++ b/Documentation/admin-guide/mm/pagemap.rst
>>>>> @@ -42,7 +42,12 @@ There are four components to pagemap:
>>>>> skip over unmapped regions.
>>>>> * ``/proc/kpagecount``. This file contains a 64-bit count of the number of
>>>>> - times each page is mapped, indexed by PFN.
>>>>> + times each page is mapped, indexed by PFN. Some kernel configurations do
>>>>> + not track the precise number of times a page part of a larger allocation
>>>>> + (e.g., THP) is mapped. In these configurations, the average number of
>>>>> + mappings per page in this larger allocation is returned instead. However,
>>>>> + if any page of the large allocation is mapped, the returned value will
>>>>> + be at least 1.
>>>>> The page-types tool in the tools/mm directory can be used to query the
>>>>> number of times a page is mapped.
>>>>> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
>>>>> index 1695509370b88..16aa1fd260771 100644
>>>>> --- a/fs/proc/internal.h
>>>>> +++ b/fs/proc/internal.h
>>>>> @@ -174,6 +174,37 @@ static inline int folio_precise_page_mapcount(struct folio *folio,
>>>>> return mapcount;
>>>>> }
>>>>> +/**
>>>>> + * folio_average_page_mapcount() - Average number of mappings per page in this
>>>>> + * folio
>>>>> + * @folio: The folio.
>>>>> + *
>>>>> + * The average number of present user page table entries that reference each
>>>>> + * page in this folio as tracked via the RMAP: either referenced directly
>>>>> + * (PTE) or as part of a larger area that covers this page (e.g., PMD).
>>>>> + *
>>>>> + * Returns: The average number of mappings per page in this folio. 0 for
>>>>> + * folios that are not mapped to user space or are not tracked via the RMAP
>>>>> + * (e.g., shared zeropage).
>>>>> + */
>>>>> +static inline int folio_average_page_mapcount(struct folio *folio)
>>>>> +{
>>>>> + int mapcount, entire_mapcount;
>>>>> + unsigned int adjust;
>>>>> +
>>>>> + if (!folio_test_large(folio))
>>>>> + return atomic_read(&folio->_mapcount) + 1;
>>>>> +
>>>>> + mapcount = folio_large_mapcount(folio);
>>>>> + entire_mapcount = folio_entire_mapcount(folio);
>>>>> + if (mapcount <= entire_mapcount)
>>>>> + return entire_mapcount;
>>>>> + mapcount -= entire_mapcount;
>>>>> +
>>>>> + adjust = folio_large_nr_pages(folio) / 2;
>>>
>>> Thanks for the review!
>>>
>>>>
>>>> Is there any reason for choosing this adjust number? A comment might be
>>>> helpful in case people want to change it later, either with some reasoning
>>>> or just saying it is chosen empirically.
>>>
>>> We're dividing by folio_large_nr_pages(folio) (shifting by folio_large_order(folio)), so this is not a magic number at all.
>>>
>>> So this should be "ordinary" rounding.
>>
>> I thought the rounding would be (mapcount + 511) / 512.
>
> Yes, that's "rounding up".
>
>> But
>> that means if one subpage is mapped, the average will be 1.
>> Your rounding means if at least half of the subpages is mapped,
>> the average will be 1. Others might think 1/3 is mapped,
>> the average will be 1. That is why I think adjust looks like
>> a magic number.
>
> I think all callers could tolerate (or benefit) from folio_average_page_mapcount() returning at least 1 in case any page is mapped.
>
> There was a reason why I decided to round to the nearest integer instead.
>
> Let me think about this once more, I went back and forth a couple of times on this.
Sure. Your current choice might be good enough for now. My intend of
adding a comment here is just to let people know the adjust can be
changed in the future. :)
Best Regards,
Yan, Zi
^ permalink raw reply
* Re: [PATCH v2 19/20] fs/proc/task_mmu: remove per-page mapcount dependency for smaps/smaps_rollup (CONFIG_NO_PAGE_MAPCOUNT)
From: David Hildenbrand @ 2025-02-24 21:17 UTC (permalink / raw)
To: Zi Yan, linux-kernel
Cc: linux-doc, cgroups, linux-mm, linux-fsdevel, linux-api,
Andrew Morton, Matthew Wilcox (Oracle), Tejun Heo, Zefan Li,
Johannes Weiner, Michal Koutný, Jonathan Corbet,
Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Muchun Song, Liam R. Howlett, Lorenzo Stoakes,
Vlastimil Babka, Jann Horn
In-Reply-To: <D80Z3H6NZARU.1HP5EKXOJ68QH@nvidia.com>
On 24.02.25 21:53, Zi Yan wrote:
> On Mon Feb 24, 2025 at 11:56 AM EST, David Hildenbrand wrote:
>> Let's implement an alternative when per-page mapcounts in large folios are
>> no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
>>
>> When computing the output for smaps / smaps_rollups, in particular when
>> calculating the USS (Unique Set Size) and the PSS (Proportional Set Size),
>> we still rely on per-page mapcounts.
>>
>> To determine private vs. shared, we'll use folio_likely_mapped_shared(),
>> similar to how we handle PM_MMAP_EXCLUSIVE. Similarly, we might now
>> under-estimate the USS and count pages towards "shared" that are
>> actually "private" ("exclusively mapped").
>>
>> When calculating the PSS, we'll now also use the average per-page
>> mapcount for large folios: this can result in both, an over-estimation
>> and an under-estimation of the PSS. The difference is not expected to
>> matter much in practice, but we'll have to learn as we go.
>>
>> We can now provide folio_precise_page_mapcount() only with
>> CONFIG_PAGE_MAPCOUNT, and remove one of the last users of per-page
>> mapcounts when CONFIG_NO_PAGE_MAPCOUNT is enabled.
>>
>> Document the new behavior.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> Documentation/filesystems/proc.rst | 13 +++++++++++++
>> fs/proc/internal.h | 8 ++++++++
>> fs/proc/task_mmu.c | 17 +++++++++++++++--
>> 3 files changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
>> index 1aa190017f796..57d55274a1f42 100644
>> --- a/Documentation/filesystems/proc.rst
>> +++ b/Documentation/filesystems/proc.rst
>> @@ -506,6 +506,19 @@ Note that even a page which is part of a MAP_SHARED mapping, but has only
>> a single pte mapped, i.e. is currently used by only one process, is accounted
>> as private and not as shared.
>>
>> +Note that in some kernel configurations, all pages part of a larger allocation
>> +(e.g., THP) might be considered "shared" if the large allocation is
>> +considered "shared": if not all pages are exclusive to the same process.
>> +Further, some kernel configurations might consider larger allocations "shared",
>> +if they were at one point considered "shared", even if they would now be
>> +considered "exclusive".
>> +
>> +Some kernel configurations do not track the precise number of times a page part
>> +of a larger allocation is mapped. In this case, when calculating the PSS, the
>> +average number of mappings per page in this larger allocation might be used
>> +as an approximation for the number of mappings of a page. The PSS calculation
>> +will be imprecise in this case.
>> +
>> "Referenced" indicates the amount of memory currently marked as referenced or
>> accessed.
>>
>> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
>> index 16aa1fd260771..70205425a2daa 100644
>> --- a/fs/proc/internal.h
>> +++ b/fs/proc/internal.h
>> @@ -143,6 +143,7 @@ unsigned name_to_int(const struct qstr *qstr);
>> /* Worst case buffer size needed for holding an integer. */
>> #define PROC_NUMBUF 13
>>
>> +#ifdef CONFIG_PAGE_MAPCOUNT
>> /**
>> * folio_precise_page_mapcount() - Number of mappings of this folio page.
>> * @folio: The folio.
>> @@ -173,6 +174,13 @@ static inline int folio_precise_page_mapcount(struct folio *folio,
>>
>> return mapcount;
>> }
>> +#else /* !CONFIG_PAGE_MAPCOUNT */
>> +static inline int folio_precise_page_mapcount(struct folio *folio,
>> + struct page *page)
>> +{
>> + BUILD_BUG();
>> +}
>> +#endif /* CONFIG_PAGE_MAPCOUNT */
>>
>> /**
>> * folio_average_page_mapcount() - Average number of mappings per page in this
>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>> index d7ee842367f0f..7ca0bc3bf417d 100644
>> --- a/fs/proc/task_mmu.c
>> +++ b/fs/proc/task_mmu.c
>> @@ -707,6 +707,8 @@ static void smaps_account(struct mem_size_stats *mss, struct page *page,
>> struct folio *folio = page_folio(page);
>> int i, nr = compound ? compound_nr(page) : 1;
>> unsigned long size = nr * PAGE_SIZE;
>> + bool exclusive;
>> + int mapcount;
>>
>> /*
>> * First accumulate quantities that depend only on |size| and the type
>> @@ -747,18 +749,29 @@ static void smaps_account(struct mem_size_stats *mss, struct page *page,
>> dirty, locked, present);
>> return;
>> }
>> +
>> + if (IS_ENABLED(CONFIG_NO_PAGE_MAPCOUNT)) {
>> + mapcount = folio_average_page_mapcount(folio);
>
> This seems inconsistent with how folio_average_page_mapcount() is used
> in patch 16 and 18.
It only looks that way. Note that the code below only does
>> if (mapcount >= 2)
>> pss /= mapcount;
Having it as 0 or 1 doesn't matter in that regard.
Thanks!
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 16/20] fs/proc/page: remove per-page mapcount dependency for /proc/kpagecount (CONFIG_NO_PAGE_MAPCOUNT)
From: David Hildenbrand @ 2025-02-24 21:15 UTC (permalink / raw)
To: Zi Yan
Cc: linux-kernel, linux-doc, cgroups, linux-mm, linux-fsdevel,
linux-api, Andrew Morton, Matthew Wilcox (Oracle), Tejun Heo,
Zefan Li, Johannes Weiner, Michal Koutný, Jonathan Corbet,
Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Muchun Song, Liam R. Howlett, Lorenzo Stoakes,
Vlastimil Babka, Jann Horn
In-Reply-To: <9010E213-9FC5-4900-B971-D032CB879F2E@nvidia.com>
On 24.02.25 22:10, Zi Yan wrote:
> On 24 Feb 2025, at 16:02, David Hildenbrand wrote:
>
>> On 24.02.25 21:40, Zi Yan wrote:
>>> On Mon Feb 24, 2025 at 11:55 AM EST, David Hildenbrand wrote:
>>>> Let's implement an alternative when per-page mapcounts in large folios
>>>> are no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
>>>>
>>>> For large folios, we'll return the per-page average mapcount within the
>>>> folio, except when the average is 0 but the folio is mapped: then we
>>>> return 1.
>>>>
>>>> For hugetlb folios and for large folios that are fully mapped
>>>> into all address spaces, there is no change.
>>>>
>>>> As an alternative, we could simply return 0 for non-hugetlb large folios,
>>>> or disable this legacy interface with CONFIG_NO_PAGE_MAPCOUNT.
>>>>
>>>> But the information exposed by this interface can still be valuable, and
>>>> frequently we deal with fully-mapped large folios where the average
>>>> corresponds to the actual page mapcount. So we'll leave it like this for
>>>> now and document the new behavior.
>>>>
>>>> Note: this interface is likely not very relevant for performance. If
>>>> ever required, we could try doing a rather expensive rmap walk to collect
>>>> precisely how often this folio page is mapped.
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>> Documentation/admin-guide/mm/pagemap.rst | 7 +++++-
>>>> fs/proc/internal.h | 31 ++++++++++++++++++++++++
>>>> fs/proc/page.c | 19 ++++++++++++---
>>>> 3 files changed, 53 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst
>>>> index caba0f52dd36c..49590306c61a0 100644
>>>> --- a/Documentation/admin-guide/mm/pagemap.rst
>>>> +++ b/Documentation/admin-guide/mm/pagemap.rst
>>>> @@ -42,7 +42,12 @@ There are four components to pagemap:
>>>> skip over unmapped regions.
>>>> * ``/proc/kpagecount``. This file contains a 64-bit count of the number of
>>>> - times each page is mapped, indexed by PFN.
>>>> + times each page is mapped, indexed by PFN. Some kernel configurations do
>>>> + not track the precise number of times a page part of a larger allocation
>>>> + (e.g., THP) is mapped. In these configurations, the average number of
>>>> + mappings per page in this larger allocation is returned instead. However,
>>>> + if any page of the large allocation is mapped, the returned value will
>>>> + be at least 1.
>>>> The page-types tool in the tools/mm directory can be used to query the
>>>> number of times a page is mapped.
>>>> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
>>>> index 1695509370b88..16aa1fd260771 100644
>>>> --- a/fs/proc/internal.h
>>>> +++ b/fs/proc/internal.h
>>>> @@ -174,6 +174,37 @@ static inline int folio_precise_page_mapcount(struct folio *folio,
>>>> return mapcount;
>>>> }
>>>> +/**
>>>> + * folio_average_page_mapcount() - Average number of mappings per page in this
>>>> + * folio
>>>> + * @folio: The folio.
>>>> + *
>>>> + * The average number of present user page table entries that reference each
>>>> + * page in this folio as tracked via the RMAP: either referenced directly
>>>> + * (PTE) or as part of a larger area that covers this page (e.g., PMD).
>>>> + *
>>>> + * Returns: The average number of mappings per page in this folio. 0 for
>>>> + * folios that are not mapped to user space or are not tracked via the RMAP
>>>> + * (e.g., shared zeropage).
>>>> + */
>>>> +static inline int folio_average_page_mapcount(struct folio *folio)
>>>> +{
>>>> + int mapcount, entire_mapcount;
>>>> + unsigned int adjust;
>>>> +
>>>> + if (!folio_test_large(folio))
>>>> + return atomic_read(&folio->_mapcount) + 1;
>>>> +
>>>> + mapcount = folio_large_mapcount(folio);
>>>> + entire_mapcount = folio_entire_mapcount(folio);
>>>> + if (mapcount <= entire_mapcount)
>>>> + return entire_mapcount;
>>>> + mapcount -= entire_mapcount;
>>>> +
>>>> + adjust = folio_large_nr_pages(folio) / 2;
>>
>> Thanks for the review!
>>
>>>
>>> Is there any reason for choosing this adjust number? A comment might be
>>> helpful in case people want to change it later, either with some reasoning
>>> or just saying it is chosen empirically.
>>
>> We're dividing by folio_large_nr_pages(folio) (shifting by folio_large_order(folio)), so this is not a magic number at all.
>>
>> So this should be "ordinary" rounding.
>
> I thought the rounding would be (mapcount + 511) / 512.
Yes, that's "rounding up".
> But
> that means if one subpage is mapped, the average will be 1.
> Your rounding means if at least half of the subpages is mapped,
> the average will be 1. Others might think 1/3 is mapped,
> the average will be 1. That is why I think adjust looks like
> a magic number.
I think all callers could tolerate (or benefit) from
folio_average_page_mapcount() returning at least 1 in case any page is
mapped.
There was a reason why I decided to round to the nearest integer instead.
Let me think about this once more, I went back and forth a couple of
times on this.
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 16/20] fs/proc/page: remove per-page mapcount dependency for /proc/kpagecount (CONFIG_NO_PAGE_MAPCOUNT)
From: Zi Yan @ 2025-02-24 21:10 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-doc, cgroups, linux-mm, linux-fsdevel,
linux-api, Andrew Morton, Matthew Wilcox (Oracle), Tejun Heo,
Zefan Li, Johannes Weiner, Michal Koutný, Jonathan Corbet,
Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Muchun Song, Liam R. Howlett, Lorenzo Stoakes,
Vlastimil Babka, Jann Horn
In-Reply-To: <8a5e94a2-8cd7-45f5-a2be-525242c0cd16@redhat.com>
On 24 Feb 2025, at 16:02, David Hildenbrand wrote:
> On 24.02.25 21:40, Zi Yan wrote:
>> On Mon Feb 24, 2025 at 11:55 AM EST, David Hildenbrand wrote:
>>> Let's implement an alternative when per-page mapcounts in large folios
>>> are no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
>>>
>>> For large folios, we'll return the per-page average mapcount within the
>>> folio, except when the average is 0 but the folio is mapped: then we
>>> return 1.
>>>
>>> For hugetlb folios and for large folios that are fully mapped
>>> into all address spaces, there is no change.
>>>
>>> As an alternative, we could simply return 0 for non-hugetlb large folios,
>>> or disable this legacy interface with CONFIG_NO_PAGE_MAPCOUNT.
>>>
>>> But the information exposed by this interface can still be valuable, and
>>> frequently we deal with fully-mapped large folios where the average
>>> corresponds to the actual page mapcount. So we'll leave it like this for
>>> now and document the new behavior.
>>>
>>> Note: this interface is likely not very relevant for performance. If
>>> ever required, we could try doing a rather expensive rmap walk to collect
>>> precisely how often this folio page is mapped.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>> Documentation/admin-guide/mm/pagemap.rst | 7 +++++-
>>> fs/proc/internal.h | 31 ++++++++++++++++++++++++
>>> fs/proc/page.c | 19 ++++++++++++---
>>> 3 files changed, 53 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst
>>> index caba0f52dd36c..49590306c61a0 100644
>>> --- a/Documentation/admin-guide/mm/pagemap.rst
>>> +++ b/Documentation/admin-guide/mm/pagemap.rst
>>> @@ -42,7 +42,12 @@ There are four components to pagemap:
>>> skip over unmapped regions.
>>> * ``/proc/kpagecount``. This file contains a 64-bit count of the number of
>>> - times each page is mapped, indexed by PFN.
>>> + times each page is mapped, indexed by PFN. Some kernel configurations do
>>> + not track the precise number of times a page part of a larger allocation
>>> + (e.g., THP) is mapped. In these configurations, the average number of
>>> + mappings per page in this larger allocation is returned instead. However,
>>> + if any page of the large allocation is mapped, the returned value will
>>> + be at least 1.
>>> The page-types tool in the tools/mm directory can be used to query the
>>> number of times a page is mapped.
>>> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
>>> index 1695509370b88..16aa1fd260771 100644
>>> --- a/fs/proc/internal.h
>>> +++ b/fs/proc/internal.h
>>> @@ -174,6 +174,37 @@ static inline int folio_precise_page_mapcount(struct folio *folio,
>>> return mapcount;
>>> }
>>> +/**
>>> + * folio_average_page_mapcount() - Average number of mappings per page in this
>>> + * folio
>>> + * @folio: The folio.
>>> + *
>>> + * The average number of present user page table entries that reference each
>>> + * page in this folio as tracked via the RMAP: either referenced directly
>>> + * (PTE) or as part of a larger area that covers this page (e.g., PMD).
>>> + *
>>> + * Returns: The average number of mappings per page in this folio. 0 for
>>> + * folios that are not mapped to user space or are not tracked via the RMAP
>>> + * (e.g., shared zeropage).
>>> + */
>>> +static inline int folio_average_page_mapcount(struct folio *folio)
>>> +{
>>> + int mapcount, entire_mapcount;
>>> + unsigned int adjust;
>>> +
>>> + if (!folio_test_large(folio))
>>> + return atomic_read(&folio->_mapcount) + 1;
>>> +
>>> + mapcount = folio_large_mapcount(folio);
>>> + entire_mapcount = folio_entire_mapcount(folio);
>>> + if (mapcount <= entire_mapcount)
>>> + return entire_mapcount;
>>> + mapcount -= entire_mapcount;
>>> +
>>> + adjust = folio_large_nr_pages(folio) / 2;
>
> Thanks for the review!
>
>>
>> Is there any reason for choosing this adjust number? A comment might be
>> helpful in case people want to change it later, either with some reasoning
>> or just saying it is chosen empirically.
>
> We're dividing by folio_large_nr_pages(folio) (shifting by folio_large_order(folio)), so this is not a magic number at all.
>
> So this should be "ordinary" rounding.
I thought the rounding would be (mapcount + 511) / 512. But
that means if one subpage is mapped, the average will be 1.
Your rounding means if at least half of the subpages is mapped,
the average will be 1. Others might think 1/3 is mapped,
the average will be 1. That is why I think adjust looks like
a magic number.
>
> Assume nr_pages = 512.
>
> With 255 we want to round down, with 256 we want to round up.
>
> 255 / 512 = 0 :)
> 256 / 512 = 0 :(
>
> Compared to:
>
> (255 + (512 / 2)) / 512 = (255 + 256) / 512 = 0 :)
> (256 + (512 / 2)) / 512 = (256 + 256) / 512 = 1 :)
>
>>
>>> + return ((mapcount + adjust) >> folio_large_order(folio)) +
>>> + entire_mapcount;
>>> +}
>>> /*
>>> * array.c
>>> */
>>> diff --git a/fs/proc/page.c b/fs/proc/page.c
>>> index a55f5acefa974..4d3290cc69667 100644
>>> --- a/fs/proc/page.c
>>> +++ b/fs/proc/page.c
>>> @@ -67,9 +67,22 @@ static ssize_t kpagecount_read(struct file *file, char __user *buf,
>>> * memmaps that were actually initialized.
>>> */
>>> page = pfn_to_online_page(pfn);
>>> - if (page)
>>> - mapcount = folio_precise_page_mapcount(page_folio(page),
>>> - page);
>>> + if (page) {
>>> + struct folio *folio = page_folio(page);
>>> +
>>> + if (IS_ENABLED(CONFIG_PAGE_MAPCOUNT)) {
>>> + mapcount = folio_precise_page_mapcount(folio, page);
>>> + } else {
>>> + /*
>>> + * Indicate the per-page average, but at least "1" for
>>> + * mapped folios.
>>> + */
>>> + mapcount = folio_average_page_mapcount(folio);
>>> + if (!mapcount && folio_test_large(folio) &&
>>> + folio_mapped(folio))
>>> + mapcount = 1;
>>
>> This should be part of folio_average_page_mapcount() right?
>
> No, that's not desired.
>
>> Otherwise, the comment on folio_average_page_mapcount() is not correct,
>> since it can return 0 when a folio is mapped to user space.
>
> It's misleading. I'll clarify the comment, probably simply saying:
>
> Returns: The average number of mappings per page in this folio.
Got it.
Best Regards,
Yan, Zi
^ permalink raw reply
* Re: [PATCH v2 16/20] fs/proc/page: remove per-page mapcount dependency for /proc/kpagecount (CONFIG_NO_PAGE_MAPCOUNT)
From: David Hildenbrand @ 2025-02-24 21:10 UTC (permalink / raw)
To: Zi Yan, linux-kernel
Cc: linux-doc, cgroups, linux-mm, linux-fsdevel, linux-api,
Andrew Morton, Matthew Wilcox (Oracle), Tejun Heo, Zefan Li,
Johannes Weiner, Michal Koutný, Jonathan Corbet,
Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Muchun Song, Liam R. Howlett, Lorenzo Stoakes,
Vlastimil Babka, Jann Horn, owner-linux-mm
In-Reply-To: <8a5e94a2-8cd7-45f5-a2be-525242c0cd16@redhat.com>
On 24.02.25 22:02, David Hildenbrand wrote:
> On 24.02.25 21:40, Zi Yan wrote:
>> On Mon Feb 24, 2025 at 11:55 AM EST, David Hildenbrand wrote:
>>> Let's implement an alternative when per-page mapcounts in large folios
>>> are no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
>>>
>>> For large folios, we'll return the per-page average mapcount within the
>>> folio, except when the average is 0 but the folio is mapped: then we
>>> return 1.
>>>
>>> For hugetlb folios and for large folios that are fully mapped
>>> into all address spaces, there is no change.
>>>
>>> As an alternative, we could simply return 0 for non-hugetlb large folios,
>>> or disable this legacy interface with CONFIG_NO_PAGE_MAPCOUNT.
>>>
>>> But the information exposed by this interface can still be valuable, and
>>> frequently we deal with fully-mapped large folios where the average
>>> corresponds to the actual page mapcount. So we'll leave it like this for
>>> now and document the new behavior.
>>>
>>> Note: this interface is likely not very relevant for performance. If
>>> ever required, we could try doing a rather expensive rmap walk to collect
>>> precisely how often this folio page is mapped.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>> Documentation/admin-guide/mm/pagemap.rst | 7 +++++-
>>> fs/proc/internal.h | 31 ++++++++++++++++++++++++
>>> fs/proc/page.c | 19 ++++++++++++---
>>> 3 files changed, 53 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst
>>> index caba0f52dd36c..49590306c61a0 100644
>>> --- a/Documentation/admin-guide/mm/pagemap.rst
>>> +++ b/Documentation/admin-guide/mm/pagemap.rst
>>> @@ -42,7 +42,12 @@ There are four components to pagemap:
>>> skip over unmapped regions.
>>>
>>> * ``/proc/kpagecount``. This file contains a 64-bit count of the number of
>>> - times each page is mapped, indexed by PFN.
>>> + times each page is mapped, indexed by PFN. Some kernel configurations do
>>> + not track the precise number of times a page part of a larger allocation
>>> + (e.g., THP) is mapped. In these configurations, the average number of
>>> + mappings per page in this larger allocation is returned instead. However,
>>> + if any page of the large allocation is mapped, the returned value will
>>> + be at least 1.
>>>
>>> The page-types tool in the tools/mm directory can be used to query the
>>> number of times a page is mapped.
>>> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
>>> index 1695509370b88..16aa1fd260771 100644
>>> --- a/fs/proc/internal.h
>>> +++ b/fs/proc/internal.h
>>> @@ -174,6 +174,37 @@ static inline int folio_precise_page_mapcount(struct folio *folio,
>>> return mapcount;
>>> }
>>>
>>> +/**
>>> + * folio_average_page_mapcount() - Average number of mappings per page in this
>>> + * folio
>>> + * @folio: The folio.
>>> + *
>>> + * The average number of present user page table entries that reference each
>>> + * page in this folio as tracked via the RMAP: either referenced directly
>>> + * (PTE) or as part of a larger area that covers this page (e.g., PMD).
>>> + *
>>> + * Returns: The average number of mappings per page in this folio. 0 for
>>> + * folios that are not mapped to user space or are not tracked via the RMAP
>>> + * (e.g., shared zeropage).
>>> + */
>>> +static inline int folio_average_page_mapcount(struct folio *folio)
>>> +{
>>> + int mapcount, entire_mapcount;
>>> + unsigned int adjust;
>>> +
>>> + if (!folio_test_large(folio))
>>> + return atomic_read(&folio->_mapcount) + 1;
>>> +
>>> + mapcount = folio_large_mapcount(folio);
>>> + entire_mapcount = folio_entire_mapcount(folio);
>>> + if (mapcount <= entire_mapcount)
>>> + return entire_mapcount;
>>> + mapcount -= entire_mapcount;
>>> +
>>> + adjust = folio_large_nr_pages(folio) / 2;
>
> Thanks for the review!
>
>>
>> Is there any reason for choosing this adjust number? A comment might be
>> helpful in case people want to change it later, either with some reasoning
>> or just saying it is chosen empirically.
>
> We're dividing by folio_large_nr_pages(folio) (shifting by
> folio_large_order(folio)), so this is not a magic number at all.
>
> So this should be "ordinary" rounding.
>
> Assume nr_pages = 512.
>
> With 255 we want to round down, with 256 we want to round up.
>
> 255 / 512 = 0 :)
> 256 / 512 = 0 :(
>
> Compared to:
>
> (255 + (512 / 2)) / 512 = (255 + 256) / 512 = 0 :)
> (256 + (512 / 2)) / 512 = (256 + 256) / 512 = 1 :)
I think adding to the function doc:
"The average is calculated by rounding to the nearest integer."
might make it clearer.
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 18/20] fs/proc/task_mmu: remove per-page mapcount dependency for "mapmax" (CONFIG_NO_PAGE_MAPCOUNT)
From: David Hildenbrand @ 2025-02-24 21:04 UTC (permalink / raw)
To: Zi Yan, linux-kernel
Cc: linux-doc, cgroups, linux-mm, linux-fsdevel, linux-api,
Andrew Morton, Matthew Wilcox (Oracle), Tejun Heo, Zefan Li,
Johannes Weiner, Michal Koutný, Jonathan Corbet,
Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Muchun Song, Liam R. Howlett, Lorenzo Stoakes,
Vlastimil Babka, Jann Horn, owner-linux-mm
In-Reply-To: <D80YXDU2A6IE.S4PQYSOT0PYI@nvidia.com>
On 24.02.25 21:45, Zi Yan wrote:
> On Mon Feb 24, 2025 at 11:56 AM EST, David Hildenbrand wrote:
>> Let's implement an alternative when per-page mapcounts in large folios are
>> no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
>>
>> For calculating "mapmax", we now use the average per-page mapcount in
>> a large folio instead of the per-page mapcount.
>>
>> For hugetlb folios and folios that are not partially mapped into MMs,
>> there is no change.
>>
>> Likely, this change will not matter much in practice, and an alternative
>> might be to simple remove this stat with CONFIG_NO_PAGE_MAPCOUNT.
>> However, there might be value to it, so let's keep it like that and
>> document the behavior.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> Documentation/filesystems/proc.rst | 5 +++++
>> fs/proc/task_mmu.c | 7 ++++++-
>> 2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
>> index 09f0aed5a08ba..1aa190017f796 100644
>> --- a/Documentation/filesystems/proc.rst
>> +++ b/Documentation/filesystems/proc.rst
>> @@ -686,6 +686,11 @@ Where:
>> node locality page counters (N0 == node0, N1 == node1, ...) and the kernel page
>> size, in KB, that is backing the mapping up.
>>
>> +Note that some kernel configurations do not track the precise number of times
>> +a page part of a larger allocation (e.g., THP) is mapped. In these
>> +configurations, "mapmax" might corresponds to the average number of mappings
>> +per page in such a larger allocation instead.
>> +
>> 1.2 Kernel data
>> ---------------
>>
>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>> index 80839bbf9657f..d7ee842367f0f 100644
>> --- a/fs/proc/task_mmu.c
>> +++ b/fs/proc/task_mmu.c
>> @@ -2862,7 +2862,12 @@ static void gather_stats(struct page *page, struct numa_maps *md, int pte_dirty,
>> unsigned long nr_pages)
>> {
>> struct folio *folio = page_folio(page);
>> - int count = folio_precise_page_mapcount(folio, page);
>> + int count;
>> +
>> + if (IS_ENABLED(CONFIG_PAGE_MAPCOUNT))
>> + count = folio_precise_page_mapcount(folio, page);
>> + else
>> + count = min_t(int, folio_average_page_mapcount(folio), 1);
>
> s/min/max ?
Indeed, thanks!
>
> Otherwise, count is at most 1. Anyway, if you change
> folio_average_page_mapcount() as I indicated in patch 16, this
> will become count = folio_average_page_mapcount(folio).
No, the average should not be 1 just because a single subpage is mapped.
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 16/20] fs/proc/page: remove per-page mapcount dependency for /proc/kpagecount (CONFIG_NO_PAGE_MAPCOUNT)
From: David Hildenbrand @ 2025-02-24 21:02 UTC (permalink / raw)
To: Zi Yan, linux-kernel
Cc: linux-doc, cgroups, linux-mm, linux-fsdevel, linux-api,
Andrew Morton, Matthew Wilcox (Oracle), Tejun Heo, Zefan Li,
Johannes Weiner, Michal Koutný, Jonathan Corbet,
Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Muchun Song, Liam R. Howlett, Lorenzo Stoakes,
Vlastimil Babka, Jann Horn, owner-linux-mm
In-Reply-To: <D80YSXJPTL7M.2GZLUFXVP2ZCC@nvidia.com>
On 24.02.25 21:40, Zi Yan wrote:
> On Mon Feb 24, 2025 at 11:55 AM EST, David Hildenbrand wrote:
>> Let's implement an alternative when per-page mapcounts in large folios
>> are no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
>>
>> For large folios, we'll return the per-page average mapcount within the
>> folio, except when the average is 0 but the folio is mapped: then we
>> return 1.
>>
>> For hugetlb folios and for large folios that are fully mapped
>> into all address spaces, there is no change.
>>
>> As an alternative, we could simply return 0 for non-hugetlb large folios,
>> or disable this legacy interface with CONFIG_NO_PAGE_MAPCOUNT.
>>
>> But the information exposed by this interface can still be valuable, and
>> frequently we deal with fully-mapped large folios where the average
>> corresponds to the actual page mapcount. So we'll leave it like this for
>> now and document the new behavior.
>>
>> Note: this interface is likely not very relevant for performance. If
>> ever required, we could try doing a rather expensive rmap walk to collect
>> precisely how often this folio page is mapped.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> Documentation/admin-guide/mm/pagemap.rst | 7 +++++-
>> fs/proc/internal.h | 31 ++++++++++++++++++++++++
>> fs/proc/page.c | 19 ++++++++++++---
>> 3 files changed, 53 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst
>> index caba0f52dd36c..49590306c61a0 100644
>> --- a/Documentation/admin-guide/mm/pagemap.rst
>> +++ b/Documentation/admin-guide/mm/pagemap.rst
>> @@ -42,7 +42,12 @@ There are four components to pagemap:
>> skip over unmapped regions.
>>
>> * ``/proc/kpagecount``. This file contains a 64-bit count of the number of
>> - times each page is mapped, indexed by PFN.
>> + times each page is mapped, indexed by PFN. Some kernel configurations do
>> + not track the precise number of times a page part of a larger allocation
>> + (e.g., THP) is mapped. In these configurations, the average number of
>> + mappings per page in this larger allocation is returned instead. However,
>> + if any page of the large allocation is mapped, the returned value will
>> + be at least 1.
>>
>> The page-types tool in the tools/mm directory can be used to query the
>> number of times a page is mapped.
>> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
>> index 1695509370b88..16aa1fd260771 100644
>> --- a/fs/proc/internal.h
>> +++ b/fs/proc/internal.h
>> @@ -174,6 +174,37 @@ static inline int folio_precise_page_mapcount(struct folio *folio,
>> return mapcount;
>> }
>>
>> +/**
>> + * folio_average_page_mapcount() - Average number of mappings per page in this
>> + * folio
>> + * @folio: The folio.
>> + *
>> + * The average number of present user page table entries that reference each
>> + * page in this folio as tracked via the RMAP: either referenced directly
>> + * (PTE) or as part of a larger area that covers this page (e.g., PMD).
>> + *
>> + * Returns: The average number of mappings per page in this folio. 0 for
>> + * folios that are not mapped to user space or are not tracked via the RMAP
>> + * (e.g., shared zeropage).
>> + */
>> +static inline int folio_average_page_mapcount(struct folio *folio)
>> +{
>> + int mapcount, entire_mapcount;
>> + unsigned int adjust;
>> +
>> + if (!folio_test_large(folio))
>> + return atomic_read(&folio->_mapcount) + 1;
>> +
>> + mapcount = folio_large_mapcount(folio);
>> + entire_mapcount = folio_entire_mapcount(folio);
>> + if (mapcount <= entire_mapcount)
>> + return entire_mapcount;
>> + mapcount -= entire_mapcount;
>> +
>> + adjust = folio_large_nr_pages(folio) / 2;
Thanks for the review!
>
> Is there any reason for choosing this adjust number? A comment might be
> helpful in case people want to change it later, either with some reasoning
> or just saying it is chosen empirically.
We're dividing by folio_large_nr_pages(folio) (shifting by
folio_large_order(folio)), so this is not a magic number at all.
So this should be "ordinary" rounding.
Assume nr_pages = 512.
With 255 we want to round down, with 256 we want to round up.
255 / 512 = 0 :)
256 / 512 = 0 :(
Compared to:
(255 + (512 / 2)) / 512 = (255 + 256) / 512 = 0 :)
(256 + (512 / 2)) / 512 = (256 + 256) / 512 = 1 :)
>
>> + return ((mapcount + adjust) >> folio_large_order(folio)) +
>> + entire_mapcount;
>> +}
>> /*
>> * array.c
>> */
>> diff --git a/fs/proc/page.c b/fs/proc/page.c
>> index a55f5acefa974..4d3290cc69667 100644
>> --- a/fs/proc/page.c
>> +++ b/fs/proc/page.c
>> @@ -67,9 +67,22 @@ static ssize_t kpagecount_read(struct file *file, char __user *buf,
>> * memmaps that were actually initialized.
>> */
>> page = pfn_to_online_page(pfn);
>> - if (page)
>> - mapcount = folio_precise_page_mapcount(page_folio(page),
>> - page);
>> + if (page) {
>> + struct folio *folio = page_folio(page);
>> +
>> + if (IS_ENABLED(CONFIG_PAGE_MAPCOUNT)) {
>> + mapcount = folio_precise_page_mapcount(folio, page);
>> + } else {
>> + /*
>> + * Indicate the per-page average, but at least "1" for
>> + * mapped folios.
>> + */
>> + mapcount = folio_average_page_mapcount(folio);
>> + if (!mapcount && folio_test_large(folio) &&
>> + folio_mapped(folio))
>> + mapcount = 1;
>
> This should be part of folio_average_page_mapcount() right?
No, that's not desired.
> Otherwise, the comment on folio_average_page_mapcount() is not correct,
> since it can return 0 when a folio is mapped to user space.
It's misleading. I'll clarify the comment, probably simply saying:
Returns: The average number of mappings per page in this folio.
Thanks!
--
Cheers,
David / dhildenb
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox