[v2,1/7] fault-inject: add support for static keys around fault injection sites

Commit Message

Vlastimil Babka June 19, 2024, 10:48 p.m. UTC

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 not for any of the
configfs based fault injection users.

Signed-off-by: Vlastimil Babka <[e-mail]>
---
 include/linux/fault-inject.h |  7 ++++++-
 lib/fault-inject.c           | 43 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 2 deletions(-)

Comments

Steven Rostedt June 25, 2024, 2:08 p.m. UTC | #1

On Thu, 20 Jun 2024 00:48:55 +0200
Vlastimil Babka <[e-mail]> wrote:

> +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);
> +		}
> +	}

So basically the above is testing if val to probability is going from
zero or non-zero. For such cases, I find it less confusing to have:

	if (attr->active) {
		if (!!attr->probability != !!val) {
			if (val)
				static_key_slow_inc(attr->active);
			else
				static_key_slow_dec(attr->active);
		}
	}

This does add a layer of nested ifs, but IMO it's a bit more clear in
what is happening, and it gets rid of the "else if".

Not saying you need to change it. This is more of an FYI.

-- Steve


> +
> +	attr->probability = val;
> +
> +	mutex_unlock(&probability_mutex);
> +
> +	return 0;
> +}

Patch

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..de9552cb22d0 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);