* [PATCH v6 1/5] tracing: Introduce faultable tracepoints
2024-08-28 14:41 [PATCH v6 0/5] Faultable Tracepoints Mathieu Desnoyers
@ 2024-08-28 14:41 ` Mathieu Desnoyers
2024-08-28 14:41 ` [PATCH v6 2/5] tracing/ftrace: Add support for " Mathieu Desnoyers
` (3 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Mathieu Desnoyers @ 2024-08-28 14:41 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu
Cc: linux-kernel, Mathieu Desnoyers, Peter Zijlstra,
Alexei Starovoitov, Yonghong Song, Paul E . McKenney,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Namhyung Kim, bpf, Joel Fernandes,
linux-trace-kernel, Michael Jeanson
When invoked from system call enter/exit instrumentation, accessing
user-space data is a common use-case for tracers. However, tracepoints
currently disable preemption around iteration on the registered
tracepoint probes and invocation of the probe callbacks, which prevents
tracers from handling page faults.
Extend the tracepoint and trace event APIs to allow defining a faultable
tracepoint which invokes its callback with preemption enabled.
Also extend the tracepoint API to allow tracers to request specific
probes to be connected to those faultable tracepoints. When the
TRACEPOINT_MAY_FAULT flag is provided on registration, the probe
callback will be called with preemption enabled, and is allowed to take
page faults. Faultable probes can only be registered on faultable
tracepoints and non-faultable probes on non-faultable tracepoints.
The tasks trace rcu mechanism is used to synchronize read-side
marshalling of the registered probes with respect to faultable probes
unregistration and teardown.
Link: https://lore.kernel.org/lkml/20231002202531.3160-1-mathieu.desnoyers@efficios.com/
Co-developed-by: Michael Jeanson <mjeanson@efficios.com>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Michael Jeanson <mjeanson@efficios.com>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Yonghong Song <yhs@fb.com>
Cc: Paul E. McKenney <paulmck@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: bpf@vger.kernel.org
Cc: Joel Fernandes <joel@joelfernandes.org>
---
Changes since v1:
- Cleanup __DO_TRACE() implementation.
- Rename "sleepable tracepoints" to "faultable tracepoints", MAYSLEEP to
MAYFAULT, and use might_fault() rather than might_sleep(), to properly
convey that the tracepoints are meant to be able to take a page fault,
which requires to be able to sleep *and* to hold the mmap_sem.
Changes since v2:
- Rename MAYFAULT to MAY_FAULT.
- Rebased on 6.5.5.
- Introduce MAY_EXIST tracepoint flag.
Changes since v3:
- Rebased on 6.6.2.
Changes since v4:
- Rebased on 6.9.6.
- Simplify flag check in tracepoint_probe_register_prio_flags().
- Update MAY_EXIST flag description.
Changes since v5:
- Rebased on v6.11-rc5.
---
include/linux/tracepoint-defs.h | 14 ++++++
include/linux/tracepoint.h | 88 +++++++++++++++++++++++----------
include/trace/define_trace.h | 7 +++
include/trace/trace_events.h | 6 +++
init/Kconfig | 1 +
kernel/trace/bpf_trace.c | 4 +-
kernel/trace/trace_fprobe.c | 5 +-
kernel/tracepoint.c | 65 ++++++++++++++----------
8 files changed, 136 insertions(+), 54 deletions(-)
diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
index 4dc4955f0fbf..94e39c86b49f 100644
--- a/include/linux/tracepoint-defs.h
+++ b/include/linux/tracepoint-defs.h
@@ -29,6 +29,19 @@ struct tracepoint_func {
int prio;
};
+/**
+ * enum tracepoint_flags - Tracepoint flags
+ * @TRACEPOINT_MAY_EXIST: On registration, don't warn if the tracepoint is
+ * already registered.
+ * @TRACEPOINT_MAY_FAULT: The tracepoint probe callback will be called with
+ * preemption enabled, and is allowed to take page
+ * faults.
+ */
+enum tracepoint_flags {
+ TRACEPOINT_MAY_EXIST = (1 << 0),
+ TRACEPOINT_MAY_FAULT = (1 << 1),
+};
+
struct tracepoint {
const char *name; /* Tracepoint name */
struct static_key key;
@@ -39,6 +52,7 @@ struct tracepoint {
int (*regfunc)(void);
void (*unregfunc)(void);
struct tracepoint_func __rcu *funcs;
+ unsigned int flags;
};
#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 6be396bb4297..7ae5496a800c 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -17,6 +17,7 @@
#include <linux/errno.h>
#include <linux/types.h>
#include <linux/rcupdate.h>
+#include <linux/rcupdate_trace.h>
#include <linux/tracepoint-defs.h>
#include <linux/static_call.h>
@@ -40,17 +41,10 @@ extern int
tracepoint_probe_register_prio(struct tracepoint *tp, void *probe, void *data,
int prio);
extern int
-tracepoint_probe_register_prio_may_exist(struct tracepoint *tp, void *probe, void *data,
- int prio);
+tracepoint_probe_register_prio_flags(struct tracepoint *tp, void *probe, void *data,
+ int prio, unsigned int flags);
extern int
tracepoint_probe_unregister(struct tracepoint *tp, void *probe, void *data);
-static inline int
-tracepoint_probe_register_may_exist(struct tracepoint *tp, void *probe,
- void *data)
-{
- return tracepoint_probe_register_prio_may_exist(tp, probe, data,
- TRACEPOINT_DEFAULT_PRIO);
-}
extern void
for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv),
void *priv);
@@ -89,6 +83,7 @@ int unregister_tracepoint_module_notifier(struct notifier_block *nb)
#ifdef CONFIG_TRACEPOINTS
static inline void tracepoint_synchronize_unregister(void)
{
+ synchronize_rcu_tasks_trace();
synchronize_srcu(&tracepoint_srcu);
synchronize_rcu();
}
@@ -191,9 +186,10 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
* it_func[0] is never NULL because there is at least one element in the array
* when the array itself is non NULL.
*/
-#define __DO_TRACE(name, args, cond, rcuidle) \
+#define __DO_TRACE(name, args, cond, rcuidle, tp_flags) \
do { \
int __maybe_unused __idx = 0; \
+ bool mayfault = (tp_flags) & TRACEPOINT_MAY_FAULT; \
\
if (!(cond)) \
return; \
@@ -202,8 +198,12 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
"Bad RCU usage for tracepoint")) \
return; \
\
- /* keep srcu and sched-rcu usage consistent */ \
- preempt_disable_notrace(); \
+ if (mayfault) { \
+ rcu_read_lock_trace(); \
+ } else { \
+ /* keep srcu and sched-rcu usage consistent */ \
+ preempt_disable_notrace(); \
+ } \
\
/* \
* For rcuidle callers, use srcu since sched-rcu \
@@ -221,20 +221,23 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
srcu_read_unlock_notrace(&tracepoint_srcu, __idx);\
} \
\
- preempt_enable_notrace(); \
+ if (mayfault) \
+ rcu_read_unlock_trace(); \
+ else \
+ preempt_enable_notrace(); \
} while (0)
#ifndef MODULE
-#define __DECLARE_TRACE_RCU(name, proto, args, cond) \
+#define __DECLARE_TRACE_RCU(name, proto, args, cond, tp_flags) \
static inline void trace_##name##_rcuidle(proto) \
{ \
if (static_key_false(&__tracepoint_##name.key)) \
__DO_TRACE(name, \
TP_ARGS(args), \
- TP_CONDITION(cond), 1); \
+ TP_CONDITION(cond), 1, tp_flags); \
}
#else
-#define __DECLARE_TRACE_RCU(name, proto, args, cond)
+#define __DECLARE_TRACE_RCU(name, proto, args, cond, tp_flags)
#endif
/*
@@ -248,7 +251,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
* site if it is not watching, as it will need to be active when the
* tracepoint is enabled.
*/
-#define __DECLARE_TRACE(name, proto, args, cond, data_proto) \
+#define __DECLARE_TRACE(name, proto, args, cond, data_proto, tp_flags) \
extern int __traceiter_##name(data_proto); \
DECLARE_STATIC_CALL(tp_func_##name, __traceiter_##name); \
extern struct tracepoint __tracepoint_##name; \
@@ -257,14 +260,16 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
if (static_key_false(&__tracepoint_##name.key)) \
__DO_TRACE(name, \
TP_ARGS(args), \
- TP_CONDITION(cond), 0); \
+ TP_CONDITION(cond), 0, tp_flags); \
if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) { \
WARN_ONCE(!rcu_is_watching(), \
"RCU not watching for tracepoint"); \
} \
+ if ((tp_flags) & TRACEPOINT_MAY_FAULT) \
+ might_fault(); \
} \
__DECLARE_TRACE_RCU(name, PARAMS(proto), PARAMS(args), \
- PARAMS(cond)) \
+ PARAMS(cond), tp_flags) \
static inline int \
register_trace_##name(void (*probe)(data_proto), void *data) \
{ \
@@ -279,6 +284,13 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
(void *)probe, data, prio); \
} \
static inline int \
+ register_trace_prio_flags_##name(void (*probe)(data_proto), void *data, \
+ int prio, unsigned int flags) \
+ { \
+ return tracepoint_probe_register_prio_flags(&__tracepoint_##name, \
+ (void *)probe, data, prio, flags); \
+ } \
+ static inline int \
unregister_trace_##name(void (*probe)(data_proto), void *data) \
{ \
return tracepoint_probe_unregister(&__tracepoint_##name,\
@@ -299,7 +311,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
* structures, so we create an array of pointers that will be used for iteration
* on the tracepoints.
*/
-#define DEFINE_TRACE_FN(_name, _reg, _unreg, proto, args) \
+#define DEFINE_TRACE_FN_FLAGS(_name, _reg, _unreg, proto, args, tp_flags) \
static const char __tpstrtab_##_name[] \
__section("__tracepoints_strings") = #_name; \
extern struct static_call_key STATIC_CALL_KEY(tp_func_##_name); \
@@ -315,7 +327,9 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
.probestub = &__probestub_##_name, \
.regfunc = _reg, \
.unregfunc = _unreg, \
- .funcs = NULL }; \
+ .funcs = NULL, \
+ .flags = (tp_flags), \
+ }; \
__TRACEPOINT_ENTRY(_name); \
int __traceiter_##_name(void *__data, proto) \
{ \
@@ -338,8 +352,11 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
} \
DEFINE_STATIC_CALL(tp_func_##_name, __traceiter_##_name);
+#define DEFINE_TRACE_FN(_name, _reg, _unreg, proto, args) \
+ DEFINE_TRACE_FN_FLAGS(_name, _reg, _unreg, PARAMS(proto), PARAMS(args), 0)
+
#define DEFINE_TRACE(name, proto, args) \
- DEFINE_TRACE_FN(name, NULL, NULL, PARAMS(proto), PARAMS(args));
+ DEFINE_TRACE_FN(name, NULL, NULL, PARAMS(proto), PARAMS(args))
#define EXPORT_TRACEPOINT_SYMBOL_GPL(name) \
EXPORT_SYMBOL_GPL(__tracepoint_##name); \
@@ -352,7 +369,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
#else /* !TRACEPOINTS_ENABLED */
-#define __DECLARE_TRACE(name, proto, args, cond, data_proto) \
+#define __DECLARE_TRACE(name, proto, args, cond, data_proto, tp_flags) \
static inline void trace_##name(proto) \
{ } \
static inline void trace_##name##_rcuidle(proto) \
@@ -364,6 +381,18 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
return -ENOSYS; \
} \
static inline int \
+ register_trace_prio_##name(void (*probe)(data_proto), \
+ void *data, int prio) \
+ { \
+ return -ENOSYS; \
+ } \
+ static inline int \
+ register_trace_prio_flags_##name(void (*probe)(data_proto), \
+ void *data, int prio, unsigned int flags) \
+ { \
+ return -ENOSYS; \
+ } \
+ static inline int \
unregister_trace_##name(void (*probe)(data_proto), \
void *data) \
{ \
@@ -378,6 +407,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
return false; \
}
+#define DEFINE_TRACE_FN_FLAGS(name, reg, unreg, proto, args, tp_flags)
#define DEFINE_TRACE_FN(name, reg, unreg, proto, args)
#define DEFINE_TRACE(name, proto, args)
#define EXPORT_TRACEPOINT_SYMBOL_GPL(name)
@@ -432,12 +462,17 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
#define DECLARE_TRACE(name, proto, args) \
__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args), \
cpu_online(raw_smp_processor_id()), \
- PARAMS(void *__data, proto))
+ PARAMS(void *__data, proto), 0)
+
+#define DECLARE_TRACE_MAY_FAULT(name, proto, args) \
+ __DECLARE_TRACE(name, PARAMS(proto), PARAMS(args), \
+ cpu_online(raw_smp_processor_id()), \
+ PARAMS(void *__data, proto), TRACEPOINT_MAY_FAULT)
#define DECLARE_TRACE_CONDITION(name, proto, args, cond) \
__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args), \
cpu_online(raw_smp_processor_id()) && (PARAMS(cond)), \
- PARAMS(void *__data, proto))
+ PARAMS(void *__data, proto), 0)
#define TRACE_EVENT_FLAGS(event, flag)
@@ -568,6 +603,9 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
#define TRACE_EVENT_FN(name, proto, args, struct, \
assign, print, reg, unreg) \
DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
+#define TRACE_EVENT_FN_MAY_FAULT(name, proto, args, struct, \
+ assign, print, reg, unreg) \
+ DECLARE_TRACE_MAY_FAULT(name, PARAMS(proto), PARAMS(args))
#define TRACE_EVENT_FN_COND(name, proto, args, cond, struct, \
assign, print, reg, unreg) \
DECLARE_TRACE_CONDITION(name, PARAMS(proto), \
diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
index 00723935dcc7..1b8ca143724a 100644
--- a/include/trace/define_trace.h
+++ b/include/trace/define_trace.h
@@ -41,6 +41,12 @@
assign, print, reg, unreg) \
DEFINE_TRACE_FN(name, reg, unreg, PARAMS(proto), PARAMS(args))
+#undef TRACE_EVENT_FN_MAY_FAULT
+#define TRACE_EVENT_FN_MAY_FAULT(name, proto, args, tstruct, \
+ assign, print, reg, unreg) \
+ DEFINE_TRACE_FN_FLAGS(name, reg, unreg, PARAMS(proto), \
+ PARAMS(args), TRACEPOINT_MAY_FAULT)
+
#undef TRACE_EVENT_FN_COND
#define TRACE_EVENT_FN_COND(name, proto, args, cond, tstruct, \
assign, print, reg, unreg) \
@@ -106,6 +112,7 @@
#undef TRACE_EVENT
#undef TRACE_EVENT_FN
+#undef TRACE_EVENT_FN_MAY_FAULT
#undef TRACE_EVENT_FN_COND
#undef TRACE_EVENT_CONDITION
#undef TRACE_EVENT_NOP
diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h
index c2f9cabf154d..df590eea8ae4 100644
--- a/include/trace/trace_events.h
+++ b/include/trace/trace_events.h
@@ -77,6 +77,12 @@
TRACE_EVENT(name, PARAMS(proto), PARAMS(args), \
PARAMS(tstruct), PARAMS(assign), PARAMS(print)) \
+#undef TRACE_EVENT_FN_MAY_FAULT
+#define TRACE_EVENT_FN_MAY_FAULT(name, proto, args, tstruct, \
+ assign, print, reg, unreg) \
+ TRACE_EVENT(name, PARAMS(proto), PARAMS(args), \
+ PARAMS(tstruct), PARAMS(assign), PARAMS(print)) \
+
#undef TRACE_EVENT_FN_COND
#define TRACE_EVENT_FN_COND(name, proto, args, cond, tstruct, \
assign, print, reg, unreg) \
diff --git a/init/Kconfig b/init/Kconfig
index 5783a0b87517..72e13ee73c43 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1936,6 +1936,7 @@ config BINDGEN_VERSION_TEXT
#
config TRACEPOINTS
bool
+ select TASKS_TRACE_RCU
source "kernel/Kconfig.kexec"
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index cd098846e251..c77eb80cbd7f 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2471,7 +2471,9 @@ int bpf_probe_register(struct bpf_raw_event_map *btp, struct bpf_raw_tp_link *li
if (prog->aux->max_tp_access > btp->writable_size)
return -EINVAL;
- return tracepoint_probe_register_may_exist(tp, (void *)btp->bpf_func, link);
+ return tracepoint_probe_register_prio_flags(tp, (void *)btp->bpf_func,
+ link, TRACEPOINT_DEFAULT_PRIO,
+ TRACEPOINT_MAY_EXIST);
}
int bpf_probe_unregister(struct bpf_raw_event_map *btp, struct bpf_raw_tp_link *link)
diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
index 62e6a8f4aae9..f4f77dfed565 100644
--- a/kernel/trace/trace_fprobe.c
+++ b/kernel/trace/trace_fprobe.c
@@ -705,8 +705,9 @@ static int __register_trace_fprobe(struct trace_fprobe *tf)
* At first, put __probestub_##TP function on the tracepoint
* and put a fprobe on the stub function.
*/
- ret = tracepoint_probe_register_prio_may_exist(tpoint,
- tpoint->probestub, NULL, 0);
+ ret = tracepoint_probe_register_prio_flags(tpoint,
+ tpoint->probestub, NULL, 0,
+ TRACEPOINT_MAY_EXIST);
if (ret < 0)
return ret;
return register_fprobe_ips(&tf->fp, &ip, 1);
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 8d1507dd0724..cc5e71383c4d 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -111,11 +111,16 @@ static inline void *allocate_probes(int count)
return p == NULL ? NULL : p->probes;
}
-static void srcu_free_old_probes(struct rcu_head *head)
+static void rcu_tasks_trace_free_old_probes(struct rcu_head *head)
{
kfree(container_of(head, struct tp_probes, rcu));
}
+static void srcu_free_old_probes(struct rcu_head *head)
+{
+ call_rcu_tasks_trace(head, rcu_tasks_trace_free_old_probes);
+}
+
static void rcu_free_old_probes(struct rcu_head *head)
{
call_srcu(&tracepoint_srcu, head, srcu_free_old_probes);
@@ -136,7 +141,7 @@ static __init int release_early_probes(void)
return 0;
}
-/* SRCU is initialized at core_initcall */
+/* SRCU and Tasks Trace RCU are initialized at core_initcall */
postcore_initcall(release_early_probes);
static inline void release_probes(struct tracepoint_func *old)
@@ -146,8 +151,9 @@ static inline void release_probes(struct tracepoint_func *old)
struct tp_probes, probes[0]);
/*
- * We can't free probes if SRCU is not initialized yet.
- * Postpone the freeing till after SRCU is initialized.
+ * We can't free probes if SRCU and Tasks Trace RCU are not
+ * initialized yet. Postpone the freeing till after both are
+ * initialized.
*/
if (unlikely(!ok_to_free_tracepoints)) {
tp_probes->rcu.next = early_probes;
@@ -156,10 +162,9 @@ static inline void release_probes(struct tracepoint_func *old)
}
/*
- * Tracepoint probes are protected by both sched RCU and SRCU,
- * by calling the SRCU callback in the sched RCU callback we
- * cover both cases. So let us chain the SRCU and sched RCU
- * callbacks to wait for both grace periods.
+ * Tracepoint probes are protected by sched RCU, SRCU and
+ * Tasks Trace RCU by chaining the callbacks we cover all three
+ * cases and wait for all three grace periods.
*/
call_rcu(&tp_probes->rcu, rcu_free_old_probes);
}
@@ -460,30 +465,45 @@ static int tracepoint_remove_func(struct tracepoint *tp,
}
/**
- * tracepoint_probe_register_prio_may_exist - Connect a probe to a tracepoint with priority
+ * tracepoint_probe_register_prio_flags - Connect a probe to a tracepoint with priority and flags
* @tp: tracepoint
* @probe: probe handler
* @data: tracepoint data
* @prio: priority of this function over other registered functions
+ * @flags: tracepoint flags argument (enum tracepoint_flags bits)
*
- * Same as tracepoint_probe_register_prio() except that it will not warn
- * if the tracepoint is already registered.
+ * Returns 0 if ok, error value on error.
+ * Note: if @tp is within a module, the caller is responsible for
+ * unregistering the probe before the module is gone. This can be
+ * performed either with a tracepoint module going notifier, or from
+ * within module exit functions.
*/
-int tracepoint_probe_register_prio_may_exist(struct tracepoint *tp, void *probe,
- void *data, int prio)
+int tracepoint_probe_register_prio_flags(struct tracepoint *tp, void *probe,
+ void *data, int prio, unsigned int flags)
{
struct tracepoint_func tp_func;
int ret;
+ /*
+ * For a probe to be registered to a tracepoint they must share the
+ * same MAY_FAULT flag value.
+ */
+ if ((tp->flags & TRACEPOINT_MAY_FAULT) != (flags & TRACEPOINT_MAY_FAULT))
+ return -EINVAL;
+
mutex_lock(&tracepoints_mutex);
tp_func.func = probe;
tp_func.data = data;
tp_func.prio = prio;
- ret = tracepoint_add_func(tp, &tp_func, prio, false);
+ /*
+ * When the MAY_EXIST flag is set, don't warn if the tracepoint is
+ * already registered.
+ */
+ ret = tracepoint_add_func(tp, &tp_func, prio, flags & TRACEPOINT_MAY_EXIST);
mutex_unlock(&tracepoints_mutex);
return ret;
}
-EXPORT_SYMBOL_GPL(tracepoint_probe_register_prio_may_exist);
+EXPORT_SYMBOL_GPL(tracepoint_probe_register_prio_flags);
/**
* tracepoint_probe_register_prio - Connect a probe to a tracepoint with priority
@@ -501,16 +521,7 @@ EXPORT_SYMBOL_GPL(tracepoint_probe_register_prio_may_exist);
int tracepoint_probe_register_prio(struct tracepoint *tp, void *probe,
void *data, int prio)
{
- struct tracepoint_func tp_func;
- int ret;
-
- mutex_lock(&tracepoints_mutex);
- tp_func.func = probe;
- tp_func.data = data;
- tp_func.prio = prio;
- ret = tracepoint_add_func(tp, &tp_func, prio, true);
- mutex_unlock(&tracepoints_mutex);
- return ret;
+ return tracepoint_probe_register_prio_flags(tp, probe, data, prio, 0);
}
EXPORT_SYMBOL_GPL(tracepoint_probe_register_prio);
@@ -520,6 +531,8 @@ EXPORT_SYMBOL_GPL(tracepoint_probe_register_prio);
* @probe: probe handler
* @data: tracepoint data
*
+ * Non-faultable probes can only be registered on non-faultable tracepoints.
+ *
* Returns 0 if ok, error value on error.
* Note: if @tp is within a module, the caller is responsible for
* unregistering the probe before the module is gone. This can be
@@ -528,7 +541,7 @@ EXPORT_SYMBOL_GPL(tracepoint_probe_register_prio);
*/
int tracepoint_probe_register(struct tracepoint *tp, void *probe, void *data)
{
- return tracepoint_probe_register_prio(tp, probe, data, TRACEPOINT_DEFAULT_PRIO);
+ return tracepoint_probe_register_prio_flags(tp, probe, data, TRACEPOINT_DEFAULT_PRIO, 0);
}
EXPORT_SYMBOL_GPL(tracepoint_probe_register);
--
2.39.2
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v6 2/5] tracing/ftrace: Add support for faultable tracepoints
2024-08-28 14:41 [PATCH v6 0/5] Faultable Tracepoints Mathieu Desnoyers
2024-08-28 14:41 ` [PATCH v6 1/5] tracing: Introduce faultable tracepoints Mathieu Desnoyers
@ 2024-08-28 14:41 ` Mathieu Desnoyers
2024-08-28 14:41 ` [PATCH v6 3/5] tracing/bpf-trace: " Mathieu Desnoyers
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Mathieu Desnoyers @ 2024-08-28 14:41 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu
Cc: linux-kernel, Mathieu Desnoyers, Peter Zijlstra,
Alexei Starovoitov, Yonghong Song, Paul E . McKenney,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Namhyung Kim, bpf, Joel Fernandes,
linux-trace-kernel, Michael Jeanson
In preparation for converting system call enter/exit instrumentation
into faultable tracepoints, make sure that ftrace can handle registering
to such tracepoints by explicitly disabling preemption within the ftrace
tracepoint probes to respect the current expectations within ftrace ring
buffer code.
This change does not yet allow ftrace to take page faults per se within
its probe, but allows its existing probes to connect to faultable
tracepoints.
Link: https://lore.kernel.org/lkml/20231002202531.3160-1-mathieu.desnoyers@efficios.com/
Co-developed-by: Michael Jeanson <mjeanson@efficios.com>
Signed-off-by: Michael Jeanson <mjeanson@efficios.com>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Yonghong Song <yhs@fb.com>
Cc: Paul E. McKenney <paulmck@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: bpf@vger.kernel.org
Cc: Joel Fernandes <joel@joelfernandes.org>
---
Changes since v4:
- Use DEFINE_INACTIVE_GUARD.
- Add brackets to multiline 'if' statements.
Changes since v5:
- Pass the TRACEPOINT_MAY_FAULT flag directly to tracepoint_probe_register_prio_flags.
---
include/trace/trace_events.h | 64 ++++++++++++++++++++++++++++++++++--
kernel/trace/trace_events.c | 16 +++++----
2 files changed, 71 insertions(+), 9 deletions(-)
diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h
index df590eea8ae4..c887f7b6fbe9 100644
--- a/include/trace/trace_events.h
+++ b/include/trace/trace_events.h
@@ -45,6 +45,16 @@
PARAMS(print)); \
DEFINE_EVENT(name, name, PARAMS(proto), PARAMS(args));
+#undef TRACE_EVENT_MAY_FAULT
+#define TRACE_EVENT_MAY_FAULT(name, proto, args, tstruct, assign, print) \
+ DECLARE_EVENT_CLASS_MAY_FAULT(name, \
+ PARAMS(proto), \
+ PARAMS(args), \
+ PARAMS(tstruct), \
+ PARAMS(assign), \
+ PARAMS(print)); \
+ DEFINE_EVENT(name, name, PARAMS(proto), PARAMS(args));
+
#include "stages/stage1_struct_define.h"
#undef DECLARE_EVENT_CLASS
@@ -57,6 +67,11 @@
\
static struct trace_event_class event_class_##name;
+#undef DECLARE_EVENT_CLASS_MAY_FAULT
+#define DECLARE_EVENT_CLASS_MAY_FAULT(name, proto, args, tstruct, assign, print) \
+ DECLARE_EVENT_CLASS(name, PARAMS(proto), PARAMS(args), \
+ PARAMS(tstruct), PARAMS(assign), PARAMS(print))
+
#undef DEFINE_EVENT
#define DEFINE_EVENT(template, name, proto, args) \
static struct trace_event_call __used \
@@ -80,7 +95,7 @@
#undef TRACE_EVENT_FN_MAY_FAULT
#define TRACE_EVENT_FN_MAY_FAULT(name, proto, args, tstruct, \
assign, print, reg, unreg) \
- TRACE_EVENT(name, PARAMS(proto), PARAMS(args), \
+ TRACE_EVENT_MAY_FAULT(name, PARAMS(proto), PARAMS(args), \
PARAMS(tstruct), PARAMS(assign), PARAMS(print)) \
#undef TRACE_EVENT_FN_COND
@@ -123,6 +138,11 @@
tstruct; \
};
+#undef DECLARE_EVENT_CLASS_MAY_FAULT
+#define DECLARE_EVENT_CLASS_MAY_FAULT(call, proto, args, tstruct, assign, print) \
+ DECLARE_EVENT_CLASS(call, PARAMS(proto), PARAMS(args), \
+ PARAMS(tstruct), PARAMS(assign), PARAMS(print))
+
#undef DEFINE_EVENT
#define DEFINE_EVENT(template, name, proto, args)
@@ -214,6 +234,11 @@ static struct trace_event_functions trace_event_type_funcs_##call = { \
.trace = trace_raw_output_##call, \
};
+#undef DECLARE_EVENT_CLASS_MAY_FAULT
+#define DECLARE_EVENT_CLASS_MAY_FAULT(call, proto, args, tstruct, assign, print) \
+ DECLARE_EVENT_CLASS(call, PARAMS(proto), PARAMS(args), \
+ PARAMS(tstruct), PARAMS(assign), PARAMS(print))
+
#undef DEFINE_EVENT_PRINT
#define DEFINE_EVENT_PRINT(template, call, proto, args, print) \
static notrace enum print_line_t \
@@ -250,6 +275,11 @@ static struct trace_event_fields trace_event_fields_##call[] = { \
tstruct \
{} };
+#undef DECLARE_EVENT_CLASS_MAY_FAULT
+#define DECLARE_EVENT_CLASS_MAY_FAULT(call, proto, args, tstruct, assign, print) \
+ DECLARE_EVENT_CLASS(call, PARAMS(proto), PARAMS(args), \
+ PARAMS(tstruct), PARAMS(assign), PARAMS(print))
+
#undef DEFINE_EVENT_PRINT
#define DEFINE_EVENT_PRINT(template, name, proto, args, print)
@@ -271,6 +301,11 @@ static inline notrace int trace_event_get_offsets_##call( \
return __data_size; \
}
+#undef DECLARE_EVENT_CLASS_MAY_FAULT
+#define DECLARE_EVENT_CLASS_MAY_FAULT(call, proto, args, tstruct, assign, print) \
+ DECLARE_EVENT_CLASS(call, PARAMS(proto), PARAMS(args), \
+ PARAMS(tstruct), PARAMS(assign), PARAMS(print))
+
#include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
/*
@@ -380,8 +415,8 @@ static inline notrace int trace_event_get_offsets_##call( \
#include "stages/stage6_event_callback.h"
-#undef DECLARE_EVENT_CLASS
-#define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \
+#undef _DECLARE_EVENT_CLASS
+#define _DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print, tp_flags) \
\
static notrace void \
trace_event_raw_event_##call(void *__data, proto) \
@@ -392,6 +427,13 @@ trace_event_raw_event_##call(void *__data, proto) \
struct trace_event_raw_##call *entry; \
int __data_size; \
\
+ DEFINE_INACTIVE_GUARD(preempt_notrace, trace_event_guard); \
+ \
+ if ((tp_flags) & TRACEPOINT_MAY_FAULT) { \
+ might_fault(); \
+ activate_guard(preempt_notrace, trace_event_guard)(); \
+ } \
+ \
if (trace_trigger_soft_disabled(trace_file)) \
return; \
\
@@ -409,6 +451,17 @@ trace_event_raw_event_##call(void *__data, proto) \
\
trace_event_buffer_commit(&fbuffer); \
}
+
+#undef DECLARE_EVENT_CLASS
+#define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \
+ _DECLARE_EVENT_CLASS(call, PARAMS(proto), PARAMS(args), \
+ PARAMS(tstruct), PARAMS(assign), PARAMS(print), 0)
+
+#undef DECLARE_EVENT_CLASS_MAY_FAULT
+#define DECLARE_EVENT_CLASS_MAY_FAULT(call, proto, args, tstruct, assign, print) \
+ _DECLARE_EVENT_CLASS(call, PARAMS(proto), PARAMS(args), \
+ PARAMS(tstruct), PARAMS(assign), PARAMS(print), TRACEPOINT_MAY_FAULT)
+
/*
* The ftrace_test_probe is compiled out, it is only here as a build time check
* to make sure that if the tracepoint handling changes, the ftrace probe will
@@ -440,6 +493,11 @@ static struct trace_event_class __used __refdata event_class_##call = { \
_TRACE_PERF_INIT(call) \
};
+#undef DECLARE_EVENT_CLASS_MAY_FAULT
+#define DECLARE_EVENT_CLASS_MAY_FAULT(call, proto, args, tstruct, assign, print) \
+ DECLARE_EVENT_CLASS(call, PARAMS(proto), PARAMS(args), \
+ PARAMS(tstruct), PARAMS(assign), PARAMS(print))
+
#undef DEFINE_EVENT
#define DEFINE_EVENT(template, call, proto, args) \
\
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 7266ec2a4eea..5722e426143d 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -532,9 +532,11 @@ int trace_event_reg(struct trace_event_call *call,
WARN_ON(!(call->flags & TRACE_EVENT_FL_TRACEPOINT));
switch (type) {
case TRACE_REG_REGISTER:
- return tracepoint_probe_register(call->tp,
- call->class->probe,
- file);
+ return tracepoint_probe_register_prio_flags(call->tp,
+ call->class->probe,
+ file,
+ TRACEPOINT_DEFAULT_PRIO,
+ call->tp->flags & TRACEPOINT_MAY_FAULT);
case TRACE_REG_UNREGISTER:
tracepoint_probe_unregister(call->tp,
call->class->probe,
@@ -543,9 +545,11 @@ int trace_event_reg(struct trace_event_call *call,
#ifdef CONFIG_PERF_EVENTS
case TRACE_REG_PERF_REGISTER:
- return tracepoint_probe_register(call->tp,
- call->class->perf_probe,
- call);
+ return tracepoint_probe_register_prio_flags(call->tp,
+ call->class->perf_probe,
+ call,
+ TRACEPOINT_DEFAULT_PRIO,
+ call->tp->flags & TRACEPOINT_MAY_FAULT);
case TRACE_REG_PERF_UNREGISTER:
tracepoint_probe_unregister(call->tp,
call->class->perf_probe,
--
2.39.2
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v6 3/5] tracing/bpf-trace: Add support for faultable tracepoints
2024-08-28 14:41 [PATCH v6 0/5] Faultable Tracepoints Mathieu Desnoyers
2024-08-28 14:41 ` [PATCH v6 1/5] tracing: Introduce faultable tracepoints Mathieu Desnoyers
2024-08-28 14:41 ` [PATCH v6 2/5] tracing/ftrace: Add support for " Mathieu Desnoyers
@ 2024-08-28 14:41 ` Mathieu Desnoyers
2024-09-05 1:21 ` Andrii Nakryiko
2024-08-28 14:41 ` [PATCH v6 4/5] tracing/perf: " Mathieu Desnoyers
2024-08-28 14:41 ` [PATCH v6 5/5] tracing: Convert sys_enter/exit to " Mathieu Desnoyers
4 siblings, 1 reply; 11+ messages in thread
From: Mathieu Desnoyers @ 2024-08-28 14:41 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu
Cc: linux-kernel, Mathieu Desnoyers, Peter Zijlstra,
Alexei Starovoitov, Yonghong Song, Paul E . McKenney,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Namhyung Kim, bpf, Joel Fernandes,
linux-trace-kernel, Michael Jeanson
In preparation for converting system call enter/exit instrumentation
into faultable tracepoints, make sure that bpf can handle registering to
such tracepoints by explicitly disabling preemption within the bpf
tracepoint probes to respect the current expectations within bpf tracing
code.
This change does not yet allow bpf to take page faults per se within its
probe, but allows its existing probes to connect to faultable
tracepoints.
Link: https://lore.kernel.org/lkml/20231002202531.3160-1-mathieu.desnoyers@efficios.com/
Co-developed-by: Michael Jeanson <mjeanson@efficios.com>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Michael Jeanson <mjeanson@efficios.com>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Yonghong Song <yhs@fb.com>
Cc: Paul E. McKenney <paulmck@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: bpf@vger.kernel.org
Cc: Joel Fernandes <joel@joelfernandes.org>
---
Changes since v4:
- Use DEFINE_INACTIVE_GUARD.
- Add brackets to multiline 'if' statements.
Changes since v5:
- Rebased on v6.11-rc5.
- Pass the TRACEPOINT_MAY_FAULT flag directly to tracepoint_probe_register_prio_flags.
---
include/trace/bpf_probe.h | 21 ++++++++++++++++-----
kernel/trace/bpf_trace.c | 2 +-
2 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/include/trace/bpf_probe.h b/include/trace/bpf_probe.h
index a2ea11cc912e..cc96dd1e7c3d 100644
--- a/include/trace/bpf_probe.h
+++ b/include/trace/bpf_probe.h
@@ -42,16 +42,27 @@
/* tracepoints with more than 12 arguments will hit build error */
#define CAST_TO_U64(...) CONCATENATE(__CAST, COUNT_ARGS(__VA_ARGS__))(__VA_ARGS__)
-#define __BPF_DECLARE_TRACE(call, proto, args) \
+#define __BPF_DECLARE_TRACE(call, proto, args, tp_flags) \
static notrace void \
__bpf_trace_##call(void *__data, proto) \
{ \
- CONCATENATE(bpf_trace_run, COUNT_ARGS(args))(__data, CAST_TO_U64(args)); \
+ DEFINE_INACTIVE_GUARD(preempt_notrace, bpf_trace_guard); \
+ \
+ if ((tp_flags) & TRACEPOINT_MAY_FAULT) { \
+ might_fault(); \
+ activate_guard(preempt_notrace, bpf_trace_guard)(); \
+ } \
+ \
+ CONCATENATE(bpf_trace_run, COUNT_ARGS(args))(__data, CAST_TO_U64(args)); \
}
#undef DECLARE_EVENT_CLASS
#define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \
- __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args))
+ __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args), 0)
+
+#undef DECLARE_EVENT_CLASS_MAY_FAULT
+#define DECLARE_EVENT_CLASS_MAY_FAULT(call, proto, args, tstruct, assign, print) \
+ __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args), TRACEPOINT_MAY_FAULT)
/*
* This part is compiled out, it is only here as a build time check
@@ -105,13 +116,13 @@ static inline void bpf_test_buffer_##call(void) \
#undef DECLARE_TRACE
#define DECLARE_TRACE(call, proto, args) \
- __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args)) \
+ __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args), 0) \
__DEFINE_EVENT(call, call, PARAMS(proto), PARAMS(args), 0)
#undef DECLARE_TRACE_WRITABLE
#define DECLARE_TRACE_WRITABLE(call, proto, args, size) \
__CHECK_WRITABLE_BUF_SIZE(call, PARAMS(proto), PARAMS(args), size) \
- __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args)) \
+ __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args), 0) \
__DEFINE_EVENT(call, call, PARAMS(proto), PARAMS(args), size)
#include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index c77eb80cbd7f..ed07283d505b 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2473,7 +2473,7 @@ int bpf_probe_register(struct bpf_raw_event_map *btp, struct bpf_raw_tp_link *li
return tracepoint_probe_register_prio_flags(tp, (void *)btp->bpf_func,
link, TRACEPOINT_DEFAULT_PRIO,
- TRACEPOINT_MAY_EXIST);
+ TRACEPOINT_MAY_EXIST | (tp->flags & TRACEPOINT_MAY_FAULT));
}
int bpf_probe_unregister(struct bpf_raw_event_map *btp, struct bpf_raw_tp_link *link)
--
2.39.2
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v6 3/5] tracing/bpf-trace: Add support for faultable tracepoints
2024-08-28 14:41 ` [PATCH v6 3/5] tracing/bpf-trace: " Mathieu Desnoyers
@ 2024-09-05 1:21 ` Andrii Nakryiko
2024-09-09 15:11 ` Mathieu Desnoyers
0 siblings, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2024-09-05 1:21 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Steven Rostedt, Masami Hiramatsu, linux-kernel, Peter Zijlstra,
Alexei Starovoitov, Yonghong Song, Paul E . McKenney,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Namhyung Kim, bpf, Joel Fernandes,
linux-trace-kernel, Michael Jeanson
On Wed, Aug 28, 2024 at 7:42 AM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> In preparation for converting system call enter/exit instrumentation
> into faultable tracepoints, make sure that bpf can handle registering to
> such tracepoints by explicitly disabling preemption within the bpf
> tracepoint probes to respect the current expectations within bpf tracing
> code.
>
> This change does not yet allow bpf to take page faults per se within its
> probe, but allows its existing probes to connect to faultable
> tracepoints.
>
> Link: https://lore.kernel.org/lkml/20231002202531.3160-1-mathieu.desnoyers@efficios.com/
> Co-developed-by: Michael Jeanson <mjeanson@efficios.com>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Signed-off-by: Michael Jeanson <mjeanson@efficios.com>
> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Yonghong Song <yhs@fb.com>
> Cc: Paul E. McKenney <paulmck@kernel.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: bpf@vger.kernel.org
> Cc: Joel Fernandes <joel@joelfernandes.org>
> ---
> Changes since v4:
> - Use DEFINE_INACTIVE_GUARD.
> - Add brackets to multiline 'if' statements.
> Changes since v5:
> - Rebased on v6.11-rc5.
> - Pass the TRACEPOINT_MAY_FAULT flag directly to tracepoint_probe_register_prio_flags.
> ---
> include/trace/bpf_probe.h | 21 ++++++++++++++++-----
> kernel/trace/bpf_trace.c | 2 +-
> 2 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/include/trace/bpf_probe.h b/include/trace/bpf_probe.h
> index a2ea11cc912e..cc96dd1e7c3d 100644
> --- a/include/trace/bpf_probe.h
> +++ b/include/trace/bpf_probe.h
> @@ -42,16 +42,27 @@
> /* tracepoints with more than 12 arguments will hit build error */
> #define CAST_TO_U64(...) CONCATENATE(__CAST, COUNT_ARGS(__VA_ARGS__))(__VA_ARGS__)
>
> -#define __BPF_DECLARE_TRACE(call, proto, args) \
> +#define __BPF_DECLARE_TRACE(call, proto, args, tp_flags) \
> static notrace void \
> __bpf_trace_##call(void *__data, proto) \
> { \
> - CONCATENATE(bpf_trace_run, COUNT_ARGS(args))(__data, CAST_TO_U64(args)); \
> + DEFINE_INACTIVE_GUARD(preempt_notrace, bpf_trace_guard); \
> + \
> + if ((tp_flags) & TRACEPOINT_MAY_FAULT) { \
> + might_fault(); \
> + activate_guard(preempt_notrace, bpf_trace_guard)(); \
> + } \
> + \
> + CONCATENATE(bpf_trace_run, COUNT_ARGS(args))(__data, CAST_TO_U64(args)); \
> }
>
> #undef DECLARE_EVENT_CLASS
> #define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \
> - __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args))
> + __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args), 0)
> +
> +#undef DECLARE_EVENT_CLASS_MAY_FAULT
> +#define DECLARE_EVENT_CLASS_MAY_FAULT(call, proto, args, tstruct, assign, print) \
> + __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args), TRACEPOINT_MAY_FAULT)
>
> /*
> * This part is compiled out, it is only here as a build time check
> @@ -105,13 +116,13 @@ static inline void bpf_test_buffer_##call(void) \
>
> #undef DECLARE_TRACE
> #define DECLARE_TRACE(call, proto, args) \
> - __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args)) \
> + __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args), 0) \
> __DEFINE_EVENT(call, call, PARAMS(proto), PARAMS(args), 0)
>
> #undef DECLARE_TRACE_WRITABLE
> #define DECLARE_TRACE_WRITABLE(call, proto, args, size) \
> __CHECK_WRITABLE_BUF_SIZE(call, PARAMS(proto), PARAMS(args), size) \
> - __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args)) \
> + __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args), 0) \
> __DEFINE_EVENT(call, call, PARAMS(proto), PARAMS(args), size)
>
> #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index c77eb80cbd7f..ed07283d505b 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2473,7 +2473,7 @@ int bpf_probe_register(struct bpf_raw_event_map *btp, struct bpf_raw_tp_link *li
>
> return tracepoint_probe_register_prio_flags(tp, (void *)btp->bpf_func,
> link, TRACEPOINT_DEFAULT_PRIO,
> - TRACEPOINT_MAY_EXIST);
> + TRACEPOINT_MAY_EXIST | (tp->flags & TRACEPOINT_MAY_FAULT));
> }
>
> int bpf_probe_unregister(struct bpf_raw_event_map *btp, struct bpf_raw_tp_link *link)
> --
> 2.39.2
>
>
I wonder if it would be better to just do this, instead of that
preempt guard. I think we don't strictly need preemption to be
disabled, we just need to stay on the same CPU, just like we do that
for many other program types.
We'll need some more BPF-specific plumbing to fully support faultable
(sleepable) tracepoints, but this should unblock your work, unless I'm
missing something. And we can take it from there, once your patches
land, to take advantage of faultable tracepoints in the BPF ecosystem.
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index b69a39316c0c..415639b7c7a4 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2302,7 +2302,8 @@ void __bpf_trace_run(struct bpf_raw_tp_link
*link, u64 *args)
struct bpf_run_ctx *old_run_ctx;
struct bpf_trace_run_ctx run_ctx;
- cant_sleep();
+ migrate_disable();
+
if (unlikely(this_cpu_inc_return(*(prog->active)) != 1)) {
bpf_prog_inc_misses_counter(prog);
goto out;
@@ -2318,6 +2319,8 @@ void __bpf_trace_run(struct bpf_raw_tp_link
*link, u64 *args)
bpf_reset_run_ctx(old_run_ctx);
out:
this_cpu_dec(*(prog->active));
+
+ migrate_enable();
}
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v6 3/5] tracing/bpf-trace: Add support for faultable tracepoints
2024-09-05 1:21 ` Andrii Nakryiko
@ 2024-09-09 15:11 ` Mathieu Desnoyers
2024-09-09 16:53 ` Andrii Nakryiko
0 siblings, 1 reply; 11+ messages in thread
From: Mathieu Desnoyers @ 2024-09-09 15:11 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Steven Rostedt, Masami Hiramatsu, linux-kernel, Peter Zijlstra,
Alexei Starovoitov, Yonghong Song, Paul E . McKenney,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Namhyung Kim, bpf, Joel Fernandes,
linux-trace-kernel, Michael Jeanson
On 2024-09-04 21:21, Andrii Nakryiko wrote:
> On Wed, Aug 28, 2024 at 7:42 AM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> In preparation for converting system call enter/exit instrumentation
>> into faultable tracepoints, make sure that bpf can handle registering to
>> such tracepoints by explicitly disabling preemption within the bpf
>> tracepoint probes to respect the current expectations within bpf tracing
>> code.
>>
>> This change does not yet allow bpf to take page faults per se within its
>> probe, but allows its existing probes to connect to faultable
>> tracepoints.
>>
>> Link: https://lore.kernel.org/lkml/20231002202531.3160-1-mathieu.desnoyers@efficios.com/
>> Co-developed-by: Michael Jeanson <mjeanson@efficios.com>
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> Signed-off-by: Michael Jeanson <mjeanson@efficios.com>
>> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Masami Hiramatsu <mhiramat@kernel.org>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Alexei Starovoitov <ast@kernel.org>
>> Cc: Yonghong Song <yhs@fb.com>
>> Cc: Paul E. McKenney <paulmck@kernel.org>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: bpf@vger.kernel.org
>> Cc: Joel Fernandes <joel@joelfernandes.org>
>> ---
>> Changes since v4:
>> - Use DEFINE_INACTIVE_GUARD.
>> - Add brackets to multiline 'if' statements.
>> Changes since v5:
>> - Rebased on v6.11-rc5.
>> - Pass the TRACEPOINT_MAY_FAULT flag directly to tracepoint_probe_register_prio_flags.
>> ---
>> include/trace/bpf_probe.h | 21 ++++++++++++++++-----
>> kernel/trace/bpf_trace.c | 2 +-
>> 2 files changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/trace/bpf_probe.h b/include/trace/bpf_probe.h
>> index a2ea11cc912e..cc96dd1e7c3d 100644
>> --- a/include/trace/bpf_probe.h
>> +++ b/include/trace/bpf_probe.h
>> @@ -42,16 +42,27 @@
>> /* tracepoints with more than 12 arguments will hit build error */
>> #define CAST_TO_U64(...) CONCATENATE(__CAST, COUNT_ARGS(__VA_ARGS__))(__VA_ARGS__)
>>
>> -#define __BPF_DECLARE_TRACE(call, proto, args) \
>> +#define __BPF_DECLARE_TRACE(call, proto, args, tp_flags) \
>> static notrace void \
>> __bpf_trace_##call(void *__data, proto) \
>> { \
>> - CONCATENATE(bpf_trace_run, COUNT_ARGS(args))(__data, CAST_TO_U64(args)); \
>> + DEFINE_INACTIVE_GUARD(preempt_notrace, bpf_trace_guard); \
>> + \
>> + if ((tp_flags) & TRACEPOINT_MAY_FAULT) { \
>> + might_fault(); \
>> + activate_guard(preempt_notrace, bpf_trace_guard)(); \
>> + } \
>> + \
>> + CONCATENATE(bpf_trace_run, COUNT_ARGS(args))(__data, CAST_TO_U64(args)); \
>> }
>>
>> #undef DECLARE_EVENT_CLASS
>> #define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \
>> - __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args))
>> + __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args), 0)
>> +
>> +#undef DECLARE_EVENT_CLASS_MAY_FAULT
>> +#define DECLARE_EVENT_CLASS_MAY_FAULT(call, proto, args, tstruct, assign, print) \
>> + __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args), TRACEPOINT_MAY_FAULT)
>>
>> /*
>> * This part is compiled out, it is only here as a build time check
>> @@ -105,13 +116,13 @@ static inline void bpf_test_buffer_##call(void) \
>>
>> #undef DECLARE_TRACE
>> #define DECLARE_TRACE(call, proto, args) \
>> - __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args)) \
>> + __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args), 0) \
>> __DEFINE_EVENT(call, call, PARAMS(proto), PARAMS(args), 0)
>>
>> #undef DECLARE_TRACE_WRITABLE
>> #define DECLARE_TRACE_WRITABLE(call, proto, args, size) \
>> __CHECK_WRITABLE_BUF_SIZE(call, PARAMS(proto), PARAMS(args), size) \
>> - __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args)) \
>> + __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args), 0) \
>> __DEFINE_EVENT(call, call, PARAMS(proto), PARAMS(args), size)
>>
>> #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>> index c77eb80cbd7f..ed07283d505b 100644
>> --- a/kernel/trace/bpf_trace.c
>> +++ b/kernel/trace/bpf_trace.c
>> @@ -2473,7 +2473,7 @@ int bpf_probe_register(struct bpf_raw_event_map *btp, struct bpf_raw_tp_link *li
>>
>> return tracepoint_probe_register_prio_flags(tp, (void *)btp->bpf_func,
>> link, TRACEPOINT_DEFAULT_PRIO,
>> - TRACEPOINT_MAY_EXIST);
>> + TRACEPOINT_MAY_EXIST | (tp->flags & TRACEPOINT_MAY_FAULT));
>> }
>>
>> int bpf_probe_unregister(struct bpf_raw_event_map *btp, struct bpf_raw_tp_link *link)
>> --
>> 2.39.2
>>
>>
>
> I wonder if it would be better to just do this, instead of that
> preempt guard. I think we don't strictly need preemption to be
> disabled, we just need to stay on the same CPU, just like we do that
> for many other program types.
I'm worried about introducing any kind of subtle synchronization
change in this series, and moving from preempt-off to migrate-disable
definitely falls under that umbrella.
I would recommend auditing all uses of this_cpu_*() APIs to make sure
accesses to per-cpu data structures are using atomics and not just using
operations that expect use of preempt-off to prevent concurrent threads
from updating to the per-cpu data concurrently.
So what you are suggesting may be a good idea, but I prefer to leave
this kind of change to a separate bpf-specific series, and I would
leave this work to someone who knows more about ebpf than me.
Thanks,
Mathieu
>
> We'll need some more BPF-specific plumbing to fully support faultable
> (sleepable) tracepoints, but this should unblock your work, unless I'm
> missing something. And we can take it from there, once your patches
> land, to take advantage of faultable tracepoints in the BPF ecosystem.
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index b69a39316c0c..415639b7c7a4 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2302,7 +2302,8 @@ void __bpf_trace_run(struct bpf_raw_tp_link
> *link, u64 *args)
> struct bpf_run_ctx *old_run_ctx;
> struct bpf_trace_run_ctx run_ctx;
>
> - cant_sleep();
> + migrate_disable();
> +
> if (unlikely(this_cpu_inc_return(*(prog->active)) != 1)) {
> bpf_prog_inc_misses_counter(prog);
> goto out;
> @@ -2318,6 +2319,8 @@ void __bpf_trace_run(struct bpf_raw_tp_link
> *link, u64 *args)
> bpf_reset_run_ctx(old_run_ctx);
> out:
> this_cpu_dec(*(prog->active));
> +
> + migrate_enable();
> }
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v6 3/5] tracing/bpf-trace: Add support for faultable tracepoints
2024-09-09 15:11 ` Mathieu Desnoyers
@ 2024-09-09 16:53 ` Andrii Nakryiko
2024-09-09 17:22 ` Mathieu Desnoyers
0 siblings, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2024-09-09 16:53 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Steven Rostedt, Masami Hiramatsu, linux-kernel, Peter Zijlstra,
Alexei Starovoitov, Yonghong Song, Paul E . McKenney,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Namhyung Kim, bpf, Joel Fernandes,
linux-trace-kernel, Michael Jeanson
On Mon, Sep 9, 2024 at 8:11 AM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> On 2024-09-04 21:21, Andrii Nakryiko wrote:
> > On Wed, Aug 28, 2024 at 7:42 AM Mathieu Desnoyers
> > <mathieu.desnoyers@efficios.com> wrote:
> >>
> >> In preparation for converting system call enter/exit instrumentation
> >> into faultable tracepoints, make sure that bpf can handle registering to
> >> such tracepoints by explicitly disabling preemption within the bpf
> >> tracepoint probes to respect the current expectations within bpf tracing
> >> code.
> >>
> >> This change does not yet allow bpf to take page faults per se within its
> >> probe, but allows its existing probes to connect to faultable
> >> tracepoints.
> >>
> >> Link: https://lore.kernel.org/lkml/20231002202531.3160-1-mathieu.desnoyers@efficios.com/
> >> Co-developed-by: Michael Jeanson <mjeanson@efficios.com>
> >> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> >> Signed-off-by: Michael Jeanson <mjeanson@efficios.com>
> >> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >> Cc: Steven Rostedt <rostedt@goodmis.org>
> >> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> >> Cc: Peter Zijlstra <peterz@infradead.org>
> >> Cc: Alexei Starovoitov <ast@kernel.org>
> >> Cc: Yonghong Song <yhs@fb.com>
> >> Cc: Paul E. McKenney <paulmck@kernel.org>
> >> Cc: Ingo Molnar <mingo@redhat.com>
> >> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> >> Cc: Mark Rutland <mark.rutland@arm.com>
> >> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> >> Cc: Namhyung Kim <namhyung@kernel.org>
> >> Cc: bpf@vger.kernel.org
> >> Cc: Joel Fernandes <joel@joelfernandes.org>
> >> ---
> >> Changes since v4:
> >> - Use DEFINE_INACTIVE_GUARD.
> >> - Add brackets to multiline 'if' statements.
> >> Changes since v5:
> >> - Rebased on v6.11-rc5.
> >> - Pass the TRACEPOINT_MAY_FAULT flag directly to tracepoint_probe_register_prio_flags.
> >> ---
> >> include/trace/bpf_probe.h | 21 ++++++++++++++++-----
> >> kernel/trace/bpf_trace.c | 2 +-
> >> 2 files changed, 17 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/include/trace/bpf_probe.h b/include/trace/bpf_probe.h
> >> index a2ea11cc912e..cc96dd1e7c3d 100644
> >> --- a/include/trace/bpf_probe.h
> >> +++ b/include/trace/bpf_probe.h
> >> @@ -42,16 +42,27 @@
> >> /* tracepoints with more than 12 arguments will hit build error */
> >> #define CAST_TO_U64(...) CONCATENATE(__CAST, COUNT_ARGS(__VA_ARGS__))(__VA_ARGS__)
> >>
> >> -#define __BPF_DECLARE_TRACE(call, proto, args) \
> >> +#define __BPF_DECLARE_TRACE(call, proto, args, tp_flags) \
> >> static notrace void \
> >> __bpf_trace_##call(void *__data, proto) \
> >> { \
> >> - CONCATENATE(bpf_trace_run, COUNT_ARGS(args))(__data, CAST_TO_U64(args)); \
> >> + DEFINE_INACTIVE_GUARD(preempt_notrace, bpf_trace_guard); \
> >> + \
> >> + if ((tp_flags) & TRACEPOINT_MAY_FAULT) { \
> >> + might_fault(); \
> >> + activate_guard(preempt_notrace, bpf_trace_guard)(); \
> >> + } \
> >> + \
> >> + CONCATENATE(bpf_trace_run, COUNT_ARGS(args))(__data, CAST_TO_U64(args)); \
> >> }
> >>
> >> #undef DECLARE_EVENT_CLASS
> >> #define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \
> >> - __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args))
> >> + __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args), 0)
> >> +
> >> +#undef DECLARE_EVENT_CLASS_MAY_FAULT
> >> +#define DECLARE_EVENT_CLASS_MAY_FAULT(call, proto, args, tstruct, assign, print) \
> >> + __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args), TRACEPOINT_MAY_FAULT)
> >>
> >> /*
> >> * This part is compiled out, it is only here as a build time check
> >> @@ -105,13 +116,13 @@ static inline void bpf_test_buffer_##call(void) \
> >>
> >> #undef DECLARE_TRACE
> >> #define DECLARE_TRACE(call, proto, args) \
> >> - __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args)) \
> >> + __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args), 0) \
> >> __DEFINE_EVENT(call, call, PARAMS(proto), PARAMS(args), 0)
> >>
> >> #undef DECLARE_TRACE_WRITABLE
> >> #define DECLARE_TRACE_WRITABLE(call, proto, args, size) \
> >> __CHECK_WRITABLE_BUF_SIZE(call, PARAMS(proto), PARAMS(args), size) \
> >> - __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args)) \
> >> + __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args), 0) \
> >> __DEFINE_EVENT(call, call, PARAMS(proto), PARAMS(args), size)
> >>
> >> #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> >> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> >> index c77eb80cbd7f..ed07283d505b 100644
> >> --- a/kernel/trace/bpf_trace.c
> >> +++ b/kernel/trace/bpf_trace.c
> >> @@ -2473,7 +2473,7 @@ int bpf_probe_register(struct bpf_raw_event_map *btp, struct bpf_raw_tp_link *li
> >>
> >> return tracepoint_probe_register_prio_flags(tp, (void *)btp->bpf_func,
> >> link, TRACEPOINT_DEFAULT_PRIO,
> >> - TRACEPOINT_MAY_EXIST);
> >> + TRACEPOINT_MAY_EXIST | (tp->flags & TRACEPOINT_MAY_FAULT));
> >> }
> >>
> >> int bpf_probe_unregister(struct bpf_raw_event_map *btp, struct bpf_raw_tp_link *link)
> >> --
> >> 2.39.2
> >>
> >>
> >
> > I wonder if it would be better to just do this, instead of that
> > preempt guard. I think we don't strictly need preemption to be
> > disabled, we just need to stay on the same CPU, just like we do that
> > for many other program types.
>
> I'm worried about introducing any kind of subtle synchronization
> change in this series, and moving from preempt-off to migrate-disable
> definitely falls under that umbrella.
>
> I would recommend auditing all uses of this_cpu_*() APIs to make sure
> accesses to per-cpu data structures are using atomics and not just using
> operations that expect use of preempt-off to prevent concurrent threads
> from updating to the per-cpu data concurrently.
>
> So what you are suggesting may be a good idea, but I prefer to leave
> this kind of change to a separate bpf-specific series, and I would
> leave this work to someone who knows more about ebpf than me.
>
Yeah, that's ok. migrate_disable() switch is probably going a bit too
far too fast, but I think we should just add
preempt_disable/preempt_enable inside __bpf_trace_run() instead of
leaving it inside those hard to find and follow tracepoint macros. So
maybe you can just pass a bool into __bpf_trace_run() and do preempt
guard (or explicit disable/enable) there?
> Thanks,
>
> Mathieu
>
> >
> > We'll need some more BPF-specific plumbing to fully support faultable
> > (sleepable) tracepoints, but this should unblock your work, unless I'm
> > missing something. And we can take it from there, once your patches
> > land, to take advantage of faultable tracepoints in the BPF ecosystem.
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index b69a39316c0c..415639b7c7a4 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2302,7 +2302,8 @@ void __bpf_trace_run(struct bpf_raw_tp_link
> > *link, u64 *args)
> > struct bpf_run_ctx *old_run_ctx;
> > struct bpf_trace_run_ctx run_ctx;
> >
> > - cant_sleep();
> > + migrate_disable();
> > +
> > if (unlikely(this_cpu_inc_return(*(prog->active)) != 1)) {
> > bpf_prog_inc_misses_counter(prog);
> > goto out;
> > @@ -2318,6 +2319,8 @@ void __bpf_trace_run(struct bpf_raw_tp_link
> > *link, u64 *args)
> > bpf_reset_run_ctx(old_run_ctx);
> > out:
> > this_cpu_dec(*(prog->active));
> > +
> > + migrate_enable();
> > }
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> https://www.efficios.com
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v6 3/5] tracing/bpf-trace: Add support for faultable tracepoints
2024-09-09 16:53 ` Andrii Nakryiko
@ 2024-09-09 17:22 ` Mathieu Desnoyers
2024-09-09 20:13 ` Andrii Nakryiko
0 siblings, 1 reply; 11+ messages in thread
From: Mathieu Desnoyers @ 2024-09-09 17:22 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Steven Rostedt, Masami Hiramatsu, linux-kernel, Peter Zijlstra,
Alexei Starovoitov, Yonghong Song, Paul E . McKenney,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Namhyung Kim, bpf, Joel Fernandes,
linux-trace-kernel, Michael Jeanson
On 2024-09-09 12:53, Andrii Nakryiko wrote:
> On Mon, Sep 9, 2024 at 8:11 AM Mathieu Desnoyers
[...]
>>>
>>> I wonder if it would be better to just do this, instead of that
>>> preempt guard. I think we don't strictly need preemption to be
>>> disabled, we just need to stay on the same CPU, just like we do that
>>> for many other program types.
>>
>> I'm worried about introducing any kind of subtle synchronization
>> change in this series, and moving from preempt-off to migrate-disable
>> definitely falls under that umbrella.
>>
>> I would recommend auditing all uses of this_cpu_*() APIs to make sure
>> accesses to per-cpu data structures are using atomics and not just using
>> operations that expect use of preempt-off to prevent concurrent threads
>> from updating to the per-cpu data concurrently.
>>
>> So what you are suggesting may be a good idea, but I prefer to leave
>> this kind of change to a separate bpf-specific series, and I would
>> leave this work to someone who knows more about ebpf than me.
>>
>
> Yeah, that's ok. migrate_disable() switch is probably going a bit too
> far too fast, but I think we should just add
> preempt_disable/preempt_enable inside __bpf_trace_run() instead of
> leaving it inside those hard to find and follow tracepoint macros. So
> maybe you can just pass a bool into __bpf_trace_run() and do preempt
> guard (or explicit disable/enable) there?
>
Passing an extra boolean to __bpf_trace_run would impact all tracepoints
calling into ebpf, adding an extra function argument and extra tests for
all of those. The impact may be small, but it is non-zero in both code size
and overhead, so it would not be my preferred approach.
I have modified the macros to add the guard within __bpf_trace_##call
following suggestions from Linus:
https://lore.kernel.org/lkml/CAHk-=wggDLDeTKbhb5hh--x=-DQd69v41137M72m6NOTmbD-cw@mail.gmail.com/
I'll Cc you on that version of the series.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v6 3/5] tracing/bpf-trace: Add support for faultable tracepoints
2024-09-09 17:22 ` Mathieu Desnoyers
@ 2024-09-09 20:13 ` Andrii Nakryiko
0 siblings, 0 replies; 11+ messages in thread
From: Andrii Nakryiko @ 2024-09-09 20:13 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Steven Rostedt, Masami Hiramatsu, linux-kernel, Peter Zijlstra,
Alexei Starovoitov, Yonghong Song, Paul E . McKenney,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Namhyung Kim, bpf, Joel Fernandes,
linux-trace-kernel, Michael Jeanson
On Mon, Sep 9, 2024 at 10:22 AM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> On 2024-09-09 12:53, Andrii Nakryiko wrote:
> > On Mon, Sep 9, 2024 at 8:11 AM Mathieu Desnoyers
> [...]
> >>>
> >>> I wonder if it would be better to just do this, instead of that
> >>> preempt guard. I think we don't strictly need preemption to be
> >>> disabled, we just need to stay on the same CPU, just like we do that
> >>> for many other program types.
> >>
> >> I'm worried about introducing any kind of subtle synchronization
> >> change in this series, and moving from preempt-off to migrate-disable
> >> definitely falls under that umbrella.
> >>
> >> I would recommend auditing all uses of this_cpu_*() APIs to make sure
> >> accesses to per-cpu data structures are using atomics and not just using
> >> operations that expect use of preempt-off to prevent concurrent threads
> >> from updating to the per-cpu data concurrently.
> >>
> >> So what you are suggesting may be a good idea, but I prefer to leave
> >> this kind of change to a separate bpf-specific series, and I would
> >> leave this work to someone who knows more about ebpf than me.
> >>
> >
> > Yeah, that's ok. migrate_disable() switch is probably going a bit too
> > far too fast, but I think we should just add
> > preempt_disable/preempt_enable inside __bpf_trace_run() instead of
> > leaving it inside those hard to find and follow tracepoint macros. So
> > maybe you can just pass a bool into __bpf_trace_run() and do preempt
> > guard (or explicit disable/enable) there?
> >
>
> Passing an extra boolean to __bpf_trace_run would impact all tracepoints
> calling into ebpf, adding an extra function argument and extra tests for
> all of those. The impact may be small, but it is non-zero in both code size
> and overhead, so it would not be my preferred approach.
>
Ok, sounds good to me, we can always change that after your patch set
makes it into upstream.
> I have modified the macros to add the guard within __bpf_trace_##call
> following suggestions from Linus:
>
> https://lore.kernel.org/lkml/CAHk-=wggDLDeTKbhb5hh--x=-DQd69v41137M72m6NOTmbD-cw@mail.gmail.com/
>
> I'll Cc you on that version of the series.
Thanks!
>
> Thanks,
>
> Mathieu
>
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> https://www.efficios.com
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v6 4/5] tracing/perf: Add support for faultable tracepoints
2024-08-28 14:41 [PATCH v6 0/5] Faultable Tracepoints Mathieu Desnoyers
` (2 preceding siblings ...)
2024-08-28 14:41 ` [PATCH v6 3/5] tracing/bpf-trace: " Mathieu Desnoyers
@ 2024-08-28 14:41 ` Mathieu Desnoyers
2024-08-28 14:41 ` [PATCH v6 5/5] tracing: Convert sys_enter/exit to " Mathieu Desnoyers
4 siblings, 0 replies; 11+ messages in thread
From: Mathieu Desnoyers @ 2024-08-28 14:41 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu
Cc: linux-kernel, Mathieu Desnoyers, Peter Zijlstra,
Alexei Starovoitov, Yonghong Song, Paul E . McKenney,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Namhyung Kim, bpf, Joel Fernandes,
linux-trace-kernel, Michael Jeanson
In preparation for converting system call enter/exit instrumentation
into faultable tracepoints, make sure that perf can handle registering
to such tracepoints by explicitly disabling preemption within the perf
tracepoint probes to respect the current expectations within perf ring
buffer code.
This change does not yet allow perf to take page faults per se within
its probe, but allows its existing probes to connect to faultable
tracepoints.
Link: https://lore.kernel.org/lkml/20231002202531.3160-1-mathieu.desnoyers@efficios.com/
Co-developed-by: Michael Jeanson <mjeanson@efficios.com>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Michael Jeanson <mjeanson@efficios.com>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Yonghong Song <yhs@fb.com>
Cc: Paul E. McKenney <paulmck@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: bpf@vger.kernel.org
Cc: Joel Fernandes <joel@joelfernandes.org>
---
Changes since v4:
- Use DEFINE_INACTIVE_GUARD.
---
include/trace/perf.h | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/include/trace/perf.h b/include/trace/perf.h
index 2c11181c82e0..161e1655b953 100644
--- a/include/trace/perf.h
+++ b/include/trace/perf.h
@@ -12,8 +12,8 @@
#undef __perf_task
#define __perf_task(t) (__task = (t))
-#undef DECLARE_EVENT_CLASS
-#define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \
+#undef _DECLARE_EVENT_CLASS
+#define _DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print, tp_flags) \
static notrace void \
perf_trace_##call(void *__data, proto) \
{ \
@@ -28,6 +28,13 @@ perf_trace_##call(void *__data, proto) \
int __data_size; \
int rctx; \
\
+ DEFINE_INACTIVE_GUARD(preempt_notrace, trace_event_guard); \
+ \
+ if ((tp_flags) & TRACEPOINT_MAY_FAULT) { \
+ might_fault(); \
+ activate_guard(preempt_notrace, trace_event_guard)(); \
+ } \
+ \
__data_size = trace_event_get_offsets_##call(&__data_offsets, args); \
\
head = this_cpu_ptr(event_call->perf_events); \
@@ -55,6 +62,17 @@ perf_trace_##call(void *__data, proto) \
head, __task); \
}
+#undef DECLARE_EVENT_CLASS
+#define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \
+ _DECLARE_EVENT_CLASS(call, PARAMS(proto), PARAMS(args), \
+ PARAMS(tstruct), PARAMS(assign), PARAMS(print), 0)
+
+#undef DECLARE_EVENT_CLASS_MAY_FAULT
+#define DECLARE_EVENT_CLASS_MAY_FAULT(call, proto, args, tstruct, assign, print) \
+ _DECLARE_EVENT_CLASS(call, PARAMS(proto), PARAMS(args), \
+ PARAMS(tstruct), PARAMS(assign), PARAMS(print), \
+ TRACEPOINT_MAY_FAULT)
+
/*
* This part is compiled out, it is only here as a build time check
* to make sure that if the tracepoint handling changes, the
--
2.39.2
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v6 5/5] tracing: Convert sys_enter/exit to faultable tracepoints
2024-08-28 14:41 [PATCH v6 0/5] Faultable Tracepoints Mathieu Desnoyers
` (3 preceding siblings ...)
2024-08-28 14:41 ` [PATCH v6 4/5] tracing/perf: " Mathieu Desnoyers
@ 2024-08-28 14:41 ` Mathieu Desnoyers
4 siblings, 0 replies; 11+ messages in thread
From: Mathieu Desnoyers @ 2024-08-28 14:41 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu
Cc: linux-kernel, Mathieu Desnoyers, Peter Zijlstra,
Alexei Starovoitov, Yonghong Song, Paul E . McKenney,
Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland,
Alexander Shishkin, Namhyung Kim, bpf, Joel Fernandes,
linux-trace-kernel, Michael Jeanson
Convert the definition of the system call enter/exit tracepoints to
faultable tracepoints now that all upstream tracers handle it.
This allows tracers to fault-in userspace system call arguments such as
path strings within their probe callbacks.
Link: https://lore.kernel.org/lkml/20231002202531.3160-1-mathieu.desnoyers@efficios.com/
Co-developed-by: Michael Jeanson <mjeanson@efficios.com>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Michael Jeanson <mjeanson@efficios.com>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Yonghong Song <yhs@fb.com>
Cc: Paul E. McKenney <paulmck@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: bpf@vger.kernel.org
Cc: Joel Fernandes <joel@joelfernandes.org>
---
Since v4:
- Use 'guard(preempt_notrace)'.
- Add brackets to multiline 'if' statements.
---
include/trace/events/syscalls.h | 4 +--
kernel/trace/trace_syscalls.c | 52 ++++++++++++++++++++++++++++-----
2 files changed, 46 insertions(+), 10 deletions(-)
diff --git a/include/trace/events/syscalls.h b/include/trace/events/syscalls.h
index b6e0cbc2c71f..dc30e3004818 100644
--- a/include/trace/events/syscalls.h
+++ b/include/trace/events/syscalls.h
@@ -15,7 +15,7 @@
#ifdef CONFIG_HAVE_SYSCALL_TRACEPOINTS
-TRACE_EVENT_FN(sys_enter,
+TRACE_EVENT_FN_MAY_FAULT(sys_enter,
TP_PROTO(struct pt_regs *regs, long id),
@@ -41,7 +41,7 @@ TRACE_EVENT_FN(sys_enter,
TRACE_EVENT_FLAGS(sys_enter, TRACE_EVENT_FL_CAP_ANY)
-TRACE_EVENT_FN(sys_exit,
+TRACE_EVENT_FN_MAY_FAULT(sys_exit,
TP_PROTO(struct pt_regs *regs, long ret),
diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
index 9c581d6da843..314666d663b6 100644
--- a/kernel/trace/trace_syscalls.c
+++ b/kernel/trace/trace_syscalls.c
@@ -299,6 +299,12 @@ static void ftrace_syscall_enter(void *data, struct pt_regs *regs, long id)
int syscall_nr;
int size;
+ /*
+ * Probe called with preemption enabled (may_fault), but ring buffer and
+ * per-cpu data require preemption to be disabled.
+ */
+ guard(preempt_notrace)();
+
syscall_nr = trace_get_syscall_nr(current, regs);
if (syscall_nr < 0 || syscall_nr >= NR_syscalls)
return;
@@ -338,6 +344,12 @@ static void ftrace_syscall_exit(void *data, struct pt_regs *regs, long ret)
struct trace_event_buffer fbuffer;
int syscall_nr;
+ /*
+ * Probe called with preemption enabled (may_fault), but ring buffer and
+ * per-cpu data require preemption to be disabled.
+ */
+ guard(preempt_notrace)();
+
syscall_nr = trace_get_syscall_nr(current, regs);
if (syscall_nr < 0 || syscall_nr >= NR_syscalls)
return;
@@ -376,8 +388,11 @@ static int reg_event_syscall_enter(struct trace_event_file *file,
if (WARN_ON_ONCE(num < 0 || num >= NR_syscalls))
return -ENOSYS;
mutex_lock(&syscall_trace_lock);
- if (!tr->sys_refcount_enter)
- ret = register_trace_sys_enter(ftrace_syscall_enter, tr);
+ if (!tr->sys_refcount_enter) {
+ ret = register_trace_prio_flags_sys_enter(ftrace_syscall_enter, tr,
+ TRACEPOINT_DEFAULT_PRIO,
+ TRACEPOINT_MAY_FAULT);
+ }
if (!ret) {
rcu_assign_pointer(tr->enter_syscall_files[num], file);
tr->sys_refcount_enter++;
@@ -414,8 +429,11 @@ static int reg_event_syscall_exit(struct trace_event_file *file,
if (WARN_ON_ONCE(num < 0 || num >= NR_syscalls))
return -ENOSYS;
mutex_lock(&syscall_trace_lock);
- if (!tr->sys_refcount_exit)
- ret = register_trace_sys_exit(ftrace_syscall_exit, tr);
+ if (!tr->sys_refcount_exit) {
+ ret = register_trace_prio_flags_sys_exit(ftrace_syscall_exit, tr,
+ TRACEPOINT_DEFAULT_PRIO,
+ TRACEPOINT_MAY_FAULT);
+ }
if (!ret) {
rcu_assign_pointer(tr->exit_syscall_files[num], file);
tr->sys_refcount_exit++;
@@ -582,6 +600,12 @@ static void perf_syscall_enter(void *ignore, struct pt_regs *regs, long id)
int rctx;
int size;
+ /*
+ * Probe called with preemption enabled (may_fault), but ring buffer and
+ * per-cpu data require preemption to be disabled.
+ */
+ guard(preempt_notrace)();
+
syscall_nr = trace_get_syscall_nr(current, regs);
if (syscall_nr < 0 || syscall_nr >= NR_syscalls)
return;
@@ -630,8 +654,11 @@ static int perf_sysenter_enable(struct trace_event_call *call)
num = ((struct syscall_metadata *)call->data)->syscall_nr;
mutex_lock(&syscall_trace_lock);
- if (!sys_perf_refcount_enter)
- ret = register_trace_sys_enter(perf_syscall_enter, NULL);
+ if (!sys_perf_refcount_enter) {
+ ret = register_trace_prio_flags_sys_enter(perf_syscall_enter, NULL,
+ TRACEPOINT_DEFAULT_PRIO,
+ TRACEPOINT_MAY_FAULT);
+ }
if (ret) {
pr_info("event trace: Could not activate syscall entry trace point");
} else {
@@ -682,6 +709,12 @@ static void perf_syscall_exit(void *ignore, struct pt_regs *regs, long ret)
int rctx;
int size;
+ /*
+ * Probe called with preemption enabled (may_fault), but ring buffer and
+ * per-cpu data require preemption to be disabled.
+ */
+ guard(preempt_notrace)();
+
syscall_nr = trace_get_syscall_nr(current, regs);
if (syscall_nr < 0 || syscall_nr >= NR_syscalls)
return;
@@ -727,8 +760,11 @@ static int perf_sysexit_enable(struct trace_event_call *call)
num = ((struct syscall_metadata *)call->data)->syscall_nr;
mutex_lock(&syscall_trace_lock);
- if (!sys_perf_refcount_exit)
- ret = register_trace_sys_exit(perf_syscall_exit, NULL);
+ if (!sys_perf_refcount_exit) {
+ ret = register_trace_prio_flags_sys_exit(perf_syscall_exit, NULL,
+ TRACEPOINT_DEFAULT_PRIO,
+ TRACEPOINT_MAY_FAULT);
+ }
if (ret) {
pr_info("event trace: Could not activate syscall exit trace point");
} else {
--
2.39.2
^ permalink raw reply [flat|nested] 11+ messages in thread