[v13,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
Hi Vincent,
kernel test robot noticed the following build errors:
[auto build test ERROR on 29142dc92c37d3259a33aef15b03e6ee25b0d188]
url: https://github.com/intel-lab-lkp/linux/commits/Vincent-Donnefort/ring-buffer-Zero-ring-buffer-sub-buffers/20240129-223025
base: 29142dc92c37d3259a33aef15b03e6ee25b0d188
patch link: https://lore.kernel.org/r/20240129142802.2145305-4-vdonnefort%40google.com
patch subject: [PATCH v13 3/6] tracing: Add snapshot refcount
config: arc-randconfig-002-20240130 (https://download.01.org/0day-ci/archive/20240130/202401301740.qzZlpcYV-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240130/202401301740.qzZlpcYV-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/202401301740.qzZlpcYV-lkp@intel.com/
All errors (new ones prefixed by >>):
kernel/trace/trace.c: In function 'tracing_set_tracer':
kernel/trace/trace.c:6644:17: error: implicit declaration of function 'tracing_disarm_snapshot_locked'; did you mean 'tracing_disarm_snapshot'? [-Werror=implicit-function-declaration]
6644 | tracing_disarm_snapshot_locked(tr);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| tracing_disarm_snapshot
>> kernel/trace/trace.c:6648:23: error: implicit declaration of function 'tracing_arm_snapshot_locked'; did you mean 'tracing_arm_snapshot'? [-Werror=implicit-function-declaration]
6648 | ret = tracing_arm_snapshot_locked(tr);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
| tracing_arm_snapshot
cc1: some warnings being treated as errors
vim +6648 kernel/trace/trace.c
6560
6561 int tracing_set_tracer(struct trace_array *tr, const char *buf)
6562 {
6563 struct tracer *t;
6564 #ifdef CONFIG_TRACER_MAX_TRACE
6565 bool had_max_tr;
6566 #endif
6567 int ret = 0;
6568
6569 mutex_lock(&trace_types_lock);
6570
6571 if (!tr->ring_buffer_expanded) {
6572 ret = __tracing_resize_ring_buffer(tr, trace_buf_size,
6573 RING_BUFFER_ALL_CPUS);
6574 if (ret < 0)
6575 goto out;
6576 ret = 0;
6577 }
6578
6579 for (t = trace_types; t; t = t->next) {
6580 if (strcmp(t->name, buf) == 0)
6581 break;
6582 }
6583 if (!t) {
6584 ret = -EINVAL;
6585 goto out;
6586 }
6587 if (t == tr->current_trace)
6588 goto out;
6589
6590 #ifdef CONFIG_TRACER_SNAPSHOT
6591 if (t->use_max_tr) {
6592 local_irq_disable();
6593 arch_spin_lock(&tr->max_lock);
6594 if (tr->cond_snapshot)
6595 ret = -EBUSY;
6596 arch_spin_unlock(&tr->max_lock);
6597 local_irq_enable();
6598 if (ret)
6599 goto out;
6600 }
6601 #endif
6602 /* Some tracers won't work on kernel command line */
6603 if (system_state < SYSTEM_RUNNING && t->noboot) {
6604 pr_warn("Tracer '%s' is not allowed on command line, ignored\n",
6605 t->name);
6606 goto out;
6607 }
6608
6609 /* Some tracers are only allowed for the top level buffer */
6610 if (!trace_ok_for_array(t, tr)) {
6611 ret = -EINVAL;
6612 goto out;
6613 }
6614
6615 /* If trace pipe files are being read, we can't change the tracer */
6616 if (tr->trace_ref) {
6617 ret = -EBUSY;
6618 goto out;
6619 }
6620
6621 trace_branch_disable();
6622
6623 tr->current_trace->enabled--;
6624
6625 if (tr->current_trace->reset)
6626 tr->current_trace->reset(tr);
6627
6628 #ifdef CONFIG_TRACER_MAX_TRACE
6629 had_max_tr = tr->current_trace->use_max_tr;
6630
6631 /* Current trace needs to be nop_trace before synchronize_rcu */
6632 tr->current_trace = &nop_trace;
6633
6634 if (had_max_tr && !t->use_max_tr) {
6635 /*
6636 * We need to make sure that the update_max_tr sees that
6637 * current_trace changed to nop_trace to keep it from
6638 * swapping the buffers after we resize it.
6639 * The update_max_tr is called from interrupts disabled
6640 * so a synchronized_sched() is sufficient.
6641 */
6642 synchronize_rcu();
6643 free_snapshot(tr);
6644 tracing_disarm_snapshot_locked(tr);
6645 }
6646
6647 if (t->use_max_tr) {
> 6648 ret = tracing_arm_snapshot_locked(tr);
6649 if (ret)
6650 goto out;
6651 }
6652 #else
6653 tr->current_trace = &nop_trace;
6654 #endif
6655
6656 if (t->init) {
6657 ret = tracer_init(t, tr);
6658 if (ret) {
6659 #ifdef CONFIG_TRACER_MAX_TRACE
6660 if (t->use_max_tr)
6661 tracing_disarm_snapshot_locked(tr);
6662 #endif
6663 goto out;
6664 }
6665 }
6666
6667 tr->current_trace = t;
6668 tr->current_trace->enabled++;
6669 trace_branch_enable(tr);
6670 out:
6671 mutex_unlock(&trace_types_lock);
6672
6673 return ret;
6674 }
6675
On Tue, Jan 30, 2024 at 05:30:38PM +0800, kernel test robot wrote:
> Hi Vincent,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on 29142dc92c37d3259a33aef15b03e6ee25b0d188]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Vincent-Donnefort/ring-buffer-Zero-ring-buffer-sub-buffers/20240129-223025
> base: 29142dc92c37d3259a33aef15b03e6ee25b0d188
> patch link: https://lore.kernel.org/r/20240129142802.2145305-4-vdonnefort%40google.com
> patch subject: [PATCH v13 3/6] tracing: Add snapshot refcount
> config: arc-randconfig-002-20240130 (https://download.01.org/0day-ci/archive/20240130/202401301740.qzZlpcYV-lkp@intel.com/config)
> compiler: arceb-elf-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240130/202401301740.qzZlpcYV-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/202401301740.qzZlpcYV-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
> kernel/trace/trace.c: In function 'tracing_set_tracer':
> kernel/trace/trace.c:6644:17: error: implicit declaration of function 'tracing_disarm_snapshot_locked'; did you mean 'tracing_disarm_snapshot'? [-Werror=implicit-function-declaration]
> 6644 | tracing_disarm_snapshot_locked(tr);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | tracing_disarm_snapshot
> >> kernel/trace/trace.c:6648:23: error: implicit declaration of function 'tracing_arm_snapshot_locked'; did you mean 'tracing_arm_snapshot'? [-Werror=implicit-function-declaration]
> 6648 | ret = tracing_arm_snapshot_locked(tr);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> | tracing_arm_snapshot
> cc1: some warnings being treated as errors
Right, two tracers (hwlat and osnoise) select _only_ MAX_TRACE and not
TRACER_SNAPSHOT.
However, AFAICT, they will not call any of the swapping functions (they don't
set use_max_tr). So I suppose arm/disarm can be ommited in that case.
>
>
> vim +6648 kernel/trace/trace.c
>
> 6560
> 6561 int tracing_set_tracer(struct trace_array *tr, const char *buf)
> 6562 {
> 6563 struct tracer *t;
> 6564 #ifdef CONFIG_TRACER_MAX_TRACE
> 6565 bool had_max_tr;
> 6566 #endif
> 6567 int ret = 0;
> 6568
> 6569 mutex_lock(&trace_types_lock);
> 6570
> 6571 if (!tr->ring_buffer_expanded) {
> 6572 ret = __tracing_resize_ring_buffer(tr, trace_buf_size,
> 6573 RING_BUFFER_ALL_CPUS);
> 6574 if (ret < 0)
> 6575 goto out;
> 6576 ret = 0;
> 6577 }
> 6578
> 6579 for (t = trace_types; t; t = t->next) {
> 6580 if (strcmp(t->name, buf) == 0)
> 6581 break;
> 6582 }
> 6583 if (!t) {
> 6584 ret = -EINVAL;
> 6585 goto out;
> 6586 }
> 6587 if (t == tr->current_trace)
> 6588 goto out;
> 6589
> 6590 #ifdef CONFIG_TRACER_SNAPSHOT
> 6591 if (t->use_max_tr) {
> 6592 local_irq_disable();
> 6593 arch_spin_lock(&tr->max_lock);
> 6594 if (tr->cond_snapshot)
> 6595 ret = -EBUSY;
> 6596 arch_spin_unlock(&tr->max_lock);
> 6597 local_irq_enable();
> 6598 if (ret)
> 6599 goto out;
> 6600 }
> 6601 #endif
> 6602 /* Some tracers won't work on kernel command line */
> 6603 if (system_state < SYSTEM_RUNNING && t->noboot) {
> 6604 pr_warn("Tracer '%s' is not allowed on command line, ignored\n",
> 6605 t->name);
> 6606 goto out;
> 6607 }
> 6608
> 6609 /* Some tracers are only allowed for the top level buffer */
> 6610 if (!trace_ok_for_array(t, tr)) {
> 6611 ret = -EINVAL;
> 6612 goto out;
> 6613 }
> 6614
> 6615 /* If trace pipe files are being read, we can't change the tracer */
> 6616 if (tr->trace_ref) {
> 6617 ret = -EBUSY;
> 6618 goto out;
> 6619 }
> 6620
> 6621 trace_branch_disable();
> 6622
> 6623 tr->current_trace->enabled--;
> 6624
> 6625 if (tr->current_trace->reset)
> 6626 tr->current_trace->reset(tr);
> 6627
> 6628 #ifdef CONFIG_TRACER_MAX_TRACE
> 6629 had_max_tr = tr->current_trace->use_max_tr;
> 6630
> 6631 /* Current trace needs to be nop_trace before synchronize_rcu */
> 6632 tr->current_trace = &nop_trace;
> 6633
> 6634 if (had_max_tr && !t->use_max_tr) {
> 6635 /*
> 6636 * We need to make sure that the update_max_tr sees that
> 6637 * current_trace changed to nop_trace to keep it from
> 6638 * swapping the buffers after we resize it.
> 6639 * The update_max_tr is called from interrupts disabled
> 6640 * so a synchronized_sched() is sufficient.
> 6641 */
> 6642 synchronize_rcu();
> 6643 free_snapshot(tr);
> 6644 tracing_disarm_snapshot_locked(tr);
> 6645 }
> 6646
> 6647 if (t->use_max_tr) {
> > 6648 ret = tracing_arm_snapshot_locked(tr);
> 6649 if (ret)
> 6650 goto out;
> 6651 }
> 6652 #else
> 6653 tr->current_trace = &nop_trace;
> 6654 #endif
> 6655
> 6656 if (t->init) {
> 6657 ret = tracer_init(t, tr);
> 6658 if (ret) {
> 6659 #ifdef CONFIG_TRACER_MAX_TRACE
> 6660 if (t->use_max_tr)
> 6661 tracing_disarm_snapshot_locked(tr);
> 6662 #endif
> 6663 goto out;
> 6664 }
> 6665 }
> 6666
> 6667 tr->current_trace = t;
> 6668 tr->current_trace->enabled++;
> 6669 trace_branch_enable(tr);
> 6670 out:
> 6671 mutex_unlock(&trace_types_lock);
> 6672
> 6673 return ret;
> 6674 }
> 6675
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
On Tue, 30 Jan 2024 10:32:45 +0000
Vincent Donnefort <vdonnefort@google.com> wrote:
> > All errors (new ones prefixed by >>):
> >
> > kernel/trace/trace.c: In function 'tracing_set_tracer':
> > kernel/trace/trace.c:6644:17: error: implicit declaration of function 'tracing_disarm_snapshot_locked'; did you mean 'tracing_disarm_snapshot'? [-Werror=implicit-function-declaration]
> > 6644 | tracing_disarm_snapshot_locked(tr);
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > | tracing_disarm_snapshot
> > >> kernel/trace/trace.c:6648:23: error: implicit declaration of function 'tracing_arm_snapshot_locked'; did you mean 'tracing_arm_snapshot'? [-Werror=implicit-function-declaration]
> > 6648 | ret = tracing_arm_snapshot_locked(tr);
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> > | tracing_arm_snapshot
> > cc1: some warnings being treated as errors
>
> Right, two tracers (hwlat and osnoise) select _only_ MAX_TRACE and not
> TRACER_SNAPSHOT.
>
> However, AFAICT, they will not call any of the swapping functions (they don't
> set use_max_tr). So I suppose arm/disarm can be ommited in that case.
Yeah, if you can test with the various configs enabled and disabled to
make sure that it still builds properly, then that should be good.
I should make sure that my own ktest config that I use to run tests
checks these variations too.
-- Steve
@@ -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);
@@ -6593,11 +6641,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
@@ -6606,8 +6655,13 @@ int tracing_set_tracer(struct trace_array *tr, const char *buf)
if (t->init) {
ret = tracer_init(t, tr);
- if (ret)
+ if (ret) {
+#ifdef CONFIG_TRACER_MAX_TRACE
+ if (t->use_max_tr)
+ tracing_disarm_snapshot_locked(tr);
+#endif
goto out;
+ }
}
tr->current_trace = t;
@@ -7709,10 +7763,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();
@@ -7722,6 +7777,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) {
@@ -8846,8 +8902,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;
@@ -8866,12 +8927,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,12 @@ 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.
+/*
+ * True if the trigger was found and unregistered, else false.
*/
-static void unregister_trigger(char *glob,
- struct event_trigger_data *test,
- struct trace_event_file *file)
+static bool try_unregister_trigger(char *glob,
+ struct event_trigger_data *test,
+ struct trace_event_file *file)
{
struct event_trigger_data *data = NULL, *iter;
@@ -626,8 +618,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)
+{
+ try_unregister_trigger(glob, test, file);
}
/*
@@ -1470,7 +1486,7 @@ register_snapshot_trigger(char *glob,
struct event_trigger_data *data,
struct trace_event_file *file)
{
- int ret = tracing_alloc_snapshot_instance(file->tr);
+ int ret = tracing_arm_snapshot(file->tr);
if (ret < 0)
return ret;
@@ -1478,6 +1494,14 @@ register_snapshot_trigger(char *glob,
return register_trigger(glob, data, file);
}
+static void unregister_snapshot_trigger(char *glob,
+ struct event_trigger_data *data,
+ struct trace_event_file *file)
+{
+ if (try_unregister_trigger(glob, data, file))
+ tracing_disarm_snapshot(file->tr);
+}
+
static int
snapshot_trigger_print(struct seq_file *m, struct event_trigger_data *data)
{
@@ -1510,7 +1534,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,
};