Introducing trace buffer mapping by user-space

index 000000000000..5468afc94be7

include/uapi/linux/trace_mmap.h new file +13 -0

@@ -0,0 +1,44 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _TRACE_MMAP_H_
+#define _TRACE_MMAP_H_
+
+#include <linux/types.h>
+
+/**
+ * struct trace_buffer_meta - Ring-buffer Meta-page description
+ * @meta_page_size:	Size of this meta-page.
+ * @meta_struct_len:	Size of this structure.
+ * @subbuf_size:	Size of each subbuf, including the header.
+ * @nr_subbufs:		Number of subbfs in the ring-buffer.
+ * @reader.lost_events:	Number of events lost at the time of the reader swap.

Quoted 22 lines · On Tue, 23 Jan 2024 11:07:53 +0000 Vincent Donnefort <[email protected]>…

index 000000000000..5468afc94be7

include/uapi/linux/trace_mmap.h new file +18 -0

@@ -0,0 +1,44 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _TRACE_MMAP_H_
+#define _TRACE_MMAP_H_
+
+#include <linux/types.h>
+
+/**
+ * struct trace_buffer_meta - Ring-buffer Meta-page description
+ * @meta_page_size:	Size of this meta-page.
+ * @meta_struct_len:	Size of this structure.
+ * @subbuf_size:	Size of each subbuf, including the header.
+ * @nr_subbufs:		Number of subbfs in the ring-buffer.
+ * @reader.lost_events:	Number of events lost at the time of the reader swap.
+ * @reader.id:		subbuf ID of the current reader. From 0 to @nr_subbufs - 1
+ * @reader.read:	Number of bytes read on the reader subbuf.
+ * @entries:		Number of entries in the ring-buffer.
+ * @overrun:		Number of entries lost in the ring-buffer.
+ * @read:		Number of entries that have been read.

include/uapi/linux/trace_mmap.h line 21

Quoted 3 lines

include/uapi/linux/trace_mmap.h new file +3 -0

+ * @subbufs_touched:	Number of subbufs that have been filled.
+ * @subbufs_lost:	Number of subbufs lost to overrun.
+ * @subbufs_read:	Number of subbufs that have been read.

Do we actually need the above 3 fields?

What's the use case for them? I don't want to expose internals in the API unless they are needed.

-- Steve

Quoted 8 lines

Quoted 3 lines

+ * @subbufs_touched: Number of subbufs that have been filled.
+ * @subbufs_lost: Number of subbufs lost to overrun.
+ * @subbufs_read: Number of subbufs that have been read.

Do we actually need the above 3 fields?

What's the use case for them? I don't want to expose internals in the API unless they are needed.

subbufs_read is gone, I just forgot to remove it here :-\.

The two other ones are used for tracing with the hypervisor. That's why I preemptively added them.

I can remove them and add just append this struct later ... or just overload this struct with another one, only shared between the kernel and the hypervisor?

kernel/trace/trace.c +1 -0

 			goto out;
+		}
 	}
 
 	tr->current_trace = t;

kernel/trace/trace_events_trigger.c +0 -4

index 46439e3bcec4..d41bf64741e2 100644
@@ -597,20 +597,9 @@ static int register_trigger(char *glob,
 	return ret;
 }
 
-/**
- * unregister_trigger - Generic event_command @unreg implementation
- * @glob: The raw string used to register the trigger
- * @test: Trigger-specific data used to find the trigger to remove

kernel/trace/trace_events_trigger.c +29 -2

+static bool __unregister_trigger(char *glob,
+				 struct event_trigger_data *test,
+				 struct trace_event_file *file)
 {
 	struct event_trigger_data *data = NULL, *iter;
 

@@ -626,8 +615,32 @@ static void unregister_trigger(char *glob,
 		}
 	}
 
-	if (data && data->ops->free)
-		data->ops->free(data);
+	if (data) {
+		if (data->ops->free)
+			data->ops->free(data);
+
+		return true;
+	}
+
+	return false;
+}
+
+/**
+ * unregister_trigger - Generic event_command @unreg implementation
+ * @glob: The raw string used to register the trigger
+ * @test: Trigger-specific data used to find the trigger to remove
+ * @file: The trace_event_file associated with the event
+ *
+ * Common implementation for event trigger unregistration.
+ *
+ * Usually used directly as the @unreg method in event command
+ * implementations.
+ */
+static void unregister_trigger(char *glob,
+			       struct event_trigger_data *test,
+			       struct trace_event_file *file)
+{
+	__unregister_trigger(glob, test, file);
 }
 
 /*

kernel/trace/trace_events_trigger.c +9 -1

 	return register_trigger(glob, data, file);
 }
 
+static void unregister_snapshot_trigger(char *glob,
+					struct event_trigger_data *data,
+					struct trace_event_file *file)
+{
+	if (__unregister_trigger(glob, data, file))
+		tracing_disarm_snapshot(file->tr);
+}
+
 static int
 snapshot_trigger_print(struct seq_file *m, struct event_trigger_data *data)
 {

@@ -1508,7 +1529,7 @@ static struct event_command trigger_snapshot_cmd = {
 	.trigger_type		= ETT_SNAPSHOT,
 	.parse			= event_trigger_parse,
 	.reg			= register_snapshot_trigger,
-	.unreg			= unregister_trigger,
+	.unreg			= unregister_snapshot_trigger,
 	.get_trigger_ops	= snapshot_get_trigger_ops,
 	.set_filter		= set_trigger_filter,
 };

Signature · 1 line

2.43.0.429.g432eaa2c6b-goog

On Tue, 23 Jan 2024 11:07:54 +0000
Vincent Donnefort <[email protected]> wrote:

[...]

Quoted 2 lines

(quoted diff) +0 -0

@@ -6592,8 +6641,11 @@ int tracing_set_tracer(struct trace_array *tr, const char *buf)
 

kernel/trace/trace.c int tracing_set_tracer(struct trace_array *tr, const char *buf) · line 6646

Quoted 6 lines

kernel/trace/trace.c +3 -1

 	if (t->init) {
 		ret = tracer_init(t, tr);
-		if (ret)
+		if (ret) {
+			if (t->use_max_tr)
+				tracing_disarm_snapshot_locked(tr);

This part is out of CONFIG_TRACER_MAX_TRACE, so it may cause a compile error if CONFIG_TRACER_MAX_TRACE is not set.

Quoted 6 lines

kernel/trace/trace.c +1 -0

 			goto out;
+		}
 	}
 
 	tr->current_trace = t;

[...]

Quoted 17 lines

kernel/trace/trace_events_trigger.c +0 -9

index 46439e3bcec4..d41bf64741e2 100644
@@ -597,20 +597,9 @@ static int register_trigger(char *glob,
 	return ret;
 }
 
-/**
- * unregister_trigger - Generic event_command @unreg implementation
- * @glob: The raw string used to register the trigger
- * @test: Trigger-specific data used to find the trigger to remove
- * @file: The trace_event_file associated with the event
- *
- * Common implementation for event trigger unregistration.
- *
- * Usually used directly as the @unreg method in event command

kernel/trace/trace_events_trigger.c static int register_trigger(char *glob, · line 613

Quoted 5 lines

kernel/trace/trace_events_trigger.c +0 -5

- * implementations.
- */
-static void unregister_trigger(char *glob,
-			       struct event_trigger_data *test,
-			       struct trace_event_file *file)

OK, so __unregister_trigger returns true if data exists, but unregister_trigger() ignores results. (I want some comment here)

Quoted 41 lines

kernel/trace/trace_events_trigger.c +29 -2

+static bool __unregister_trigger(char *glob,
+				 struct event_trigger_data *test,
+				 struct trace_event_file *file)
 {
 	struct event_trigger_data *data = NULL, *iter;
 

@@ -626,8 +615,32 @@ static void unregister_trigger(char *glob,
 		}
 	}
 
-	if (data && data->ops->free)
-		data->ops->free(data);
+	if (data) {
+		if (data->ops->free)
+			data->ops->free(data);
+
+		return true;
+	}
+
+	return false;
+}
+
+/**
+ * unregister_trigger - Generic event_command @unreg implementation
+ * @glob: The raw string used to register the trigger
+ * @test: Trigger-specific data used to find the trigger to remove
+ * @file: The trace_event_file associated with the event
+ *
+ * Common implementation for event trigger unregistration.
+ *
+ * Usually used directly as the @unreg method in event command
+ * implementations.
+ */
+static void unregister_trigger(char *glob,
+			       struct event_trigger_data *test,
+			       struct trace_event_file *file)
+{
+	__unregister_trigger(glob, test, file);
 }
 

kernel/trace/trace_events_trigger.c register_snapshot_trigger(char *glob, · line 1487

Quoted 8 lines

kernel/trace/trace_events_trigger.c +1 -1

 /*

@@ -1470,12 +1483,20 @@ register_snapshot_trigger(char *glob,
 			  struct event_trigger_data *data,
 			  struct trace_event_file *file)
 {
-	if (tracing_alloc_snapshot_instance(file->tr) != 0)
+	if (tracing_arm_snapshot(file->tr))
 		return 0;

BTW, is this return value correct? It seems that the register_*_trigger() will return error code when it fails.

Thanks,

Quoted 27 lines

kernel/trace/trace_events_trigger.c +9 -1

 	return register_trigger(glob, data, file);
 }
 
+static void unregister_snapshot_trigger(char *glob,
+					struct event_trigger_data *data,
+					struct trace_event_file *file)
+{
+	if (__unregister_trigger(glob, data, file))
+		tracing_disarm_snapshot(file->tr);
+}
+
 static int
 snapshot_trigger_print(struct seq_file *m, struct event_trigger_data *data)
 {

@@ -1508,7 +1529,7 @@ static struct event_command trigger_snapshot_cmd = {
 	.trigger_type		= ETT_SNAPSHOT,
 	.parse			= event_trigger_parse,
 	.reg			= register_snapshot_trigger,
-	.unreg			= unregister_trigger,
+	.unreg			= unregister_snapshot_trigger,
 	.get_trigger_ops	= snapshot_get_trigger_ops,
 	.set_filter		= set_trigger_filter,
 };

Signature · 1 line

2.43.0.429.g432eaa2c6b-goog

Signature · 1 line

Masami Hiramatsu (Google) <[email protected]>

Quoted 10 lines

kernel/trace/trace_events_trigger.c register_snapshot_trigger(char *glob, · line 1487

Quoted 7 lines

kernel/trace/trace_events_trigger.c +1 -1

@@ -1470,12 +1483,20 @@ register_snapshot_trigger(char *glob,
 			  struct event_trigger_data *data,
 			  struct trace_event_file *file)
 {
-	if (tracing_alloc_snapshot_instance(file->tr) != 0)
+	if (tracing_arm_snapshot(file->tr))
 		return 0;

BTW, is this return value correct? It seems that the register_*_trigger() will return error code when it fails.

It should indeed be

ret = tracing_arm_snapshot()
if (ret)

     return ret;

From: Masami Hiramatsu (Google) <[email protected]>

Fix register_snapshot_trigger() to return error code if it failed to allocate a snapshot instead of 0 (success). Unless that, it will register snapshot trigger without an error.

Fixes 0bbe7f719985 ("tracing: Fix the race between registering 'snapshot' event trigger and triggering 'snapshot' operation")

Cc [email protected]

Signed-off-by Masami Hiramatsu (Google) <[email protected]>

---
 kernel/trace/trace_events_trigger.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

kernel/trace/trace_events_trigger.c +4 -2

index 46439e3bcec4..b33c3861fbbb 100644
@@ -1470,8 +1470,10 @@ register_snapshot_trigger(char *glob,
 			  struct event_trigger_data *data,
 			  struct trace_event_file *file)
 {
-	if (tracing_alloc_snapshot_instance(file->tr) != 0)
-		return 0;
+	int ret = tracing_alloc_snapshot_instance(file->tr);
+
+	if (ret < 0)
+		return ret;
 
 	return register_trigger(glob, data, file);
 }