[PATCH RFC 0/4] static key support for error injection functions

[PATCH RFC 0/4] static key support for error injection functions

[Vlastimil Babka]

Incomplete, help needed from ftrace/kprobe and bpf folks.

As previously mentioned by myself [1] and others [2] the functions
designed for error injection can bring visible overhead in fastpaths
such as slab or page allocation, because even if nothing hooks into them
at a given moment, they are noninline function calls regardless of
CONFIG_ options since commits 4f6923fbb352 ("mm: make should_failslab
always available for fault injection") and af3b854492f3
("mm/page_alloc.c: allow error injection").

Live patching their callsites has been also suggested in both [1] and
[2] threads, and this is an attempt to do that with static keys that
guard the call sites. When disabled, the error injection functions still
exist and are noinline, but are not being called. Any of the existing
mechanisms that can inject errors should make sure to enable the
respective static key. I have added that support to some of them but
need help with the others.

- the legacy fault injection, i.e. CONFIG_FAILSLAB and
  CONFIG_FAIL_PAGE_ALLOC is handled in Patch 1, and can be passed the
  address of the static key if it exists. The key will be activated if the
  fault injection probability becomes non-zero, and deactivated in the
  opposite transition. This also removes the overhead of the evaluation
  (on top of the noninline function call) when these mechanisms are
  configured in the kernel but unused at the moment.

- the generic error injection using kretprobes with
  override_function_with_return is handled in patch 2. The
  ALLOW_ERROR_INJECTION() annotation is extended so that static key
  address can be passed, and the framework controls it when error
  injection is enabled or disabled in debugfs for the function.

There are two more users I know of but am not familiar enough to fix up
myself. I hope people that are more familiar can help me here.

- ftrace seems to be using override_function_with_return from
  #define ftrace_override_function_with_return but I found no place
  where the latter is used. I assume it might be hidden behind more
  macro magic? But the point is if ftrace can be instructed to act like
  an error injection, it would also have to use some form of metadata
  (from patch 2 presumably?) to get to the static key and control it.

  If ftrace can only observe the function being called, maybe it
  wouldn't be wrong to just observe nothing if the static key isn't
  enabled because nobody is doing the fault injection?

- bpftrace, as can be seen from the example in commit 4f6923fbb352
  description. I suppose bpf is already aware what functions the
  currently loaded bpf programs hook into, so that it could look up the
  static key and control it. Maybe using again the metadata from patch 2,
  or extending its own, as I've noticed there's e.g. BTF_ID(func,
  should_failslab)

Now I realize maybe handling this at the k(ret)probe level would be
sufficient for all cases except the legacy fault injection from Patch 1?
Also wanted to note that by AFAIU by using the static_key_slow_dec/inc
API (as done in patches 1/2) should allow all mechanisms to coexist
naturally without fighting each other on the static key state, and also
handle the reference count for e.g. active probes or bpf programs if
there's no similar internal mechanism.

Patches 3 and 4 implement the static keys for the two mm fault injection
sites in slab and page allocators. For a quick demonstration I've run a
VM and the simple test from [1] that stresses the slab allocator and got
this time before the series:

real    0m8.349s
user    0m0.694s
sys     0m7.648s

with perf showing

   0.61%  nonexistent  [kernel.kallsyms]  [k] should_failslab.constprop.0
   0.00%  nonexistent  [kernel.kallsyms]  [k] should_fail_alloc_page                                                                                                                                                                                        ▒

And after the series

real    0m7.924s
user    0m0.727s
sys     0m7.191s

and the functions gone from perf report.

There might be other such fault injection callsites in hotpaths of other
subsystems but I didn't search for them at this point.

[1] https://lore.kernel.org/all/6d5bb852-8703-4abf-a52b-90816bccbd7f@suse.cz/
[2] https://lore.kernel.org/all/3j5d3p22ssv7xoaghzraa7crcfih3h2qqjlhmjppbp6f42pg2t@kg7qoicog5ye/

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
Vlastimil Babka (4):
      fault-inject: add support for static keys around fault injection sites
      error-injection: support static keys around injectable functions
      mm, slab: add static key for should_failslab()
      mm, page_alloc: add static key for should_fail_alloc_page()

 include/asm-generic/error-injection.h | 13 ++++++++++-
 include/asm-generic/vmlinux.lds.h     |  2 +-
 include/linux/error-injection.h       |  9 +++++---
 include/linux/fault-inject.h          |  7 +++++-
 kernel/fail_function.c                | 22 +++++++++++++++---
 lib/error-inject.c                    |  6 ++++-
 lib/fault-inject.c                    | 43 ++++++++++++++++++++++++++++++++++-
 mm/fail_page_alloc.c                  |  3 ++-
 mm/failslab.c                         |  2 +-
 mm/internal.h                         |  2 ++
 mm/page_alloc.c                       | 11 ++++++---
 mm/slab.h                             |  3 +++
 mm/slub.c                             | 10 +++++---
 13 files changed, 114 insertions(+), 19 deletions(-)
---
base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
change-id: 20240530-fault-injection-statickeys-66b7222e91b7

Best regards,
-- 
Vlastimil Babka <vbabka@suse.cz>

[PATCH RFC 1/4] fault-inject: add support for static keys around fault injection sites

[Vlastimil Babka]

Some fault injection sites are placed in hotpaths and incur overhead
even if not enabled, due to one or more function calls leading up to
should_fail_ex() that returns false due to attr->probability == 0.

This overhead can be eliminated if the outermost call into the checks is
guarded with a static key, so add support for that. The framework should
be told that such static key exist for a fault_attr, by initializing
fault_attr->active with the static key address. When it's not NULL,
enable the static key from setup_fault_attr() when the fault probability
is non-zero.

Also wire up writing into debugfs "probability" file to enable or
disable the static key when transitioning between zero and non-zero
probability.

For now, do not add configfs interface support as the immediate plan is
to leverage this for should_failslab() and should_fail_alloc_page()
after other necessary preparatory changes, and none of the configfs
based fault injection users.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 include/linux/fault-inject.h |  7 ++++++-
 lib/fault-inject.c           | 43 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/include/linux/fault-inject.h b/include/linux/fault-inject.h
index 6d5edef09d45..cfe75cc1bac4 100644
--- a/include/linux/fault-inject.h
+++ b/include/linux/fault-inject.h
@@ -9,6 +9,7 @@
 #include <linux/configfs.h>
 #include <linux/ratelimit.h>
 #include <linux/atomic.h>
+#include <linux/jump_label.h>
 
 /*
  * For explanation of the elements of this struct, see
@@ -30,13 +31,14 @@ struct fault_attr {
 	unsigned long count;
 	struct ratelimit_state ratelimit_state;
 	struct dentry *dname;
+	struct static_key *active;
 };
 
 enum fault_flags {
 	FAULT_NOWARN =	1 << 0,
 };
 
-#define FAULT_ATTR_INITIALIZER {					\
+#define FAULT_ATTR_INITIALIZER_KEY(_key) {				\
 		.interval = 1,						\
 		.times = ATOMIC_INIT(1),				\
 		.require_end = ULONG_MAX,				\
@@ -44,8 +46,11 @@ enum fault_flags {
 		.ratelimit_state = RATELIMIT_STATE_INIT_DISABLED,	\
 		.verbose = 2,						\
 		.dname = NULL,						\
+		.active = (_key),					\
 	}
 
+#define FAULT_ATTR_INITIALIZER		FAULT_ATTR_INITIALIZER_KEY(NULL)
+
 #define DECLARE_FAULT_ATTR(name) struct fault_attr name = FAULT_ATTR_INITIALIZER
 int setup_fault_attr(struct fault_attr *attr, char *str);
 bool should_fail_ex(struct fault_attr *attr, ssize_t size, int flags);
diff --git a/lib/fault-inject.c b/lib/fault-inject.c
index d608f9b48c10..93c46d2ec106 100644
--- a/lib/fault-inject.c
+++ b/lib/fault-inject.c
@@ -35,6 +35,9 @@ int setup_fault_attr(struct fault_attr *attr, char *str)
 	atomic_set(&attr->times, times);
 	atomic_set(&attr->space, space);
 
+	if (probability != 0 && attr->active)
+		static_key_slow_inc(attr->active);
+
 	return 1;
 }
 EXPORT_SYMBOL_GPL(setup_fault_attr);
@@ -166,6 +169,12 @@ EXPORT_SYMBOL_GPL(should_fail);
 
 #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
 
+/*
+ * Protect updating probability from debugfs as that may trigger static key
+ * changes when changing between zero and non-zero.
+ */
+static DEFINE_MUTEX(probability_mutex);
+
 static int debugfs_ul_set(void *data, u64 val)
 {
 	*(unsigned long *)data = val;
@@ -186,6 +195,38 @@ static void debugfs_create_ul(const char *name, umode_t mode,
 	debugfs_create_file(name, mode, parent, value, &fops_ul);
 }
 
+static int debugfs_prob_set(void *data, u64 val)
+{
+	struct fault_attr *attr = data;
+
+	mutex_lock(&probability_mutex);
+
+	if (attr->active) {
+		if (attr->probability != 0 && val == 0) {
+			static_key_slow_dec(attr->active);
+		} else if (attr->probability == 0 && val != 0) {
+			static_key_slow_inc(attr->active);
+		}
+	}
+
+	attr->probability = val;
+
+	mutex_unlock(&probability_mutex);
+
+	return 0;
+}
+
+static int debugfs_prob_get(void *data, u64 *val)
+{
+	struct fault_attr *attr = data;
+
+	*val = attr->probability;
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(fops_prob, debugfs_prob_get, debugfs_prob_set, "%llu\n");
+
 #ifdef CONFIG_FAULT_INJECTION_STACKTRACE_FILTER
 
 static int debugfs_stacktrace_depth_set(void *data, u64 val)
@@ -218,7 +259,7 @@ struct dentry *fault_create_debugfs_attr(const char *name,
 	if (IS_ERR(dir))
 		return dir;
 
-	debugfs_create_ul("probability", mode, dir, &attr->probability);
+	debugfs_create_file("probability", mode, dir, &attr, &fops_prob);
 	debugfs_create_ul("interval", mode, dir, &attr->interval);
 	debugfs_create_atomic_t("times", mode, dir, &attr->times);
 	debugfs_create_atomic_t("space", mode, dir, &attr->space);

-- 
2.45.1

[PATCH RFC 2/4] error-injection: support static keys around injectable functions

[Vlastimil Babka]

Error injectable functions cannot be inlined and since some are called
from hot paths, this incurrs overhead even if no error injection is
enabled for them.

To remove this overhead when disabled, allow the callsites of error
injectable functions to put the calls behind a static key, which the
framework can control when error injection is enabled or disabled for
the function.

Introduce a new ALLOW_ERROR_INJECTION_KEY() macro that adds a parameter
with the static key's address, and store it in struct
error_injection_entry. This new field has caused a mismatch when
populating the injection list from the _error_injection_whitelist
section with the current STRUCT_ALIGN(), so change the alignment to 8.

During the population, copy the key's address also to struct ei_entry,
and make it possible to retrieve it along with the error type by
get_injectable_error_type().

Finally, make the processing of writes to the debugfs inject file enable
the static key when the function is added to the injection list, and
disable when removed.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 include/asm-generic/error-injection.h | 13 ++++++++++++-
 include/asm-generic/vmlinux.lds.h     |  2 +-
 include/linux/error-injection.h       |  9 ++++++---
 kernel/fail_function.c                | 22 +++++++++++++++++++---
 lib/error-inject.c                    |  6 +++++-
 5 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/include/asm-generic/error-injection.h b/include/asm-generic/error-injection.h
index b05253f68eaa..eed2731f3820 100644
--- a/include/asm-generic/error-injection.h
+++ b/include/asm-generic/error-injection.h
@@ -12,6 +12,7 @@ enum {
 
 struct error_injection_entry {
 	unsigned long	addr;
+	unsigned long	static_key_addr;
 	int		etype;
 };
 
@@ -25,16 +26,26 @@ struct pt_regs;
  * 'Error Injectable Functions' section.
  */
 #define ALLOW_ERROR_INJECTION(fname, _etype)				\
-static struct error_injection_entry __used				\
+static struct error_injection_entry __used __aligned(8)			\
 	__section("_error_injection_whitelist")				\
 	_eil_addr_##fname = {						\
 		.addr = (unsigned long)fname,				\
 		.etype = EI_ETYPE_##_etype,				\
 	}
 
+#define ALLOW_ERROR_INJECTION_KEY(fname, _etype, key)			\
+static struct error_injection_entry __used __aligned(8)			\
+	__section("_error_injection_whitelist")				\
+	_eil_addr_##fname = {						\
+		.addr = (unsigned long)fname,				\
+		.static_key_addr = (unsigned long)key,			\
+		.etype = EI_ETYPE_##_etype,				\
+	}
+
 void override_function_with_return(struct pt_regs *regs);
 #else
 #define ALLOW_ERROR_INJECTION(fname, _etype)
+#define ALLOW_ERROR_INJECTION_KEY(fname, _etype, key)
 
 static inline void override_function_with_return(struct pt_regs *regs) { }
 #endif
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 5703526d6ebf..1b15a0af2a00 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -248,7 +248,7 @@
 
 #ifdef CONFIG_FUNCTION_ERROR_INJECTION
 #define ERROR_INJECT_WHITELIST()			\
-	STRUCT_ALIGN();					\
+	. = ALIGN(8);					\
 	BOUNDED_SECTION(_error_injection_whitelist)
 #else
 #define ERROR_INJECT_WHITELIST()
diff --git a/include/linux/error-injection.h b/include/linux/error-injection.h
index 20e738f4eae8..bec81b57a9d5 100644
--- a/include/linux/error-injection.h
+++ b/include/linux/error-injection.h
@@ -6,10 +6,12 @@
 #include <linux/errno.h>
 #include <asm-generic/error-injection.h>
 
+struct static_key;
+
 #ifdef CONFIG_FUNCTION_ERROR_INJECTION
 
-extern bool within_error_injection_list(unsigned long addr);
-extern int get_injectable_error_type(unsigned long addr);
+bool within_error_injection_list(unsigned long addr);
+int get_injectable_error_type(unsigned long addr, struct static_key **key_addr);
 
 #else /* !CONFIG_FUNCTION_ERROR_INJECTION */
 
@@ -18,7 +20,8 @@ static inline bool within_error_injection_list(unsigned long addr)
 	return false;
 }
 
-static inline int get_injectable_error_type(unsigned long addr)
+static inline int get_injectable_error_type(unsigned long addr,
+					    struct static_key **key_addr)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/kernel/fail_function.c b/kernel/fail_function.c
index d971a0189319..9240eb137e00 100644
--- a/kernel/fail_function.c
+++ b/kernel/fail_function.c
@@ -27,15 +27,16 @@ struct fei_attr {
 	struct list_head list;
 	struct kprobe kp;
 	unsigned long retval;
+	struct static_key *key;
 };
 static DEFINE_MUTEX(fei_lock);
 static LIST_HEAD(fei_attr_list);
 static DECLARE_FAULT_ATTR(fei_fault_attr);
 static struct dentry *fei_debugfs_dir;
 
-static unsigned long adjust_error_retval(unsigned long addr, unsigned long retv)
+static unsigned long __adjust_error_retval(int type, unsigned long retv)
 {
-	switch (get_injectable_error_type(addr)) {
+	switch (type) {
 	case EI_ETYPE_NULL:
 		return 0;
 	case EI_ETYPE_ERRNO:
@@ -53,9 +54,17 @@ static unsigned long adjust_error_retval(unsigned long addr, unsigned long retv)
 	return retv;
 }
 
+static unsigned long adjust_error_retval(unsigned long addr, unsigned long retv)
+{
+	int type = get_injectable_error_type(addr, NULL);
+
+	return __adjust_error_retval(type, retv);
+}
+
 static struct fei_attr *fei_attr_new(const char *sym, unsigned long addr)
 {
 	struct fei_attr *attr;
+	int type;
 
 	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
 	if (attr) {
@@ -66,7 +75,10 @@ static struct fei_attr *fei_attr_new(const char *sym, unsigned long addr)
 		}
 		attr->kp.pre_handler = fei_kprobe_handler;
 		attr->kp.post_handler = fei_post_handler;
-		attr->retval = adjust_error_retval(addr, 0);
+
+		type = get_injectable_error_type(addr, &attr->key);
+		attr->retval = __adjust_error_retval(type, 0);
+
 		INIT_LIST_HEAD(&attr->list);
 	}
 	return attr;
@@ -218,6 +230,8 @@ static int fei_open(struct inode *inode, struct file *file)
 
 static void fei_attr_remove(struct fei_attr *attr)
 {
+	if (attr->key)
+		static_key_slow_dec(attr->key);
 	fei_debugfs_remove_attr(attr);
 	unregister_kprobe(&attr->kp);
 	list_del(&attr->list);
@@ -295,6 +309,8 @@ static ssize_t fei_write(struct file *file, const char __user *buffer,
 		fei_attr_free(attr);
 		goto out;
 	}
+	if (attr->key)
+		static_key_slow_inc(attr->key);
 	fei_debugfs_add_attr(attr);
 	list_add_tail(&attr->list, &fei_attr_list);
 	ret = count;
diff --git a/lib/error-inject.c b/lib/error-inject.c
index 887acd9a6ea6..e5f3b63f0dbb 100644
--- a/lib/error-inject.c
+++ b/lib/error-inject.c
@@ -17,6 +17,7 @@ struct ei_entry {
 	struct list_head list;
 	unsigned long start_addr;
 	unsigned long end_addr;
+	struct static_key *key;
 	int etype;
 	void *priv;
 };
@@ -37,7 +38,7 @@ bool within_error_injection_list(unsigned long addr)
 	return ret;
 }
 
-int get_injectable_error_type(unsigned long addr)
+int get_injectable_error_type(unsigned long addr, struct static_key **key_addr)
 {
 	struct ei_entry *ent;
 	int ei_type = -EINVAL;
@@ -46,6 +47,8 @@ int get_injectable_error_type(unsigned long addr)
 	list_for_each_entry(ent, &error_injection_list, list) {
 		if (addr >= ent->start_addr && addr < ent->end_addr) {
 			ei_type = ent->etype;
+			if (key_addr)
+				*key_addr = ent->key;
 			break;
 		}
 	}
@@ -86,6 +89,7 @@ static void populate_error_injection_list(struct error_injection_entry *start,
 		ent->start_addr = entry;
 		ent->end_addr = entry + size;
 		ent->etype = iter->etype;
+		ent->key = (struct static_key *) iter->static_key_addr;
 		ent->priv = priv;
 		INIT_LIST_HEAD(&ent->list);
 		list_add_tail(&ent->list, &error_injection_list);

-- 
2.45.1

[PATCH RFC 3/4] mm, slab: add static key for should_failslab()

[Vlastimil Babka]

Since commit 4f6923fbb352 ("mm: make should_failslab always available for
fault injection") should_failslab() is unconditionally a noinline
function. This adds visible overhead to the slab allocation hotpath,
even if the function is empty. With CONFIG_FAILSLAB=y there's additional
overhead when the functionality is not enabled by a boot parameter or
debugfs.

The overhead can be eliminated with a static key around the callsite.
Fault injection and error injection frameworks can now be told that the
this function has a static key associated, and are able to enable and
disable it accordingly.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/failslab.c |  2 +-
 mm/slab.h     |  3 +++
 mm/slub.c     | 10 +++++++---
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/mm/failslab.c b/mm/failslab.c
index ffc420c0e767..878fd08e5dac 100644
--- a/mm/failslab.c
+++ b/mm/failslab.c
@@ -9,7 +9,7 @@ static struct {
 	bool ignore_gfp_reclaim;
 	bool cache_filter;
 } failslab = {
-	.attr = FAULT_ATTR_INITIALIZER,
+	.attr = FAULT_ATTR_INITIALIZER_KEY(&should_failslab_active.key),
 	.ignore_gfp_reclaim = true,
 	.cache_filter = false,
 };
diff --git a/mm/slab.h b/mm/slab.h
index 5f8f47c5bee0..792e19cb37b8 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -11,6 +11,7 @@
 #include <linux/memcontrol.h>
 #include <linux/kfence.h>
 #include <linux/kasan.h>
+#include <linux/jump_label.h>
 
 /*
  * Internal slab definitions
@@ -160,6 +161,8 @@ static_assert(IS_ALIGNED(offsetof(struct slab, freelist), sizeof(freelist_aba_t)
  */
 #define slab_page(s) folio_page(slab_folio(s), 0)
 
+DECLARE_STATIC_KEY_FALSE(should_failslab_active);
+
 /*
  * If network-based swap is enabled, sl*b must keep track of whether pages
  * were allocated from pfmemalloc reserves.
diff --git a/mm/slub.c b/mm/slub.c
index 0809760cf789..3bb579760a37 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3874,13 +3874,15 @@ static __always_inline void maybe_wipe_obj_freeptr(struct kmem_cache *s,
 			0, sizeof(void *));
 }
 
+DEFINE_STATIC_KEY_FALSE(should_failslab_active);
+
 noinline int should_failslab(struct kmem_cache *s, gfp_t gfpflags)
 {
 	if (__should_failslab(s, gfpflags))
 		return -ENOMEM;
 	return 0;
 }
-ALLOW_ERROR_INJECTION(should_failslab, ERRNO);
+ALLOW_ERROR_INJECTION_KEY(should_failslab, ERRNO, &should_failslab_active);
 
 static __fastpath_inline
 struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s, gfp_t flags)
@@ -3889,8 +3891,10 @@ struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s, gfp_t flags)
 
 	might_alloc(flags);
 
-	if (unlikely(should_failslab(s, flags)))
-		return NULL;
+	if (static_branch_unlikely(&should_failslab_active)) {
+		if (should_failslab(s, flags))
+			return NULL;
+	}
 
 	return s;
 }

-- 
2.45.1

[PATCH RFC 4/4] mm, page_alloc: add static key for should_fail_alloc_page()

[Vlastimil Babka]

Similarly to should_failslab(), remove the overhead of calling the
noinline function should_fail_alloc_page() with a static key that guards
the allocation hotpath callsite and is controlled by the fault and error
injection frameworks.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/fail_page_alloc.c |  3 ++-
 mm/internal.h        |  2 ++
 mm/page_alloc.c      | 11 ++++++++---
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/mm/fail_page_alloc.c b/mm/fail_page_alloc.c
index b1b09cce9394..0906b76d78e8 100644
--- a/mm/fail_page_alloc.c
+++ b/mm/fail_page_alloc.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/fault-inject.h>
 #include <linux/mm.h>
+#include "internal.h"
 
 static struct {
 	struct fault_attr attr;
@@ -9,7 +10,7 @@ static struct {
 	bool ignore_gfp_reclaim;
 	u32 min_order;
 } fail_page_alloc = {
-	.attr = FAULT_ATTR_INITIALIZER,
+	.attr = FAULT_ATTR_INITIALIZER_KEY(&should_fail_alloc_page_active.key),
 	.ignore_gfp_reclaim = true,
 	.ignore_gfp_highmem = true,
 	.min_order = 1,
diff --git a/mm/internal.h b/mm/internal.h
index b2c75b12014e..8539e39b02e6 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -410,6 +410,8 @@ extern char * const zone_names[MAX_NR_ZONES];
 /* perform sanity checks on struct pages being allocated or freed */
 DECLARE_STATIC_KEY_MAYBE(CONFIG_DEBUG_VM, check_pages_enabled);
 
+DECLARE_STATIC_KEY_FALSE(should_fail_alloc_page_active);
+
 extern int min_free_kbytes;
 
 void setup_per_zone_wmarks(void);
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 2e22ce5675ca..e5dc3bafa549 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -274,6 +274,8 @@ int user_min_free_kbytes = -1;
 static int watermark_boost_factor __read_mostly = 15000;
 static int watermark_scale_factor = 10;
 
+DEFINE_STATIC_KEY_FALSE(should_fail_alloc_page_active);
+
 /* movable_zone is the "real" zone pages in ZONE_MOVABLE are taken from */
 int movable_zone;
 EXPORT_SYMBOL(movable_zone);
@@ -3012,7 +3014,7 @@ noinline bool should_fail_alloc_page(gfp_t gfp_mask, unsigned int order)
 {
 	return __should_fail_alloc_page(gfp_mask, order);
 }
-ALLOW_ERROR_INJECTION(should_fail_alloc_page, TRUE);
+ALLOW_ERROR_INJECTION_KEY(should_fail_alloc_page, TRUE, &should_fail_alloc_page_active);
 
 static inline long __zone_watermark_unusable_free(struct zone *z,
 				unsigned int order, unsigned int alloc_flags)
@@ -4430,8 +4432,11 @@ static inline bool prepare_alloc_pages(gfp_t gfp_mask, unsigned int order,
 
 	might_alloc(gfp_mask);
 
-	if (should_fail_alloc_page(gfp_mask, order))
-		return false;
+	if (static_branch_unlikely(&should_fail_alloc_page_active)) {
+		if (should_fail_alloc_page(gfp_mask, order)) {
+			return false;
+		}
+	}
 
 	*alloc_flags = gfp_to_alloc_flags_cma(gfp_mask, *alloc_flags);
 

-- 
2.45.1

Re: [PATCH RFC 0/4] static key support for error injection functions

[Mark Rutland]

Hi,

On Fri, May 31, 2024 at 11:33:31AM +0200, Vlastimil Babka wrote:
> Incomplete, help needed from ftrace/kprobe and bpf folks.

> - the generic error injection using kretprobes with
>   override_function_with_return is handled in patch 2. The
>   ALLOW_ERROR_INJECTION() annotation is extended so that static key
>   address can be passed, and the framework controls it when error
>   injection is enabled or disabled in debugfs for the function.
> 
> There are two more users I know of but am not familiar enough to fix up
> myself. I hope people that are more familiar can help me here.
> 
> - ftrace seems to be using override_function_with_return from
>   #define ftrace_override_function_with_return but I found no place
>   where the latter is used. I assume it might be hidden behind more
>   macro magic? But the point is if ftrace can be instructed to act like
>   an error injection, it would also have to use some form of metadata
>   (from patch 2 presumably?) to get to the static key and control it.

I don't think you've missed anything; nothing currently uses
ftrace_override_function_with_return(). I added that in commit:

  94d095ffa0e16bb7 ("ftrace: abstract DYNAMIC_FTRACE_WITH_ARGS accesses")

... so that it was possible to do anything that was possible with
FTRACE_WITH_REGS and/or kprobes, under the expectation that we might
want to move fault injection and BPF probes over to fprobes in future,
as ftrace/fprobes is generally faster than kprobes (e.g. for
architectures that can't do KPROBES_ON_FTRACE or OPTPROBES).

That's just the mechanism for the handler to use; I'd expect whatever
registered the handler to be responsible for flipping the static key,
and I don't think anything needs to change within ftrace itself.

>   If ftrace can only observe the function being called, maybe it
>   wouldn't be wrong to just observe nothing if the static key isn't
>   enabled because nobody is doing the fault injection?

Yep, that sounds right to me.

Mark.

Re: [PATCH RFC 3/4] mm, slab: add static key for should_failslab()

[Alexei Starovoitov]

On Fri, May 31, 2024 at 2:33 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> Since commit 4f6923fbb352 ("mm: make should_failslab always available for
> fault injection") should_failslab() is unconditionally a noinline
> function. This adds visible overhead to the slab allocation hotpath,
> even if the function is empty. With CONFIG_FAILSLAB=y there's additional
> overhead when the functionality is not enabled by a boot parameter or
> debugfs.
>
> The overhead can be eliminated with a static key around the callsite.
> Fault injection and error injection frameworks can now be told that the
> this function has a static key associated, and are able to enable and
> disable it accordingly.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  mm/failslab.c |  2 +-
>  mm/slab.h     |  3 +++
>  mm/slub.c     | 10 +++++++---
>  3 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/mm/failslab.c b/mm/failslab.c
> index ffc420c0e767..878fd08e5dac 100644
> --- a/mm/failslab.c
> +++ b/mm/failslab.c
> @@ -9,7 +9,7 @@ static struct {
>         bool ignore_gfp_reclaim;
>         bool cache_filter;
>  } failslab = {
> -       .attr = FAULT_ATTR_INITIALIZER,
> +       .attr = FAULT_ATTR_INITIALIZER_KEY(&should_failslab_active.key),
>         .ignore_gfp_reclaim = true,
>         .cache_filter = false,
>  };
> diff --git a/mm/slab.h b/mm/slab.h
> index 5f8f47c5bee0..792e19cb37b8 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -11,6 +11,7 @@
>  #include <linux/memcontrol.h>
>  #include <linux/kfence.h>
>  #include <linux/kasan.h>
> +#include <linux/jump_label.h>
>
>  /*
>   * Internal slab definitions
> @@ -160,6 +161,8 @@ static_assert(IS_ALIGNED(offsetof(struct slab, freelist), sizeof(freelist_aba_t)
>   */
>  #define slab_page(s) folio_page(slab_folio(s), 0)
>
> +DECLARE_STATIC_KEY_FALSE(should_failslab_active);
> +
>  /*
>   * If network-based swap is enabled, sl*b must keep track of whether pages
>   * were allocated from pfmemalloc reserves.
> diff --git a/mm/slub.c b/mm/slub.c
> index 0809760cf789..3bb579760a37 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3874,13 +3874,15 @@ static __always_inline void maybe_wipe_obj_freeptr(struct kmem_cache *s,
>                         0, sizeof(void *));
>  }
>
> +DEFINE_STATIC_KEY_FALSE(should_failslab_active);
> +
>  noinline int should_failslab(struct kmem_cache *s, gfp_t gfpflags)
>  {
>         if (__should_failslab(s, gfpflags))
>                 return -ENOMEM;
>         return 0;
>  }
> -ALLOW_ERROR_INJECTION(should_failslab, ERRNO);
> +ALLOW_ERROR_INJECTION_KEY(should_failslab, ERRNO, &should_failslab_active);
>
>  static __fastpath_inline
>  struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s, gfp_t flags)
> @@ -3889,8 +3891,10 @@ struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s, gfp_t flags)
>
>         might_alloc(flags);
>
> -       if (unlikely(should_failslab(s, flags)))
> -               return NULL;
> +       if (static_branch_unlikely(&should_failslab_active)) {
> +               if (should_failslab(s, flags))
> +                       return NULL;
> +       }

makes sense.
Acked-by: Alexei Starovoitov <ast@kernel.org>

Do you have any microbenchmark numbers before/after this optimization?

Re: [PATCH RFC 3/4] mm, slab: add static key for should_failslab()

[Yosry Ahmed]

On Fri, May 31, 2024 at 9:44 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, May 31, 2024 at 2:33 AM Vlastimil Babka <vbabka@suse.cz> wrote:
> >
> > Since commit 4f6923fbb352 ("mm: make should_failslab always available for
> > fault injection") should_failslab() is unconditionally a noinline
> > function. This adds visible overhead to the slab allocation hotpath,
> > even if the function is empty. With CONFIG_FAILSLAB=y there's additional
> > overhead when the functionality is not enabled by a boot parameter or
> > debugfs.
> >
> > The overhead can be eliminated with a static key around the callsite.
> > Fault injection and error injection frameworks can now be told that the
> > this function has a static key associated, and are able to enable and
> > disable it accordingly.
> >
> > Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> > ---
> >  mm/failslab.c |  2 +-
> >  mm/slab.h     |  3 +++
> >  mm/slub.c     | 10 +++++++---
> >  3 files changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/mm/failslab.c b/mm/failslab.c
> > index ffc420c0e767..878fd08e5dac 100644
> > --- a/mm/failslab.c
> > +++ b/mm/failslab.c
> > @@ -9,7 +9,7 @@ static struct {
> >         bool ignore_gfp_reclaim;
> >         bool cache_filter;
> >  } failslab = {
> > -       .attr = FAULT_ATTR_INITIALIZER,
> > +       .attr = FAULT_ATTR_INITIALIZER_KEY(&should_failslab_active.key),
> >         .ignore_gfp_reclaim = true,
> >         .cache_filter = false,
> >  };
> > diff --git a/mm/slab.h b/mm/slab.h
> > index 5f8f47c5bee0..792e19cb37b8 100644
> > --- a/mm/slab.h
> > +++ b/mm/slab.h
> > @@ -11,6 +11,7 @@
> >  #include <linux/memcontrol.h>
> >  #include <linux/kfence.h>
> >  #include <linux/kasan.h>
> > +#include <linux/jump_label.h>
> >
> >  /*
> >   * Internal slab definitions
> > @@ -160,6 +161,8 @@ static_assert(IS_ALIGNED(offsetof(struct slab, freelist), sizeof(freelist_aba_t)
> >   */
> >  #define slab_page(s) folio_page(slab_folio(s), 0)
> >
> > +DECLARE_STATIC_KEY_FALSE(should_failslab_active);
> > +
> >  /*
> >   * If network-based swap is enabled, sl*b must keep track of whether pages
> >   * were allocated from pfmemalloc reserves.
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 0809760cf789..3bb579760a37 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -3874,13 +3874,15 @@ static __always_inline void maybe_wipe_obj_freeptr(struct kmem_cache *s,
> >                         0, sizeof(void *));
> >  }
> >
> > +DEFINE_STATIC_KEY_FALSE(should_failslab_active);
> > +
> >  noinline int should_failslab(struct kmem_cache *s, gfp_t gfpflags)
> >  {
> >         if (__should_failslab(s, gfpflags))
> >                 return -ENOMEM;
> >         return 0;
> >  }
> > -ALLOW_ERROR_INJECTION(should_failslab, ERRNO);
> > +ALLOW_ERROR_INJECTION_KEY(should_failslab, ERRNO, &should_failslab_active);
> >
> >  static __fastpath_inline
> >  struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s, gfp_t flags)
> > @@ -3889,8 +3891,10 @@ struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s, gfp_t flags)
> >
> >         might_alloc(flags);
> >
> > -       if (unlikely(should_failslab(s, flags)))
> > -               return NULL;
> > +       if (static_branch_unlikely(&should_failslab_active)) {
> > +               if (should_failslab(s, flags))
> > +                       return NULL;
> > +       }
>
> makes sense.
> Acked-by: Alexei Starovoitov <ast@kernel.org>
>
> Do you have any microbenchmark numbers before/after this optimization?

There are numbers in the cover letter for the entire series:
https://lore.kernel.org/lkml/20240531-fault-injection-statickeys-v1-0-a513fd0a9614@suse.cz/

Re: [PATCH RFC 0/4] static key support for error injection functions

[Roman Gushchin]

On Fri, May 31, 2024 at 11:33:31AM +0200, Vlastimil Babka wrote:
> Incomplete, help needed from ftrace/kprobe and bpf folks.
> 
> As previously mentioned by myself [1] and others [2] the functions
> designed for error injection can bring visible overhead in fastpaths
> such as slab or page allocation, because even if nothing hooks into them
> at a given moment, they are noninline function calls regardless of
> CONFIG_ options since commits 4f6923fbb352 ("mm: make should_failslab
> always available for fault injection") and af3b854492f3
> ("mm/page_alloc.c: allow error injection").
> 
> Live patching their callsites has been also suggested in both [1] and
> [2] threads, and this is an attempt to do that with static keys that
> guard the call sites. When disabled, the error injection functions still
> exist and are noinline, but are not being called. Any of the existing
> mechanisms that can inject errors should make sure to enable the
> respective static key. I have added that support to some of them but
> need help with the others.

I think it's a clever idea and makes total sense!

> 
> - the legacy fault injection, i.e. CONFIG_FAILSLAB and
>   CONFIG_FAIL_PAGE_ALLOC is handled in Patch 1, and can be passed the
>   address of the static key if it exists. The key will be activated if the
>   fault injection probability becomes non-zero, and deactivated in the
>   opposite transition. This also removes the overhead of the evaluation
>   (on top of the noninline function call) when these mechanisms are
>   configured in the kernel but unused at the moment.
> 
> - the generic error injection using kretprobes with
>   override_function_with_return is handled in patch 2. The
>   ALLOW_ERROR_INJECTION() annotation is extended so that static key
>   address can be passed, and the framework controls it when error
>   injection is enabled or disabled in debugfs for the function.
> 
> There are two more users I know of but am not familiar enough to fix up
> myself. I hope people that are more familiar can help me here.
> 
> - ftrace seems to be using override_function_with_return from
>   #define ftrace_override_function_with_return but I found no place
>   where the latter is used. I assume it might be hidden behind more
>   macro magic? But the point is if ftrace can be instructed to act like
>   an error injection, it would also have to use some form of metadata
>   (from patch 2 presumably?) to get to the static key and control it.
> 
>   If ftrace can only observe the function being called, maybe it
>   wouldn't be wrong to just observe nothing if the static key isn't
>   enabled because nobody is doing the fault injection?
> 
> - bpftrace, as can be seen from the example in commit 4f6923fbb352
>   description. I suppose bpf is already aware what functions the
>   currently loaded bpf programs hook into, so that it could look up the
>   static key and control it. Maybe using again the metadata from patch 2,
>   or extending its own, as I've noticed there's e.g. BTF_ID(func,
>   should_failslab)
> 
> Now I realize maybe handling this at the k(ret)probe level would be
> sufficient for all cases except the legacy fault injection from Patch 1?
> Also wanted to note that by AFAIU by using the static_key_slow_dec/inc
> API (as done in patches 1/2) should allow all mechanisms to coexist
> naturally without fighting each other on the static key state, and also
> handle the reference count for e.g. active probes or bpf programs if
> there's no similar internal mechanism.
> 
> Patches 3 and 4 implement the static keys for the two mm fault injection
> sites in slab and page allocators. For a quick demonstration I've run a
> VM and the simple test from [1] that stresses the slab allocator and got
> this time before the series:
> 
> real    0m8.349s
> user    0m0.694s
> sys     0m7.648s
> 
> with perf showing
> 
>    0.61%  nonexistent  [kernel.kallsyms]  [k] should_failslab.constprop.0
>    0.00%  nonexistent  [kernel.kallsyms]  [k] should_fail_alloc_page                                                                                                                                                                                        ▒
> 
> And after the series
> 
> real    0m7.924s
> user    0m0.727s
> sys     0m7.191s

Is "user" increase a measurement error or it's real?

Otherwise, nice savings!

Re: [PATCH RFC 3/4] mm, slab: add static key for should_failslab()

[Roman Gushchin]

On Fri, May 31, 2024 at 11:33:34AM +0200, Vlastimil Babka wrote:
> Since commit 4f6923fbb352 ("mm: make should_failslab always available for
> fault injection") should_failslab() is unconditionally a noinline
> function. This adds visible overhead to the slab allocation hotpath,
> even if the function is empty. With CONFIG_FAILSLAB=y there's additional
> overhead when the functionality is not enabled by a boot parameter or
> debugfs.
> 
> The overhead can be eliminated with a static key around the callsite.
> Fault injection and error injection frameworks can now be told that the
> this function has a static key associated, and are able to enable and
> disable it accordingly.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev>

Re: [PATCH RFC 4/4] mm, page_alloc: add static key for should_fail_alloc_page()

[Roman Gushchin]

On Fri, May 31, 2024 at 11:33:35AM +0200, Vlastimil Babka wrote:
> Similarly to should_failslab(), remove the overhead of calling the
> noinline function should_fail_alloc_page() with a static key that guards
> the allocation hotpath callsite and is controlled by the fault and error
> injection frameworks.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev>

Thanks!

Re: [PATCH RFC 0/4] static key support for error injection functions

[Vlastimil Babka]

On 5/31/24 5:31 PM, Mark Rutland wrote:
> Hi,
> 
> On Fri, May 31, 2024 at 11:33:31AM +0200, Vlastimil Babka wrote:
>> Incomplete, help needed from ftrace/kprobe and bpf folks.
> 
>> - the generic error injection using kretprobes with
>>   override_function_with_return is handled in patch 2. The
>>   ALLOW_ERROR_INJECTION() annotation is extended so that static key
>>   address can be passed, and the framework controls it when error
>>   injection is enabled or disabled in debugfs for the function.
>> 
>> There are two more users I know of but am not familiar enough to fix up
>> myself. I hope people that are more familiar can help me here.
>> 
>> - ftrace seems to be using override_function_with_return from
>>   #define ftrace_override_function_with_return but I found no place
>>   where the latter is used. I assume it might be hidden behind more
>>   macro magic? But the point is if ftrace can be instructed to act like
>>   an error injection, it would also have to use some form of metadata
>>   (from patch 2 presumably?) to get to the static key and control it.
> 
> I don't think you've missed anything; nothing currently uses
> ftrace_override_function_with_return(). I added that in commit:

Ah, great, thanks for confirming that.

>   94d095ffa0e16bb7 ("ftrace: abstract DYNAMIC_FTRACE_WITH_ARGS accesses")
> 
> ... so that it was possible to do anything that was possible with
> FTRACE_WITH_REGS and/or kprobes, under the expectation that we might
> want to move fault injection and BPF probes over to fprobes in future,
> as ftrace/fprobes is generally faster than kprobes (e.g. for
> architectures that can't do KPROBES_ON_FTRACE or OPTPROBES).
> 
> That's just the mechanism for the handler to use; I'd expect whatever
> registered the handler to be responsible for flipping the static key,
> and I don't think anything needs to change within ftrace itself.
> 
>>   If ftrace can only observe the function being called, maybe it
>>   wouldn't be wrong to just observe nothing if the static key isn't
>>   enabled because nobody is doing the fault injection?
> 
> Yep, that sounds right to me.

Good.

> Mark.

Re: [PATCH RFC 0/4] static key support for error injection functions

[Vlastimil Babka]

On 6/1/24 1:39 AM, Roman Gushchin wrote:
> On Fri, May 31, 2024 at 11:33:31AM +0200, Vlastimil Babka wrote:
>> Incomplete, help needed from ftrace/kprobe and bpf folks.
>> 
>> As previously mentioned by myself [1] and others [2] the functions
>> designed for error injection can bring visible overhead in fastpaths
>> such as slab or page allocation, because even if nothing hooks into them
>> at a given moment, they are noninline function calls regardless of
>> CONFIG_ options since commits 4f6923fbb352 ("mm: make should_failslab
>> always available for fault injection") and af3b854492f3
>> ("mm/page_alloc.c: allow error injection").
>> 
>> Live patching their callsites has been also suggested in both [1] and
>> [2] threads, and this is an attempt to do that with static keys that
>> guard the call sites. When disabled, the error injection functions still
>> exist and are noinline, but are not being called. Any of the existing
>> mechanisms that can inject errors should make sure to enable the
>> respective static key. I have added that support to some of them but
>> need help with the others.
> 
> I think it's a clever idea and makes total sense!

Thanks!

>> 
>> Patches 3 and 4 implement the static keys for the two mm fault injection
>> sites in slab and page allocators. For a quick demonstration I've run a
>> VM and the simple test from [1] that stresses the slab allocator and got
>> this time before the series:
>> 
>> real    0m8.349s
>> user    0m0.694s
>> sys     0m7.648s
>> 
>> with perf showing
>> 
>>    0.61%  nonexistent  [kernel.kallsyms]  [k] should_failslab.constprop.0
>>    0.00%  nonexistent  [kernel.kallsyms]  [k] should_fail_alloc_page                                                                                                                                                                                        ▒
>> 
>> And after the series
>> 
>> real    0m7.924s
>> user    0m0.727s
>> sys     0m7.191s
> 
> Is "user" increase a measurement error or it's real?

Hm interesting, I have actually did the measurement 3 times even though I
pasted just one, and it's consistent. But could be just artifact of where
things landed in the cache, and might change a bit with every kernel
build/boot. Will see. There's no reason why this should affect user time.

> Otherwise, nice savings!

Re: [PATCH RFC 3/4] mm, slab: add static key for should_failslab()

[Vlastimil Babka]

On 5/31/24 6:43 PM, Alexei Starovoitov wrote:
> On Fri, May 31, 2024 at 2:33 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>>         might_alloc(flags);
>>
>> -       if (unlikely(should_failslab(s, flags)))
>> -               return NULL;
>> +       if (static_branch_unlikely(&should_failslab_active)) {
>> +               if (should_failslab(s, flags))
>> +                       return NULL;
>> +       }
> 
> makes sense.
> Acked-by: Alexei Starovoitov <ast@kernel.org>

Thanks :) but please note the cover letter where I explain how I need help
with the bpftrace side (and ftrace, but that seems sorted). Without that
part, bpftrace will silently stop doing the injection as the static key will
remain disabled.

> Do you have any microbenchmark numbers before/after this optimization?

Also in cover letter, but will include it in the patch commit log next time.

Re: [PATCH RFC 0/4] static key support for error injection functions

[Wei Yang]

On Fri, May 31, 2024 at 11:33:31AM +0200, Vlastimil Babka wrote:
>Incomplete, help needed from ftrace/kprobe and bpf folks.
>
>As previously mentioned by myself [1] and others [2] the functions
>designed for error injection can bring visible overhead in fastpaths
>such as slab or page allocation, because even if nothing hooks into them
>at a given moment, they are noninline function calls regardless of
>CONFIG_ options since commits 4f6923fbb352 ("mm: make should_failslab
>always available for fault injection") and af3b854492f3
>("mm/page_alloc.c: allow error injection").
>
>Live patching their callsites has been also suggested in both [1] and
>[2] threads, and this is an attempt to do that with static keys that
>guard the call sites. When disabled, the error injection functions still
>exist and are noinline, but are not being called. Any of the existing
>mechanisms that can inject errors should make sure to enable the
>respective static key. I have added that support to some of them but
>need help with the others.
>
>- the legacy fault injection, i.e. CONFIG_FAILSLAB and
>  CONFIG_FAIL_PAGE_ALLOC is handled in Patch 1, and can be passed the
>  address of the static key if it exists. The key will be activated if the
>  fault injection probability becomes non-zero, and deactivated in the
>  opposite transition. This also removes the overhead of the evaluation
>  (on top of the noninline function call) when these mechanisms are
>  configured in the kernel but unused at the moment.
>
>- the generic error injection using kretprobes with
>  override_function_with_return is handled in patch 2. The
>  ALLOW_ERROR_INJECTION() annotation is extended so that static key
>  address can be passed, and the framework controls it when error
>  injection is enabled or disabled in debugfs for the function.
>
>There are two more users I know of but am not familiar enough to fix up
>myself. I hope people that are more familiar can help me here.
>
>- ftrace seems to be using override_function_with_return from
>  #define ftrace_override_function_with_return but I found no place
>  where the latter is used. I assume it might be hidden behind more
>  macro magic? But the point is if ftrace can be instructed to act like
>  an error injection, it would also have to use some form of metadata
>  (from patch 2 presumably?) to get to the static key and control it.
>
>  If ftrace can only observe the function being called, maybe it
>  wouldn't be wrong to just observe nothing if the static key isn't
>  enabled because nobody is doing the fault injection?
>
>- bpftrace, as can be seen from the example in commit 4f6923fbb352
>  description. I suppose bpf is already aware what functions the
>  currently loaded bpf programs hook into, so that it could look up the
>  static key and control it. Maybe using again the metadata from patch 2,
>  or extending its own, as I've noticed there's e.g. BTF_ID(func,
>  should_failslab)
>
>Now I realize maybe handling this at the k(ret)probe level would be
>sufficient for all cases except the legacy fault injection from Patch 1?
>Also wanted to note that by AFAIU by using the static_key_slow_dec/inc
>API (as done in patches 1/2) should allow all mechanisms to coexist
>naturally without fighting each other on the static key state, and also
>handle the reference count for e.g. active probes or bpf programs if
>there's no similar internal mechanism.
>
>Patches 3 and 4 implement the static keys for the two mm fault injection
>sites in slab and page allocators. For a quick demonstration I've run a
>VM and the simple test from [1] that stresses the slab allocator and got

I took a look into [1] and I see some data like "1.43% plus the overhead in
its caller", but not clearly find which test cases are.

Sorry for my unfamiliarity, would you mind giving more words on the cases?

>this time before the series:
>
>real    0m8.349s
>user    0m0.694s
>sys     0m7.648s
>
>with perf showing
>
>   0.61%  nonexistent  [kernel.kallsyms]  [k] should_failslab.constprop.0
>   0.00%  nonexistent  [kernel.kallsyms]  [k] should_fail_alloc_page                                                                                                                                                                                        ▒
>
>And after the series
>
>real    0m7.924s
>user    0m0.727s
>sys     0m7.191s
>

Maybe add the percentage here would be more helpful.

>and the functions gone from perf report.
>
>There might be other such fault injection callsites in hotpaths of other
>subsystems but I didn't search for them at this point.
>
>[1] https://lore.kernel.org/all/6d5bb852-8703-4abf-a52b-90816bccbd7f@suse.cz/
>[2] https://lore.kernel.org/all/3j5d3p22ssv7xoaghzraa7crcfih3h2qqjlhmjppbp6f42pg2t@kg7qoicog5ye/
>
>Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>---
>Vlastimil Babka (4):
>      fault-inject: add support for static keys around fault injection sites
>      error-injection: support static keys around injectable functions
>      mm, slab: add static key for should_failslab()
>      mm, page_alloc: add static key for should_fail_alloc_page()
>
> include/asm-generic/error-injection.h | 13 ++++++++++-
> include/asm-generic/vmlinux.lds.h     |  2 +-
> include/linux/error-injection.h       |  9 +++++---
> include/linux/fault-inject.h          |  7 +++++-
> kernel/fail_function.c                | 22 +++++++++++++++---
> lib/error-inject.c                    |  6 ++++-
> lib/fault-inject.c                    | 43 ++++++++++++++++++++++++++++++++++-
> mm/fail_page_alloc.c                  |  3 ++-
> mm/failslab.c                         |  2 +-
> mm/internal.h                         |  2 ++
> mm/page_alloc.c                       | 11 ++++++---
> mm/slab.h                             |  3 +++
> mm/slub.c                             | 10 +++++---
> 13 files changed, 114 insertions(+), 19 deletions(-)
>---
>base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
>change-id: 20240530-fault-injection-statickeys-66b7222e91b7
>
>Best regards,
>-- 
>Vlastimil Babka <vbabka@suse.cz>
>

-- 
Wei Yang
Help you, Help me

Re: [PATCH RFC 2/4] error-injection: support static keys around injectable functions

[Masami Hiramatsu]

On Fri, 31 May 2024 11:33:33 +0200
Vlastimil Babka <vbabka@suse.cz> wrote:

> Error injectable functions cannot be inlined and since some are called
> from hot paths, this incurrs overhead even if no error injection is
> enabled for them.
> 
> To remove this overhead when disabled, allow the callsites of error
> injectable functions to put the calls behind a static key, which the
> framework can control when error injection is enabled or disabled for
> the function.
> 
> Introduce a new ALLOW_ERROR_INJECTION_KEY() macro that adds a parameter
> with the static key's address, and store it in struct
> error_injection_entry. This new field has caused a mismatch when
> populating the injection list from the _error_injection_whitelist
> section with the current STRUCT_ALIGN(), so change the alignment to 8.
> 
> During the population, copy the key's address also to struct ei_entry,
> and make it possible to retrieve it along with the error type by
> get_injectable_error_type().
> 
> Finally, make the processing of writes to the debugfs inject file enable
> the static key when the function is added to the injection list, and
> disable when removed.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  include/asm-generic/error-injection.h | 13 ++++++++++++-
>  include/asm-generic/vmlinux.lds.h     |  2 +-
>  include/linux/error-injection.h       |  9 ++++++---
>  kernel/fail_function.c                | 22 +++++++++++++++++++---
>  lib/error-inject.c                    |  6 +++++-
>  5 files changed, 43 insertions(+), 9 deletions(-)
> 
> diff --git a/include/asm-generic/error-injection.h b/include/asm-generic/error-injection.h
> index b05253f68eaa..eed2731f3820 100644
> --- a/include/asm-generic/error-injection.h
> +++ b/include/asm-generic/error-injection.h
> @@ -12,6 +12,7 @@ enum {
>  
>  struct error_injection_entry {
>  	unsigned long	addr;
> +	unsigned long	static_key_addr;
>  	int		etype;
>  };
>  
> @@ -25,16 +26,26 @@ struct pt_regs;
>   * 'Error Injectable Functions' section.
>   */
>  #define ALLOW_ERROR_INJECTION(fname, _etype)				\
> -static struct error_injection_entry __used				\
> +static struct error_injection_entry __used __aligned(8)			\
>  	__section("_error_injection_whitelist")				\
>  	_eil_addr_##fname = {						\
>  		.addr = (unsigned long)fname,				\
>  		.etype = EI_ETYPE_##_etype,				\
>  	}
>  
> +#define ALLOW_ERROR_INJECTION_KEY(fname, _etype, key)			\
> +static struct error_injection_entry __used __aligned(8)			\
> +	__section("_error_injection_whitelist")				\
> +	_eil_addr_##fname = {						\
> +		.addr = (unsigned long)fname,				\
> +		.static_key_addr = (unsigned long)key,			\
> +		.etype = EI_ETYPE_##_etype,				\
> +	}
> +
>  void override_function_with_return(struct pt_regs *regs);
>  #else
>  #define ALLOW_ERROR_INJECTION(fname, _etype)
> +#define ALLOW_ERROR_INJECTION_KEY(fname, _etype, key)
>  
>  static inline void override_function_with_return(struct pt_regs *regs) { }
>  #endif
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 5703526d6ebf..1b15a0af2a00 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -248,7 +248,7 @@
>  
>  #ifdef CONFIG_FUNCTION_ERROR_INJECTION
>  #define ERROR_INJECT_WHITELIST()			\
> -	STRUCT_ALIGN();					\
> +	. = ALIGN(8);					\
>  	BOUNDED_SECTION(_error_injection_whitelist)
>  #else
>  #define ERROR_INJECT_WHITELIST()
> diff --git a/include/linux/error-injection.h b/include/linux/error-injection.h
> index 20e738f4eae8..bec81b57a9d5 100644
> --- a/include/linux/error-injection.h
> +++ b/include/linux/error-injection.h
> @@ -6,10 +6,12 @@
>  #include <linux/errno.h>
>  #include <asm-generic/error-injection.h>
>  
> +struct static_key;
> +
>  #ifdef CONFIG_FUNCTION_ERROR_INJECTION
>  
> -extern bool within_error_injection_list(unsigned long addr);
> -extern int get_injectable_error_type(unsigned long addr);
> +bool within_error_injection_list(unsigned long addr);
> +int get_injectable_error_type(unsigned long addr, struct static_key **key_addr);

This seems like an add-hoc change. Since this is called in a cold path
(only used when adding new function), can you add new 
`struct static_key *get_injection_key(unsigned long addr)`
to find the static_key from the address?

Other part looks good to me.

Thank you,



-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH RFC 3/4] mm, slab: add static key for should_failslab()

[Alexei Starovoitov]

On Sat, Jun 1, 2024 at 1:57 PM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 5/31/24 6:43 PM, Alexei Starovoitov wrote:
> > On Fri, May 31, 2024 at 2:33 AM Vlastimil Babka <vbabka@suse.cz> wrote:
> >>         might_alloc(flags);
> >>
> >> -       if (unlikely(should_failslab(s, flags)))
> >> -               return NULL;
> >> +       if (static_branch_unlikely(&should_failslab_active)) {
> >> +               if (should_failslab(s, flags))
> >> +                       return NULL;
> >> +       }
> >
> > makes sense.
> > Acked-by: Alexei Starovoitov <ast@kernel.org>
>
> Thanks :) but please note the cover letter where I explain how I need help
> with the bpftrace side (and ftrace, but that seems sorted). Without that
> part, bpftrace will silently stop doing the injection as the static key will
> remain disabled.

Right. That part was clear. Once this set lands we can add
static key on/off logic either in the kernel directly, or in libbpf.
In the kernel is certainly cleaner.
How will ftrace handle it? I couldn't figure it out from this set.
Ideally key toggle should be a part of generic kprobe attach logic
and not bpf specific, then both bpf and kprobe will work.

Re: [PATCH RFC 0/4] static key support for error injection functions

[David Rientjes]

[-- Attachment #1: Type: text/plain, Size: 3059 bytes --]

On Fri, 31 May 2024, Vlastimil Babka wrote:

> Patches 3 and 4 implement the static keys for the two mm fault injection
> sites in slab and page allocators. For a quick demonstration I've run a
> VM and the simple test from [1] that stresses the slab allocator and got
> this time before the series:
> 
> real    0m8.349s
> user    0m0.694s
> sys     0m7.648s
> 
> with perf showing
> 
>    0.61%  nonexistent  [kernel.kallsyms]  [k] should_failslab.constprop.0
>    0.00%  nonexistent  [kernel.kallsyms]  [k] should_fail_alloc_page                                                                                                                                                                                        ▒
> 
> And after the series
> 
> real    0m7.924s
> user    0m0.727s
> sys     0m7.191s
> 
> and the functions gone from perf report.
> 

Impressive results that will no doubt be a win for kernels that enable 
these options.

Both CONFIG_FAILSLAB and CONFIG_FAIL_PAGE_ALLOC go out of their way to 
have no overhead, both in performance and kernel text overhead, when the 
.config options are disabled.

Do we have any insight into the distros or users that enable either of 
these options and are expecting optimal performance?  I would have assumed 
that while CONFIG_FAULT_INJECTION may be enabled that any users who would 
care deeply about this would have disabled both of these debug options.

> There might be other such fault injection callsites in hotpaths of other
> subsystems but I didn't search for them at this point.
> 
> [1] https://lore.kernel.org/all/6d5bb852-8703-4abf-a52b-90816bccbd7f@suse.cz/
> [2] https://lore.kernel.org/all/3j5d3p22ssv7xoaghzraa7crcfih3h2qqjlhmjppbp6f42pg2t@kg7qoicog5ye/
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
> Vlastimil Babka (4):
>       fault-inject: add support for static keys around fault injection sites
>       error-injection: support static keys around injectable functions
>       mm, slab: add static key for should_failslab()
>       mm, page_alloc: add static key for should_fail_alloc_page()
> 
>  include/asm-generic/error-injection.h | 13 ++++++++++-
>  include/asm-generic/vmlinux.lds.h     |  2 +-
>  include/linux/error-injection.h       |  9 +++++---
>  include/linux/fault-inject.h          |  7 +++++-
>  kernel/fail_function.c                | 22 +++++++++++++++---
>  lib/error-inject.c                    |  6 ++++-
>  lib/fault-inject.c                    | 43 ++++++++++++++++++++++++++++++++++-
>  mm/fail_page_alloc.c                  |  3 ++-
>  mm/failslab.c                         |  2 +-
>  mm/internal.h                         |  2 ++
>  mm/page_alloc.c                       | 11 ++++++---
>  mm/slab.h                             |  3 +++
>  mm/slub.c                             | 10 +++++---
>  13 files changed, 114 insertions(+), 19 deletions(-)
> ---
> base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
> change-id: 20240530-fault-injection-statickeys-66b7222e91b7
> 
> Best regards,
> -- 
> Vlastimil Babka <vbabka@suse.cz>
> 
> 

Re: [PATCH RFC 0/4] static key support for error injection functions

[Vlastimil Babka]

On 6/2/24 10:47 PM, David Rientjes wrote:
> On Fri, 31 May 2024, Vlastimil Babka wrote:
> 
>> Patches 3 and 4 implement the static keys for the two mm fault injection
>> sites in slab and page allocators. For a quick demonstration I've run a
>> VM and the simple test from [1] that stresses the slab allocator and got
>> this time before the series:
>> 
>> real    0m8.349s
>> user    0m0.694s
>> sys     0m7.648s
>> 
>> with perf showing
>> 
>>    0.61%  nonexistent  [kernel.kallsyms]  [k] should_failslab.constprop.0
>>    0.00%  nonexistent  [kernel.kallsyms]  [k] should_fail_alloc_page                                                                                                                                                                                        ▒
>> 
>> And after the series
>> 
>> real    0m7.924s
>> user    0m0.727s
>> sys     0m7.191s
>> 
>> and the functions gone from perf report.
>> 
> 
> Impressive results that will no doubt be a win for kernels that enable 
> these options.

Oh, I should have been more clear about it. This was done with both of these
options disabled. It's measuring just removing the overhead of calling the
empty noninline function, otherwise the difference would be larger. CPU
mitigations (defaults) were enabled though, which affects the cost of
function calls (this was in KVM guest on Ryzen 2700).

> Both CONFIG_FAILSLAB and CONFIG_FAIL_PAGE_ALLOC go out of their way to 
> have no overhead, both in performance and kernel text overhead, when the 
> .config options are disabled.

Except the unavoidable function call overhead since commits 4f6923fbb352 and
af3b854492f3.

> Do we have any insight into the distros or users that enable either of 
> these options and are expecting optimal performance?  I would have assumed 
> that while CONFIG_FAULT_INJECTION may be enabled that any users who would 
> care deeply about this would have disabled both of these debug options.

Eliminating the empty function call overhead, which is currently not
possible to configure out in any way, was my primary goal. For our distro we
disable the options and they are enabled only in a debug kernel option. So
the additional benefit of the static key is we could enable them with no
cost, and have them available for when needed, without the need to change
kernel. This is great for debugging functionality in general
(debug_pagealloc, page_owner), maybe this would be less likely to be useful,
but one never knows.

>> There might be other such fault injection callsites in hotpaths of other
>> subsystems but I didn't search for them at this point.
>> 
>> [1] https://lore.kernel.org/all/6d5bb852-8703-4abf-a52b-90816bccbd7f@suse.cz/
>> [2] https://lore.kernel.org/all/3j5d3p22ssv7xoaghzraa7crcfih3h2qqjlhmjppbp6f42pg2t@kg7qoicog5ye/
>> 
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>> ---
>> Vlastimil Babka (4):
>>       fault-inject: add support for static keys around fault injection sites
>>       error-injection: support static keys around injectable functions
>>       mm, slab: add static key for should_failslab()
>>       mm, page_alloc: add static key for should_fail_alloc_page()
>> 
>>  include/asm-generic/error-injection.h | 13 ++++++++++-
>>  include/asm-generic/vmlinux.lds.h     |  2 +-
>>  include/linux/error-injection.h       |  9 +++++---
>>  include/linux/fault-inject.h          |  7 +++++-
>>  kernel/fail_function.c                | 22 +++++++++++++++---
>>  lib/error-inject.c                    |  6 ++++-
>>  lib/fault-inject.c                    | 43 ++++++++++++++++++++++++++++++++++-
>>  mm/fail_page_alloc.c                  |  3 ++-
>>  mm/failslab.c                         |  2 +-
>>  mm/internal.h                         |  2 ++
>>  mm/page_alloc.c                       | 11 ++++++---
>>  mm/slab.h                             |  3 +++
>>  mm/slub.c                             | 10 +++++---
>>  13 files changed, 114 insertions(+), 19 deletions(-)
>> ---
>> base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
>> change-id: 20240530-fault-injection-statickeys-66b7222e91b7
>> 
>> Best regards,
>> -- 
>> Vlastimil Babka <vbabka@suse.cz>
>> 
>>