[v12,3/6] tracing: Add snapshot refcount
Commit Message
When a ring-buffer is memory mapped by user-space, no trace or
ring-buffer swap is possible. This means the snapshot feature is
mutually exclusive with the memory mapping. Having a refcount on
snapshot users will help to know if a mapping is possible or not.
Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
Comments
On Tue, 23 Jan 2024 11:07:54 +0000
Vincent Donnefort <vdonnefort@google.com> wrote:
[...]
> @@ -6592,8 +6641,11 @@ int tracing_set_tracer(struct trace_array *tr, const char *buf)
>
> 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.
> goto out;
> + }
> }
>
> tr->current_trace = t;
[...]
> diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
> index 46439e3bcec4..d41bf64741e2 100644
> --- a/kernel/trace/trace_events_trigger.c
> +++ b/kernel/trace/trace_events_trigger.c
> @@ -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
> - * 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)
> +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);
> }
>
> /*
> @@ -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,
>
> 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,
> };
> --
> 2.43.0.429.g432eaa2c6b-goog
>
Hi Masami,
Thanks for taking the time to look at those changes.
On Thu, Jan 25, 2024 at 12:11:49AM +0900, Masami Hiramatsu wrote:
> On Tue, 23 Jan 2024 11:07:54 +0000
> Vincent Donnefort <vdonnefort@google.com> wrote:
>
> [...]
> > @@ -6592,8 +6641,11 @@ int tracing_set_tracer(struct trace_array *tr, const char *buf)
> >
> > 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.
Duh, yes it must depends on TRACER_MAX_TRACE :-\
>
> > goto out;
> > + }
> > }
> >
> > tr->current_trace = t;
> [...]
> > diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
> > index 46439e3bcec4..d41bf64741e2 100644
> > --- a/kernel/trace/trace_events_trigger.c
> > +++ b/kernel/trace/trace_events_trigger.c
> > @@ -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
> > - * 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)
Will add something for the __unregister_trigger flavour.
>
> > +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);
> > }
> >
> > /*
> > @@ -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;
>
> Thanks,
>
> >
> > 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,
> > };
> > --
> > 2.43.0.429.g432eaa2c6b-goog
> >
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>
Hi Vincent,
On Thu, 25 Jan 2024 14:53:40 +0000
Vincent Donnefort <vdonnefort@google.com> wrote:
> > > @@ -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;
OK, then there is a bug. We need to fix it before changing this
for backporting.
Thank you,
Hi Vincent,
kernel test robot noticed the following build errors:
[auto build test ERROR on 4f1991a92cfe89096b2d1f5583a2e093bdd55c37]
url: https://github.com/intel-lab-lkp/linux/commits/Vincent-Donnefort/ring-buffer-Zero-ring-buffer-sub-buffers/20240123-191131
base: 4f1991a92cfe89096b2d1f5583a2e093bdd55c37
patch link: https://lore.kernel.org/r/20240123110757.3657908-4-vdonnefort%40google.com
patch subject: [PATCH v12 3/6] tracing: Add snapshot refcount
config: i386-buildonly-randconfig-005-20240126 (https://download.01.org/0day-ci/archive/20240126/202401260918.sn23MDek-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240126/202401260918.sn23MDek-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401260918.sn23MDek-lkp@intel.com/
All errors (new ones prefixed by >>):
>> kernel/trace/trace.c:6645:11: error: no member named 'use_max_tr' in 'struct tracer'
6645 | if (t->use_max_tr)
| ~ ^
>> kernel/trace/trace.c:6646:5: error: call to undeclared function 'tracing_disarm_snapshot_locked'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
6646 | tracing_disarm_snapshot_locked(tr);
| ^
kernel/trace/trace.c:6646:5: note: did you mean 'tracing_disarm_snapshot'?
kernel/trace/trace.h:1986:20: note: 'tracing_disarm_snapshot' declared here
1986 | static inline void tracing_disarm_snapshot(struct trace_array *tr) { }
| ^
2 errors generated.
vim +6645 kernel/trace/trace.c
6641
6642 if (t->init) {
6643 ret = tracer_init(t, tr);
6644 if (ret) {
> 6645 if (t->use_max_tr)
> 6646 tracing_disarm_snapshot_locked(tr);
6647 goto out;
6648 }
6649 }
6650
6651 tr->current_trace = t;
6652 tr->current_trace->enabled++;
6653 trace_branch_enable(tr);
6654 out:
6655 mutex_unlock(&trace_types_lock);
6656
6657 return ret;
6658 }
6659
Hi Vincent,
kernel test robot noticed the following build errors:
[auto build test ERROR on 4f1991a92cfe89096b2d1f5583a2e093bdd55c37]
url: https://github.com/intel-lab-lkp/linux/commits/Vincent-Donnefort/ring-buffer-Zero-ring-buffer-sub-buffers/20240123-191131
base: 4f1991a92cfe89096b2d1f5583a2e093bdd55c37
patch link: https://lore.kernel.org/r/20240123110757.3657908-4-vdonnefort%40google.com
patch subject: [PATCH v12 3/6] tracing: Add snapshot refcount
config: i386-randconfig-012-20240126 (https://download.01.org/0day-ci/archive/20240126/202401261026.Cc8uBwxf-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240126/202401261026.Cc8uBwxf-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401261026.Cc8uBwxf-lkp@intel.com/
All errors (new ones prefixed by >>):
kernel/trace/trace.c: In function 'tracing_set_tracer':
>> kernel/trace/trace.c:6645:9: error: 'struct tracer' has no member named 'use_max_tr'
6645 | if (t->use_max_tr)
| ^~
>> kernel/trace/trace.c:6646:5: error: implicit declaration of function 'tracing_disarm_snapshot_locked'; did you mean 'tracing_disarm_snapshot'? [-Werror=implicit-function-declaration]
6646 | tracing_disarm_snapshot_locked(tr);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| tracing_disarm_snapshot
cc1: some warnings being treated as errors
vim +6645 kernel/trace/trace.c
6641
6642 if (t->init) {
6643 ret = tracer_init(t, tr);
6644 if (ret) {
> 6645 if (t->use_max_tr)
> 6646 tracing_disarm_snapshot_locked(tr);
6647 goto out;
6648 }
6649 }
6650
6651 tr->current_trace = t;
6652 tr->current_trace->enabled++;
6653 trace_branch_enable(tr);
6654 out:
6655 mutex_unlock(&trace_types_lock);
6656
6657 return ret;
6658 }
6659
@@ -1300,6 +1300,52 @@ static void free_snapshot(struct trace_array *tr)
tr->allocated_snapshot = false;
}
+static int tracing_arm_snapshot_locked(struct trace_array *tr)
+{
+ int ret;
+
+ lockdep_assert_held(&trace_types_lock);
+
+ if (tr->snapshot == UINT_MAX)
+ return -EBUSY;
+
+ ret = tracing_alloc_snapshot_instance(tr);
+ if (ret)
+ return ret;
+
+ tr->snapshot++;
+
+ return 0;
+}
+
+static void tracing_disarm_snapshot_locked(struct trace_array *tr)
+{
+ lockdep_assert_held(&trace_types_lock);
+
+ if (WARN_ON(!tr->snapshot))
+ return;
+
+ tr->snapshot--;
+}
+
+int tracing_arm_snapshot(struct trace_array *tr)
+{
+ int ret;
+
+ mutex_lock(&trace_types_lock);
+ ret = tracing_arm_snapshot_locked(tr);
+ mutex_unlock(&trace_types_lock);
+
+ return ret;
+}
+
+void tracing_disarm_snapshot(struct trace_array *tr)
+{
+ mutex_lock(&trace_types_lock);
+ tracing_disarm_snapshot_locked(tr);
+ mutex_unlock(&trace_types_lock);
+}
+
/**
* tracing_alloc_snapshot - allocate snapshot buffer.
*
@@ -1373,10 +1419,6 @@ int tracing_snapshot_cond_enable(struct trace_array *tr, void *cond_data,
mutex_lock(&trace_types_lock);
- ret = tracing_alloc_snapshot_instance(tr);
- if (ret)
- goto fail_unlock;
-
if (tr->current_trace->use_max_tr) {
ret = -EBUSY;
goto fail_unlock;
@@ -1395,6 +1437,10 @@ int tracing_snapshot_cond_enable(struct trace_array *tr, void *cond_data,
goto fail_unlock;
}
+ ret = tracing_arm_snapshot_locked(tr);
+ if (ret)
+ goto fail_unlock;
+
local_irq_disable();
arch_spin_lock(&tr->max_lock);
tr->cond_snapshot = cond_snapshot;
@@ -1439,6 +1485,8 @@ int tracing_snapshot_cond_disable(struct trace_array *tr)
arch_spin_unlock(&tr->max_lock);
local_irq_enable();
+ tracing_disarm_snapshot(tr);
+
return ret;
}
EXPORT_SYMBOL_GPL(tracing_snapshot_cond_disable);
@@ -6579,11 +6627,12 @@ int tracing_set_tracer(struct trace_array *tr, const char *buf)
*/
synchronize_rcu();
free_snapshot(tr);
+ tracing_disarm_snapshot_locked(tr);
}
- if (t->use_max_tr && !tr->allocated_snapshot) {
- ret = tracing_alloc_snapshot_instance(tr);
- if (ret < 0)
+ if (t->use_max_tr) {
+ ret = tracing_arm_snapshot_locked(tr);
+ if (ret)
goto out;
}
#else
@@ -6592,8 +6641,11 @@ int tracing_set_tracer(struct trace_array *tr, const char *buf)
if (t->init) {
ret = tracer_init(t, tr);
- if (ret)
+ if (ret) {
+ if (t->use_max_tr)
+ tracing_disarm_snapshot_locked(tr);
goto out;
+ }
}
tr->current_trace = t;
@@ -7695,10 +7747,11 @@ tracing_snapshot_write(struct file *filp, const char __user *ubuf, size_t cnt,
if (tr->allocated_snapshot)
ret = resize_buffer_duplicate_size(&tr->max_buffer,
&tr->array_buffer, iter->cpu_file);
- else
- ret = tracing_alloc_snapshot_instance(tr);
- if (ret < 0)
+
+ ret = tracing_arm_snapshot_locked(tr);
+ if (ret)
break;
+
/* Now, we're going to swap */
if (iter->cpu_file == RING_BUFFER_ALL_CPUS) {
local_irq_disable();
@@ -7708,6 +7761,7 @@ tracing_snapshot_write(struct file *filp, const char __user *ubuf, size_t cnt,
smp_call_function_single(iter->cpu_file, tracing_swap_cpu_buffer,
(void *)tr, 1);
}
+ tracing_disarm_snapshot_locked(tr);
break;
default:
if (tr->allocated_snapshot) {
@@ -8832,8 +8886,13 @@ ftrace_trace_snapshot_callback(struct trace_array *tr, struct ftrace_hash *hash,
ops = param ? &snapshot_count_probe_ops : &snapshot_probe_ops;
- if (glob[0] == '!')
- return unregister_ftrace_function_probe_func(glob+1, tr, ops);
+ if (glob[0] == '!') {
+ ret = unregister_ftrace_function_probe_func(glob+1, tr, ops);
+ if (!ret)
+ tracing_disarm_snapshot(tr);
+
+ return ret;
+ }
if (!param)
goto out_reg;
@@ -8852,12 +8911,13 @@ ftrace_trace_snapshot_callback(struct trace_array *tr, struct ftrace_hash *hash,
return ret;
out_reg:
- ret = tracing_alloc_snapshot_instance(tr);
+ ret = tracing_arm_snapshot(tr);
if (ret < 0)
goto out;
ret = register_ftrace_function_probe(glob, tr, ops, count);
-
+ if (ret < 0)
+ tracing_disarm_snapshot(tr);
out:
return ret < 0 ? ret : 0;
}
@@ -334,6 +334,7 @@ struct trace_array {
*/
struct array_buffer max_buffer;
bool allocated_snapshot;
+ unsigned int snapshot;
#endif
#ifdef CONFIG_TRACER_MAX_TRACE
unsigned long max_latency;
@@ -1973,12 +1974,16 @@ static inline void trace_event_eval_update(struct trace_eval_map **map, int len)
#ifdef CONFIG_TRACER_SNAPSHOT
void tracing_snapshot_instance(struct trace_array *tr);
int tracing_alloc_snapshot_instance(struct trace_array *tr);
+int tracing_arm_snapshot(struct trace_array *tr);
+void tracing_disarm_snapshot(struct trace_array *tr);
#else
static inline void tracing_snapshot_instance(struct trace_array *tr) { }
static inline int tracing_alloc_snapshot_instance(struct trace_array *tr)
{
return 0;
}
+static inline int tracing_arm_snapshot(struct trace_array *tr) { return 0; }
+static inline void tracing_disarm_snapshot(struct trace_array *tr) { }
#endif
#ifdef CONFIG_PREEMPT_TRACER
@@ -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
- * implementations.
- */
-static void unregister_trigger(char *glob,
- struct event_trigger_data *test,
- struct trace_event_file *file)
+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);
}
/*
@@ -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;
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,
};