* [for-next][PATCH 01/15] tracing: Add printf attribute to log function
2021-02-03 16:05 [for-next][PATCH 00/15] tracing: Updates for 5.12 Steven Rostedt
@ 2021-02-03 16:05 ` Steven Rostedt
2021-02-03 16:05 ` [for-next][PATCH 02/15] tracing: Update trace_ignore_this_task() kernel-doc comment Steven Rostedt
` (12 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Steven Rostedt @ 2021-02-03 16:05 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Tom Rix
From: Tom Rix <trix@redhat.com>
Attributing the function allows the compiler to more thoroughly
check the use of the function with -Wformat and similar flags.
Link: https://lkml.kernel.org/r/20201221162715.3757291-1-trix@redhat.com
Signed-off-by: Tom Rix <trix@redhat.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
include/linux/trace.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/linux/trace.h b/include/linux/trace.h
index 886a4ffd9d45..be1e130ed87c 100644
--- a/include/linux/trace.h
+++ b/include/linux/trace.h
@@ -34,8 +34,9 @@ int unregister_ftrace_export(struct trace_export *export);
struct trace_array;
void trace_printk_init_buffers(void);
+__printf(3, 4)
int trace_array_printk(struct trace_array *tr, unsigned long ip,
- const char *fmt, ...);
+ const char *fmt, ...);
int trace_array_init_printk(struct trace_array *tr);
void trace_array_put(struct trace_array *tr);
struct trace_array *trace_array_get_by_name(const char *name);
--
2.29.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [for-next][PATCH 02/15] tracing: Update trace_ignore_this_task() kernel-doc comment
2021-02-03 16:05 [for-next][PATCH 00/15] tracing: Updates for 5.12 Steven Rostedt
2021-02-03 16:05 ` [for-next][PATCH 01/15] tracing: Add printf attribute to log function Steven Rostedt
@ 2021-02-03 16:05 ` Steven Rostedt
2021-02-03 16:05 ` [for-next][PATCH 03/15] tracing: Remove get/put_cpu() from function_trace_init Steven Rostedt
` (11 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Steven Rostedt @ 2021-02-03 16:05 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Qiujun Huang
From: Qiujun Huang <hqjagain@gmail.com>
Update kernel-doc parameter after
commit b3b1e6ededa4 ("ftrace: Create set_ftrace_notrace_pid to not trace tasks")
added @filtered_no_pids.
Link: https://lkml.kernel.org/r/20201231153558.4804-1-hqjagain@gmail.com
Signed-off-by: Qiujun Huang <hqjagain@gmail.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
kernel/trace/trace.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index b8a2d786b503..9e4f4043a3df 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -530,6 +530,7 @@ trace_find_filtered_pid(struct trace_pid_list *filtered_pids, pid_t search_pid)
/**
* trace_ignore_this_task - should a task be ignored for tracing
* @filtered_pids: The list of pids to check
+ * @filtered_no_pids: The list of pids not to be traced
* @task: The task that should be ignored if not filtered
*
* Checks if @task should be traced or not from @filtered_pids.
@@ -780,7 +781,7 @@ u64 ftrace_now(int cpu)
}
/**
- * tracing_is_enabled - Show if global_trace has been disabled
+ * tracing_is_enabled - Show if global_trace has been enabled
*
* Shows if the global trace has been enabled or not. It uses the
* mirror flag "buffer_disabled" to be used in fast paths such as for
--
2.29.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [for-next][PATCH 03/15] tracing: Remove get/put_cpu() from function_trace_init
2021-02-03 16:05 [for-next][PATCH 00/15] tracing: Updates for 5.12 Steven Rostedt
2021-02-03 16:05 ` [for-next][PATCH 01/15] tracing: Add printf attribute to log function Steven Rostedt
2021-02-03 16:05 ` [for-next][PATCH 02/15] tracing: Update trace_ignore_this_task() kernel-doc comment Steven Rostedt
@ 2021-02-03 16:05 ` Steven Rostedt
2021-02-03 16:05 ` [for-next][PATCH 04/15] ring-buffer: Remove cpu_buffer argument from the rb_inc_page() Steven Rostedt
` (10 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Steven Rostedt @ 2021-02-03 16:05 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Qiujun Huang
From: Qiujun Huang <hqjagain@gmail.com>
Since commit b6f11df26fdc ("trace: Call tracing_reset_online_cpus before
tracer->init()"), get/put_cpu() are not needed anymore.
We can use raw_smp_processor_id() instead.
Link: https://lkml.kernel.org/r/20201230140521.31920-1-hqjagain@gmail.com
Signed-off-by: Qiujun Huang <hqjagain@gmail.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
kernel/trace/trace_functions.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
index c5095dd28e20..f67aec5bb771 100644
--- a/kernel/trace/trace_functions.c
+++ b/kernel/trace/trace_functions.c
@@ -106,8 +106,7 @@ static int function_trace_init(struct trace_array *tr)
ftrace_init_array_ops(tr, func);
- tr->array_buffer.cpu = get_cpu();
- put_cpu();
+ tr->array_buffer.cpu = raw_smp_processor_id();
tracing_start_cmdline_record();
tracing_start_function_trace(tr);
--
2.29.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [for-next][PATCH 04/15] ring-buffer: Remove cpu_buffer argument from the rb_inc_page()
2021-02-03 16:05 [for-next][PATCH 00/15] tracing: Updates for 5.12 Steven Rostedt
` (2 preceding siblings ...)
2021-02-03 16:05 ` [for-next][PATCH 03/15] tracing: Remove get/put_cpu() from function_trace_init Steven Rostedt
@ 2021-02-03 16:05 ` Steven Rostedt
2021-02-03 16:05 ` [for-next][PATCH 05/15] ring-buffer: Drop unneeded check in ring_buffer_resize() Steven Rostedt
` (9 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Steven Rostedt @ 2021-02-03 16:05 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Qiujun Huang
From: Qiujun Huang <hqjagain@gmail.com>
The cpu_buffer argument is not used inside the rb_inc_page() after
commit 3adc54fa82a6 ("ring-buffer: make the buffer a true circular link
list").
And cpu_buffer argument is not used inside the two functions too,
rb_is_head_page/rb_set_list_to_head.
Link: https://lkml.kernel.org/r/20201225140356.23008-1-hqjagain@gmail.com
Signed-off-by: Qiujun Huang <hqjagain@gmail.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
kernel/trace/ring_buffer.c | 35 ++++++++++++++++-------------------
1 file changed, 16 insertions(+), 19 deletions(-)
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index ec08f948dd80..8fccee76a5f3 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -1112,8 +1112,7 @@ static struct list_head *rb_list_head(struct list_head *list)
* its flags will be non zero.
*/
static inline int
-rb_is_head_page(struct ring_buffer_per_cpu *cpu_buffer,
- struct buffer_page *page, struct list_head *list)
+rb_is_head_page(struct buffer_page *page, struct list_head *list)
{
unsigned long val;
@@ -1142,8 +1141,7 @@ static bool rb_is_reader_page(struct buffer_page *page)
/*
* rb_set_list_to_head - set a list_head to be pointing to head.
*/
-static void rb_set_list_to_head(struct ring_buffer_per_cpu *cpu_buffer,
- struct list_head *list)
+static void rb_set_list_to_head(struct list_head *list)
{
unsigned long *ptr;
@@ -1166,7 +1164,7 @@ static void rb_head_page_activate(struct ring_buffer_per_cpu *cpu_buffer)
/*
* Set the previous list pointer to have the HEAD flag.
*/
- rb_set_list_to_head(cpu_buffer, head->list.prev);
+ rb_set_list_to_head(head->list.prev);
}
static void rb_list_head_clear(struct list_head *list)
@@ -1241,8 +1239,7 @@ static int rb_head_page_set_normal(struct ring_buffer_per_cpu *cpu_buffer,
old_flag, RB_PAGE_NORMAL);
}
-static inline void rb_inc_page(struct ring_buffer_per_cpu *cpu_buffer,
- struct buffer_page **bpage)
+static inline void rb_inc_page(struct buffer_page **bpage)
{
struct list_head *p = rb_list_head((*bpage)->list.next);
@@ -1274,11 +1271,11 @@ rb_set_head_page(struct ring_buffer_per_cpu *cpu_buffer)
*/
for (i = 0; i < 3; i++) {
do {
- if (rb_is_head_page(cpu_buffer, page, page->list.prev)) {
+ if (rb_is_head_page(page, page->list.prev)) {
cpu_buffer->head_page = page;
return page;
}
- rb_inc_page(cpu_buffer, &page);
+ rb_inc_page(&page);
} while (page != head);
}
@@ -1824,7 +1821,7 @@ rb_remove_pages(struct ring_buffer_per_cpu *cpu_buffer, unsigned long nr_pages)
cond_resched();
to_remove_page = tmp_iter_page;
- rb_inc_page(cpu_buffer, &tmp_iter_page);
+ rb_inc_page(&tmp_iter_page);
/* update the counters */
page_entries = rb_page_entries(to_remove_page);
@@ -2271,7 +2268,7 @@ static void rb_inc_iter(struct ring_buffer_iter *iter)
if (iter->head_page == cpu_buffer->reader_page)
iter->head_page = rb_set_head_page(cpu_buffer);
else
- rb_inc_page(cpu_buffer, &iter->head_page);
+ rb_inc_page(&iter->head_page);
iter->page_stamp = iter->read_stamp = iter->head_page->page->time_stamp;
iter->head = 0;
@@ -2374,7 +2371,7 @@ rb_handle_head_page(struct ring_buffer_per_cpu *cpu_buffer,
* want the outer most commit to reset it.
*/
new_head = next_page;
- rb_inc_page(cpu_buffer, &new_head);
+ rb_inc_page(&new_head);
ret = rb_head_page_set_head(cpu_buffer, new_head, next_page,
RB_PAGE_NORMAL);
@@ -2526,7 +2523,7 @@ rb_move_tail(struct ring_buffer_per_cpu *cpu_buffer,
next_page = tail_page;
- rb_inc_page(cpu_buffer, &next_page);
+ rb_inc_page(&next_page);
/*
* If for some reason, we had an interrupt storm that made
@@ -2552,7 +2549,7 @@ rb_move_tail(struct ring_buffer_per_cpu *cpu_buffer,
* the buffer, unless the commit page is still on the
* reader page.
*/
- if (rb_is_head_page(cpu_buffer, next_page, &tail_page->list)) {
+ if (rb_is_head_page(next_page, &tail_page->list)) {
/*
* If the commit is not on the reader page, then
@@ -2879,7 +2876,7 @@ rb_set_commit_to_write(struct ring_buffer_per_cpu *cpu_buffer)
return;
local_set(&cpu_buffer->commit_page->page->commit,
rb_page_write(cpu_buffer->commit_page));
- rb_inc_page(cpu_buffer, &cpu_buffer->commit_page);
+ rb_inc_page(&cpu_buffer->commit_page);
/* add barrier to keep gcc from optimizing too much */
barrier();
}
@@ -3638,14 +3635,14 @@ rb_decrement_entry(struct ring_buffer_per_cpu *cpu_buffer,
* Because the commit page may be on the reader page we
* start with the next page and check the end loop there.
*/
- rb_inc_page(cpu_buffer, &bpage);
+ rb_inc_page(&bpage);
start = bpage;
do {
if (bpage->page == (void *)addr) {
local_dec(&bpage->entries);
return;
}
- rb_inc_page(cpu_buffer, &bpage);
+ rb_inc_page(&bpage);
} while (bpage != start);
/* commit not part of this buffer?? */
@@ -4367,7 +4364,7 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
cpu_buffer->pages = reader->list.prev;
/* The reader page will be pointing to the new head */
- rb_set_list_to_head(cpu_buffer, &cpu_buffer->reader_page->list);
+ rb_set_list_to_head(&cpu_buffer->reader_page->list);
/*
* We want to make sure we read the overruns after we set up our
@@ -4406,7 +4403,7 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
* Now make the new head point back to the reader page.
*/
rb_list_head(reader->list.next)->prev = &cpu_buffer->reader_page->list;
- rb_inc_page(cpu_buffer, &cpu_buffer->head_page);
+ rb_inc_page(&cpu_buffer->head_page);
local_inc(&cpu_buffer->pages_read);
--
2.29.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [for-next][PATCH 05/15] ring-buffer: Drop unneeded check in ring_buffer_resize()
2021-02-03 16:05 [for-next][PATCH 00/15] tracing: Updates for 5.12 Steven Rostedt
` (3 preceding siblings ...)
2021-02-03 16:05 ` [for-next][PATCH 04/15] ring-buffer: Remove cpu_buffer argument from the rb_inc_page() Steven Rostedt
@ 2021-02-03 16:05 ` Steven Rostedt
2021-02-03 16:05 ` [for-next][PATCH 07/15] tracing: Inline tracing_gen_ctx_flags() Steven Rostedt
` (8 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Steven Rostedt @ 2021-02-03 16:05 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Qiujun Huang
From: Qiujun Huang <hqjagain@gmail.com>
Remove the cpumask check, as we has done it at the beginning of
the function.
Also fix a typo. s/also the on the/also on the/
Link: https://lkml.kernel.org/r/20201224144634.3210-1-hqjagain@gmail.com
Signed-off-by: Qiujun Huang <hqjagain@gmail.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
kernel/trace/ring_buffer.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 8fccee76a5f3..b9dad3500041 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -2059,10 +2059,6 @@ int ring_buffer_resize(struct trace_buffer *buffer, unsigned long size,
put_online_cpus();
} else {
- /* Make sure this CPU has been initialized */
- if (!cpumask_test_cpu(cpu_id, buffer->cpumask))
- goto out;
-
cpu_buffer = buffer->buffers[cpu_id];
if (nr_pages == cpu_buffer->nr_pages)
@@ -2580,7 +2576,7 @@ rb_move_tail(struct ring_buffer_per_cpu *cpu_buffer,
* have filled up the buffer with events
* from interrupts and such, and wrapped.
*
- * Note, if the tail page is also the on the
+ * Note, if the tail page is also on the
* reader_page, we let it move out.
*/
if (unlikely((cpu_buffer->commit_page !=
--
2.29.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [for-next][PATCH 07/15] tracing: Inline tracing_gen_ctx_flags()
2021-02-03 16:05 [for-next][PATCH 00/15] tracing: Updates for 5.12 Steven Rostedt
` (4 preceding siblings ...)
2021-02-03 16:05 ` [for-next][PATCH 05/15] ring-buffer: Drop unneeded check in ring_buffer_resize() Steven Rostedt
@ 2021-02-03 16:05 ` Steven Rostedt
2021-02-03 16:05 ` [for-next][PATCH 08/15] tracing: Use in_serving_softirq() to deduct softirq status Steven Rostedt
` (7 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Steven Rostedt @ 2021-02-03 16:05 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Sebastian Andrzej Siewior
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Inline tracing_gen_ctx_flags(). This allows to have one ifdef
CONFIG_TRACE_IRQFLAGS_SUPPORT.
This requires to move `trace_flag_type' so tracing_gen_ctx_flags() can
use it.
Link: https://lkml.kernel.org/r/20210125194511.3924915-3-bigeasy@linutronix.de
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Link: https://lkml.kernel.org/r/20210125140323.6b1ff20c@gandalf.local.home
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
include/linux/trace_events.h | 54 ++++++++++++++++++++++++++++++++++--
kernel/trace/trace.c | 38 ++-----------------------
kernel/trace/trace.h | 19 -------------
3 files changed, 53 insertions(+), 58 deletions(-)
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index 091250b0895a..67ae708de40d 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -160,9 +160,57 @@ static inline void tracing_generic_entry_update(struct trace_entry *entry,
entry->flags = trace_ctx >> 16;
}
-unsigned int tracing_gen_ctx_flags(unsigned long irqflags);
-unsigned int tracing_gen_ctx(void);
-unsigned int tracing_gen_ctx_dec(void);
+unsigned int tracing_gen_ctx_irq_test(unsigned int irqs_status);
+
+enum trace_flag_type {
+ TRACE_FLAG_IRQS_OFF = 0x01,
+ TRACE_FLAG_IRQS_NOSUPPORT = 0x02,
+ TRACE_FLAG_NEED_RESCHED = 0x04,
+ TRACE_FLAG_HARDIRQ = 0x08,
+ TRACE_FLAG_SOFTIRQ = 0x10,
+ TRACE_FLAG_PREEMPT_RESCHED = 0x20,
+ TRACE_FLAG_NMI = 0x40,
+};
+
+#ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
+static inline unsigned int tracing_gen_ctx_flags(unsigned long irqflags)
+{
+ unsigned int irq_status = irqs_disabled_flags(irqflags) ?
+ TRACE_FLAG_IRQS_OFF : 0;
+ return tracing_gen_ctx_irq_test(irq_status);
+}
+static inline unsigned int tracing_gen_ctx(void)
+{
+ unsigned long irqflags;
+
+ local_save_flags(irqflags);
+ return tracing_gen_ctx_flags(irqflags);
+}
+#else
+
+static inline unsigned int tracing_gen_ctx_flags(unsigned long irqflags)
+{
+ return tracing_gen_ctx_irq_test(TRACE_FLAG_IRQS_NOSUPPORT);
+}
+static inline unsigned int tracing_gen_ctx(void)
+{
+ return tracing_gen_ctx_irq_test(TRACE_FLAG_IRQS_NOSUPPORT);
+}
+#endif
+
+static inline unsigned int tracing_gen_ctx_dec(void)
+{
+ unsigned int trace_ctx;
+
+ trace_ctx = tracing_gen_ctx();
+ /*
+ * Subtract one from the preeption counter if preemption is enabled,
+ * see trace_event_buffer_reserve()for details.
+ */
+ if (IS_ENABLED(CONFIG_PREEMPTION))
+ trace_ctx--;
+ return trace_ctx;
+}
struct trace_event_file;
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 0b3cce6ecf52..584fa2a1304a 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2579,20 +2579,13 @@ enum print_line_t trace_handle_return(struct trace_seq *s)
}
EXPORT_SYMBOL_GPL(trace_handle_return);
-unsigned int tracing_gen_ctx_flags(unsigned long irqflags)
+unsigned int tracing_gen_ctx_irq_test(unsigned int irqs_status)
{
- unsigned int trace_flags = 0;
+ unsigned int trace_flags = irqs_status;
unsigned int pc;
pc = preempt_count();
-#ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
- if (irqs_disabled_flags(irqflags))
- trace_flags |= TRACE_FLAG_IRQS_OFF;
-#else
- trace_flags |= TRACE_FLAG_IRQS_NOSUPPORT;
-#endif
-
if (pc & NMI_MASK)
trace_flags |= TRACE_FLAG_NMI;
if (pc & HARDIRQ_MASK)
@@ -2608,33 +2601,6 @@ unsigned int tracing_gen_ctx_flags(unsigned long irqflags)
return (trace_flags << 16) | (pc & 0xff);
}
-unsigned int tracing_gen_ctx(void)
-{
- unsigned long irqflags;
-
-#ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
- local_save_flags(irqflags);
-#else
- irqflags = 0;
-#endif
- return tracing_gen_ctx_flags(irqflags);
-}
-
-unsigned int tracing_gen_ctx_dec(void)
-{
- unsigned int trace_ctx;
-
- trace_ctx = tracing_gen_ctx();
-
- /*
- * Subtract one from the preeption counter if preemption is enabled,
- * see trace_event_buffer_reserve()for details.
- */
- if (IS_ENABLED(CONFIG_PREEMPTION))
- trace_ctx--;
- return trace_ctx;
-}
-
struct ring_buffer_event *
trace_buffer_lock_reserve(struct trace_buffer *buffer,
int type,
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 8daf3a0758b1..93fb08ab8bb6 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -136,25 +136,6 @@ struct kretprobe_trace_entry_head {
unsigned long ret_ip;
};
-/*
- * trace_flag_type is an enumeration that holds different
- * states when a trace occurs. These are:
- * IRQS_OFF - interrupts were disabled
- * IRQS_NOSUPPORT - arch does not support irqs_disabled_flags
- * NEED_RESCHED - reschedule is requested
- * HARDIRQ - inside an interrupt handler
- * SOFTIRQ - inside a softirq handler
- */
-enum trace_flag_type {
- TRACE_FLAG_IRQS_OFF = 0x01,
- TRACE_FLAG_IRQS_NOSUPPORT = 0x02,
- TRACE_FLAG_NEED_RESCHED = 0x04,
- TRACE_FLAG_HARDIRQ = 0x08,
- TRACE_FLAG_SOFTIRQ = 0x10,
- TRACE_FLAG_PREEMPT_RESCHED = 0x20,
- TRACE_FLAG_NMI = 0x40,
-};
-
#define TRACE_BUF_SIZE 1024
struct trace_array;
--
2.29.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [for-next][PATCH 08/15] tracing: Use in_serving_softirq() to deduct softirq status.
2021-02-03 16:05 [for-next][PATCH 00/15] tracing: Updates for 5.12 Steven Rostedt
` (5 preceding siblings ...)
2021-02-03 16:05 ` [for-next][PATCH 07/15] tracing: Inline tracing_gen_ctx_flags() Steven Rostedt
@ 2021-02-03 16:05 ` Steven Rostedt
2021-02-03 16:05 ` [for-next][PATCH 09/15] tracing: Remove NULL check from current in tracing_generic_entry_update() Steven Rostedt
` (6 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Steven Rostedt @ 2021-02-03 16:05 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Sebastian Andrzej Siewior
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
PREEMPT_RT does not report "serving softirq" because the tracing core
looks at the preemption counter while PREEMPT_RT does not update it
while processing softirqs in order to remain preemptible. The
information is stored somewhere else.
The in_serving_softirq() macro and the SOFTIRQ_OFFSET define are still
working but not on the preempt-counter.
Use in_serving_softirq() macro which works on PREEMPT_RT. On !PREEMPT_RT
the compiler (gcc-10 / clang-11) is smart enough to optimize the
in_serving_softirq() related read of the preemption counter away.
The only difference I noticed by using in_serving_softirq() on
!PREEMPT_RT is that gcc-10 implemented tracing_gen_ctx_flags() as
reading FLAG, jmp _tracing_gen_ctx_flags(). Without in_serving_softirq()
it inlined _tracing_gen_ctx_flags() into tracing_gen_ctx_flags().
Link: https://lkml.kernel.org/r/20210125194511.3924915-4-bigeasy@linutronix.de
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
kernel/trace/trace.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 584fa2a1304a..75620c29e904 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2590,8 +2590,7 @@ unsigned int tracing_gen_ctx_irq_test(unsigned int irqs_status)
trace_flags |= TRACE_FLAG_NMI;
if (pc & HARDIRQ_MASK)
trace_flags |= TRACE_FLAG_HARDIRQ;
-
- if (pc & SOFTIRQ_OFFSET)
+ if (in_serving_softirq())
trace_flags |= TRACE_FLAG_SOFTIRQ;
if (tif_need_resched())
--
2.29.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [for-next][PATCH 09/15] tracing: Remove NULL check from current in tracing_generic_entry_update().
2021-02-03 16:05 [for-next][PATCH 00/15] tracing: Updates for 5.12 Steven Rostedt
` (6 preceding siblings ...)
2021-02-03 16:05 ` [for-next][PATCH 08/15] tracing: Use in_serving_softirq() to deduct softirq status Steven Rostedt
@ 2021-02-03 16:05 ` Steven Rostedt
2021-02-03 16:05 ` [for-next][PATCH 10/15] tracing: Fix spelling mistake in Kconfig "infinit" -> "infinite" Steven Rostedt
` (5 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Steven Rostedt @ 2021-02-03 16:05 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Sebastian Andrzej Siewior
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
I can't imagine when or why `current' would return a NULL pointer. This
check was added in commit
72829bc3d63cd ("ftrace: move enums to ftrace.h and make helper function global")
but it doesn't give me hint why it was needed.
Assume `current' never returns a NULL pointer and remove the check.
Link: https://lkml.kernel.org/r/20210125194511.3924915-5-bigeasy@linutronix.de
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
include/linux/trace_events.h | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index 67ae708de40d..5d1eeac4bfbe 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -152,10 +152,8 @@ static inline void tracing_generic_entry_update(struct trace_entry *entry,
unsigned short type,
unsigned int trace_ctx)
{
- struct task_struct *tsk = current;
-
entry->preempt_count = trace_ctx & 0xff;
- entry->pid = (tsk) ? tsk->pid : 0;
+ entry->pid = current->pid;
entry->type = type;
entry->flags = trace_ctx >> 16;
}
--
2.29.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [for-next][PATCH 10/15] tracing: Fix spelling mistake in Kconfig "infinit" -> "infinite"
2021-02-03 16:05 [for-next][PATCH 00/15] tracing: Updates for 5.12 Steven Rostedt
` (7 preceding siblings ...)
2021-02-03 16:05 ` [for-next][PATCH 09/15] tracing: Remove NULL check from current in tracing_generic_entry_update() Steven Rostedt
@ 2021-02-03 16:05 ` Steven Rostedt
2021-02-03 16:05 ` [for-next][PATCH 11/15] tracing: Fix spelling of controlling in uprobes Steven Rostedt
` (4 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Steven Rostedt @ 2021-02-03 16:05 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Colin Ian King
From: Colin Ian King <colin.king@canonical.com>
There is a spelling mistake in the Kconfig help text. Fix it.
Link: https://lkml.kernel.org/r/20201216114051.12056-1-colin.king@canonical.com
Signed-off-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
kernel/trace/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index c1a62ae7e812..4f976f8d9a38 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -545,7 +545,7 @@ config KPROBE_EVENTS_ON_NOTRACE
using kprobe events.
If kprobes can use ftrace instead of breakpoint, ftrace related
- functions are protected from kprobe-events to prevent an infinit
+ functions are protected from kprobe-events to prevent an infinite
recursion or any unexpected execution path which leads to a kernel
crash.
--
2.29.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [for-next][PATCH 11/15] tracing: Fix spelling of controlling in uprobes
2021-02-03 16:05 [for-next][PATCH 00/15] tracing: Updates for 5.12 Steven Rostedt
` (8 preceding siblings ...)
2021-02-03 16:05 ` [for-next][PATCH 10/15] tracing: Fix spelling mistake in Kconfig "infinit" -> "infinite" Steven Rostedt
@ 2021-02-03 16:05 ` Steven Rostedt
2021-02-03 16:05 ` [for-next][PATCH 12/15] tracing: Fix a kernel doc warning Steven Rostedt
` (3 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Steven Rostedt @ 2021-02-03 16:05 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Bhaskar Chowdhury, Randy Dunlap
From: Bhaskar Chowdhury <unixbhaskar@gmail.com>
s/controling/controlling/p
Link: https://lkml.kernel.org/r/20210112045008.29834-1-unixbhaskar@gmail.com
Signed-off-by: Bhaskar Chowdhury <unixbhaskar@gmail.com>
Acked-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
kernel/trace/trace_uprobe.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index a1ed96a7a462..9d9440303075 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -1635,7 +1635,7 @@ void destroy_local_trace_uprobe(struct trace_event_call *event_call)
}
#endif /* CONFIG_PERF_EVENTS */
-/* Make a trace interface for controling probe points */
+/* Make a trace interface for controlling probe points */
static __init int init_uprobe_trace(void)
{
int ret;
--
2.29.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [for-next][PATCH 12/15] tracing: Fix a kernel doc warning
2021-02-03 16:05 [for-next][PATCH 00/15] tracing: Updates for 5.12 Steven Rostedt
` (9 preceding siblings ...)
2021-02-03 16:05 ` [for-next][PATCH 11/15] tracing: Fix spelling of controlling in uprobes Steven Rostedt
@ 2021-02-03 16:05 ` Steven Rostedt
2021-02-03 16:05 ` [for-next][PATCH 13/15] tracing: Remove definition of DEBUG in trace_mmiotrace.c Steven Rostedt
` (2 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Steven Rostedt @ 2021-02-03 16:05 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Bean Huo
From: Bean Huo <beanhuo@micron.com>
Add description for trace_array_put() parameter.
kernel/trace/trace.c:464: warning: Function parameter or member 'this_tr' not described in 'trace_array_put'
Link: https://lkml.kernel.org/r/20210112111202.23508-1-huobean@gmail.com
Signed-off-by: Bean Huo <beanhuo@micron.com>
[ Merged as one of the original fixes was already fixed by someone else ]
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
kernel/trace/trace.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 75620c29e904..7fd432334ff5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -454,6 +454,7 @@ static void __trace_array_put(struct trace_array *this_tr)
/**
* trace_array_put - Decrement the reference counter for this trace array.
+ * @this_tr : pointer to the trace array
*
* NOTE: Use this when we no longer need the trace array returned by
* trace_array_get_by_name(). This ensures the trace array can be later
--
2.29.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [for-next][PATCH 13/15] tracing: Remove definition of DEBUG in trace_mmiotrace.c
2021-02-03 16:05 [for-next][PATCH 00/15] tracing: Updates for 5.12 Steven Rostedt
` (10 preceding siblings ...)
2021-02-03 16:05 ` [for-next][PATCH 12/15] tracing: Fix a kernel doc warning Steven Rostedt
@ 2021-02-03 16:05 ` Steven Rostedt
2021-02-03 16:05 ` [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure Steven Rostedt
2021-02-03 16:05 ` [for-next][PATCH 15/15] kernel: trace: preemptirq_delay_test: add cpu affinity Steven Rostedt
13 siblings, 0 replies; 22+ messages in thread
From: Steven Rostedt @ 2021-02-03 16:05 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Tom Rix, Karol Herbst
From: Tom Rix <trix@redhat.com>
Defining DEBUG should only be done in development.
So remove DEBUG.
Link: https://lkml.kernel.org/r/20210115153348.131791-1-trix@redhat.com
Signed-off-by: Tom Rix <trix@redhat.com>
Reviewed-by: Karol Herbst <kherbst@redhat.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
kernel/trace/trace_mmiotrace.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/kernel/trace/trace_mmiotrace.c b/kernel/trace/trace_mmiotrace.c
index 7221ae0b4c47..64e77b513697 100644
--- a/kernel/trace/trace_mmiotrace.c
+++ b/kernel/trace/trace_mmiotrace.c
@@ -5,8 +5,6 @@
* Copyright (C) 2008 Pekka Paalanen <pq@iki.fi>
*/
-#define DEBUG 1
-
#include <linux/kernel.h>
#include <linux/mmiotrace.h>
#include <linux/pci.h>
--
2.29.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure
2021-02-03 16:05 [for-next][PATCH 00/15] tracing: Updates for 5.12 Steven Rostedt
` (11 preceding siblings ...)
2021-02-03 16:05 ` [for-next][PATCH 13/15] tracing: Remove definition of DEBUG in trace_mmiotrace.c Steven Rostedt
@ 2021-02-03 16:05 ` Steven Rostedt
2021-02-03 17:05 ` Peter Zijlstra
` (3 more replies)
2021-02-03 16:05 ` [for-next][PATCH 15/15] kernel: trace: preemptirq_delay_test: add cpu affinity Steven Rostedt
13 siblings, 4 replies; 22+ messages in thread
From: Steven Rostedt @ 2021-02-03 16:05 UTC (permalink / raw)
To: linux-kernel
Cc: Ingo Molnar, Andrew Morton, Peter Zijlstra, Josh Poimboeuf,
Mathieu Desnoyers, Ingo Molnar, Alexei Starovoitov,
Daniel Borkmann, Dmitry Vyukov, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, netdev,
bpf, Kees Cook, Florian Weimer, syzbot+83aa762ef23b6f0d1991,
syzbot+d29e58bb557324e55e5e, Matt Mullins
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
The list of tracepoint callbacks is managed by an array that is protected
by RCU. To update this array, a new array is allocated, the updates are
copied over to the new array, and then the list of functions for the
tracepoint is switched over to the new array. After a completion of an RCU
grace period, the old array is freed.
This process happens for both adding a callback as well as removing one.
But on removing a callback, if the new array fails to be allocated, the
callback is not removed, and may be used after it is freed by the clients
of the tracepoint.
There's really no reason to fail if the allocation for a new array fails
when removing a function. Instead, the function can simply be replaced by a
stub function that could be cleaned up on the next modification of the
array. That is, instead of calling the function registered to the
tracepoint, it would call a stub function in its place.
Link: https://lore.kernel.org/r/20201115055256.65625-1-mmullins@mmlx.us
Link: https://lore.kernel.org/r/20201116175107.02db396d@gandalf.local.home
Link: https://lore.kernel.org/r/20201117211836.54acaef2@oasis.local.home
Link: https://lkml.kernel.org/r/20201118093405.7a6d2290@gandalf.local.home
[ Note, this version does use undefined compiler behavior (assuming that
a stub function with no parameters or return, can be called by a location
that thinks it has parameters but still no return value. Static calls
do the same thing, so this trick is not without precedent.
There's another solution that uses RCU tricks and is more complex, but
can be an alternative if this solution becomes an issue.
Link: https://lore.kernel.org/lkml/20210127170721.58bce7cc@gandalf.local.home/
]
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Song Liu <songliubraving@fb.com>
Cc: Yonghong Song <yhs@fb.com>
Cc: Andrii Nakryiko <andriin@fb.com>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@chromium.org>
Cc: netdev <netdev@vger.kernel.org>
Cc: bpf <bpf@vger.kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Florian Weimer <fw@deneb.enyo.de>
Fixes: 97e1c18e8d17b ("tracing: Kernel Tracepoints")
Reported-by: syzbot+83aa762ef23b6f0d1991@syzkaller.appspotmail.com
Reported-by: syzbot+d29e58bb557324e55e5e@syzkaller.appspotmail.com
Reported-by: Matt Mullins <mmullins@mmlx.us>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Matt Mullins <mmullins@mmlx.us>
---
kernel/tracepoint.c | 80 ++++++++++++++++++++++++++++++++++++---------
1 file changed, 64 insertions(+), 16 deletions(-)
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 7261fa0f5e3c..e8f20ae29c18 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -53,6 +53,12 @@ struct tp_probes {
struct tracepoint_func probes[];
};
+/* Called in removal of a func but failed to allocate a new tp_funcs */
+static void tp_stub_func(void)
+{
+ return;
+}
+
static inline void *allocate_probes(int count)
{
struct tp_probes *p = kmalloc(struct_size(p, probes, count),
@@ -131,6 +137,7 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
{
struct tracepoint_func *old, *new;
int nr_probes = 0;
+ int stub_funcs = 0;
int pos = -1;
if (WARN_ON(!tp_func->func))
@@ -147,14 +154,34 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
if (old[nr_probes].func == tp_func->func &&
old[nr_probes].data == tp_func->data)
return ERR_PTR(-EEXIST);
+ if (old[nr_probes].func == tp_stub_func)
+ stub_funcs++;
}
}
- /* + 2 : one for new probe, one for NULL func */
- new = allocate_probes(nr_probes + 2);
+ /* + 2 : one for new probe, one for NULL func - stub functions */
+ new = allocate_probes(nr_probes + 2 - stub_funcs);
if (new == NULL)
return ERR_PTR(-ENOMEM);
if (old) {
- if (pos < 0) {
+ if (stub_funcs) {
+ /* Need to copy one at a time to remove stubs */
+ int probes = 0;
+
+ pos = -1;
+ for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
+ if (old[nr_probes].func == tp_stub_func)
+ continue;
+ if (pos < 0 && old[nr_probes].prio < prio)
+ pos = probes++;
+ new[probes++] = old[nr_probes];
+ }
+ nr_probes = probes;
+ if (pos < 0)
+ pos = probes;
+ else
+ nr_probes--; /* Account for insertion */
+
+ } else if (pos < 0) {
pos = nr_probes;
memcpy(new, old, nr_probes * sizeof(struct tracepoint_func));
} else {
@@ -188,8 +215,9 @@ static void *func_remove(struct tracepoint_func **funcs,
/* (N -> M), (N > 1, M >= 0) probes */
if (tp_func->func) {
for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
- if (old[nr_probes].func == tp_func->func &&
- old[nr_probes].data == tp_func->data)
+ if ((old[nr_probes].func == tp_func->func &&
+ old[nr_probes].data == tp_func->data) ||
+ old[nr_probes].func == tp_stub_func)
nr_del++;
}
}
@@ -208,14 +236,32 @@ static void *func_remove(struct tracepoint_func **funcs,
/* N -> M, (N > 1, M > 0) */
/* + 1 for NULL */
new = allocate_probes(nr_probes - nr_del + 1);
- if (new == NULL)
- return ERR_PTR(-ENOMEM);
- for (i = 0; old[i].func; i++)
- if (old[i].func != tp_func->func
- || old[i].data != tp_func->data)
- new[j++] = old[i];
- new[nr_probes - nr_del].func = NULL;
- *funcs = new;
+ if (new) {
+ for (i = 0; old[i].func; i++)
+ if ((old[i].func != tp_func->func
+ || old[i].data != tp_func->data)
+ && old[i].func != tp_stub_func)
+ new[j++] = old[i];
+ new[nr_probes - nr_del].func = NULL;
+ *funcs = new;
+ } else {
+ /*
+ * Failed to allocate, replace the old function
+ * with calls to tp_stub_func.
+ */
+ for (i = 0; old[i].func; i++)
+ if (old[i].func == tp_func->func &&
+ old[i].data == tp_func->data) {
+ old[i].func = tp_stub_func;
+ /* Set the prio to the next event. */
+ if (old[i + 1].func)
+ old[i].prio =
+ old[i + 1].prio;
+ else
+ old[i].prio = -1;
+ }
+ *funcs = old;
+ }
}
debug_print_probes(*funcs);
return old;
@@ -295,10 +341,12 @@ static int tracepoint_remove_func(struct tracepoint *tp,
tp_funcs = rcu_dereference_protected(tp->funcs,
lockdep_is_held(&tracepoints_mutex));
old = func_remove(&tp_funcs, func);
- if (IS_ERR(old)) {
- WARN_ON_ONCE(PTR_ERR(old) != -ENOMEM);
+ if (WARN_ON_ONCE(IS_ERR(old)))
return PTR_ERR(old);
- }
+
+ if (tp_funcs == old)
+ /* Failed allocating new tp_funcs, replaced func with stub */
+ return 0;
if (!tp_funcs) {
/* Removed last function */
--
2.29.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure
2021-02-03 16:05 ` [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure Steven Rostedt
@ 2021-02-03 17:05 ` Peter Zijlstra
2021-02-03 17:09 ` Peter Zijlstra
` (2 subsequent siblings)
3 siblings, 0 replies; 22+ messages in thread
From: Peter Zijlstra @ 2021-02-03 17:05 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Josh Poimboeuf,
Mathieu Desnoyers, Ingo Molnar, Alexei Starovoitov,
Daniel Borkmann, Dmitry Vyukov, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, netdev,
bpf, Kees Cook, Florian Weimer, syzbot+83aa762ef23b6f0d1991,
syzbot+d29e58bb557324e55e5e, Matt Mullins
On Wed, Feb 03, 2021 at 11:05:31AM -0500, Steven Rostedt wrote:
> [ Note, this version does use undefined compiler behavior (assuming that
> a stub function with no parameters or return, can be called by a location
> that thinks it has parameters but still no return value. Static calls
> do the same thing, so this trick is not without precedent.
Specifically it relies on the C ABI being caller cleanup. CDECL is that.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure
2021-02-03 16:05 ` [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure Steven Rostedt
2021-02-03 17:05 ` Peter Zijlstra
@ 2021-02-03 17:09 ` Peter Zijlstra
2021-02-03 17:23 ` Steven Rostedt
2021-02-03 17:57 ` Mathieu Desnoyers
2021-02-03 17:57 ` [PATCH 1/1] tracepoint: cleanup: do not fail unregistration Mathieu Desnoyers
3 siblings, 1 reply; 22+ messages in thread
From: Peter Zijlstra @ 2021-02-03 17:09 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Josh Poimboeuf,
Mathieu Desnoyers, Ingo Molnar, Alexei Starovoitov,
Daniel Borkmann, Dmitry Vyukov, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, netdev,
bpf, Kees Cook, Florian Weimer, syzbot+83aa762ef23b6f0d1991,
syzbot+d29e58bb557324e55e5e, Matt Mullins
On Wed, Feb 03, 2021 at 11:05:31AM -0500, Steven Rostedt wrote:
> + if (new) {
> + for (i = 0; old[i].func; i++)
> + if ((old[i].func != tp_func->func
> + || old[i].data != tp_func->data)
> + && old[i].func != tp_stub_func)
logical operators go at the end..
> + new[j++] = old[i];
> + new[nr_probes - nr_del].func = NULL;
> + *funcs = new;
> + } else {
> + /*
> + * Failed to allocate, replace the old function
> + * with calls to tp_stub_func.
> + */
> + for (i = 0; old[i].func; i++)
{
> + if (old[i].func == tp_func->func &&
> + old[i].data == tp_func->data) {
like here.
> + old[i].func = tp_stub_func;
> + /* Set the prio to the next event. */
> + if (old[i + 1].func)
> + old[i].prio =
> + old[i + 1].prio;
multi line demands { }, but in this case just don't line-break.
> + else
> + old[i].prio = -1;
> + }
}
> + *funcs = old;
> + }
> }
> debug_print_probes(*funcs);
> return old;
> @@ -295,10 +341,12 @@ static int tracepoint_remove_func(struct tracepoint *tp,
> tp_funcs = rcu_dereference_protected(tp->funcs,
> lockdep_is_held(&tracepoints_mutex));
> old = func_remove(&tp_funcs, func);
> - if (IS_ERR(old)) {
> - WARN_ON_ONCE(PTR_ERR(old) != -ENOMEM);
> + if (WARN_ON_ONCE(IS_ERR(old)))
> return PTR_ERR(old);
> - }
> +
> + if (tp_funcs == old)
> + /* Failed allocating new tp_funcs, replaced func with stub */
> + return 0;
{ }
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure
2021-02-03 17:09 ` Peter Zijlstra
@ 2021-02-03 17:23 ` Steven Rostedt
0 siblings, 0 replies; 22+ messages in thread
From: Steven Rostedt @ 2021-02-03 17:23 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Josh Poimboeuf,
Mathieu Desnoyers, Ingo Molnar, Alexei Starovoitov,
Daniel Borkmann, Dmitry Vyukov, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, netdev,
bpf, Kees Cook, Florian Weimer, syzbot+83aa762ef23b6f0d1991,
syzbot+d29e58bb557324e55e5e, Matt Mullins
On Wed, 3 Feb 2021 18:09:27 +0100
Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Feb 03, 2021 at 11:05:31AM -0500, Steven Rostedt wrote:
> > + if (new) {
> > + for (i = 0; old[i].func; i++)
> > + if ((old[i].func != tp_func->func
> > + || old[i].data != tp_func->data)
> > + && old[i].func != tp_stub_func)
>
> logical operators go at the end..
Agreed. I just added that "if (new) {" around the original block, didn't
think about the formatting when doing so.
>
> > + new[j++] = old[i];
> > + new[nr_probes - nr_del].func = NULL;
> > + *funcs = new;
> > + } else {
> > + /*
> > + * Failed to allocate, replace the old function
> > + * with calls to tp_stub_func.
> > + */
> > + for (i = 0; old[i].func; i++)
>
> {
>
> > + if (old[i].func == tp_func->func &&
> > + old[i].data == tp_func->data) {
>
> like here.
>
> > + old[i].func = tp_stub_func;
> > + /* Set the prio to the next event. */
> > + if (old[i + 1].func)
> > + old[i].prio =
> > + old[i + 1].prio;
>
> multi line demands { }, but in this case just don't line-break.
Sure.
>
> > + else
> > + old[i].prio = -1;
> > + }
>
> }
>
> > + *funcs = old;
> > + }
> > }
> > debug_print_probes(*funcs);
> > return old;
> > @@ -295,10 +341,12 @@ static int tracepoint_remove_func(struct tracepoint *tp,
> > tp_funcs = rcu_dereference_protected(tp->funcs,
> > lockdep_is_held(&tracepoints_mutex));
> > old = func_remove(&tp_funcs, func);
> > - if (IS_ERR(old)) {
> > - WARN_ON_ONCE(PTR_ERR(old) != -ENOMEM);
> > + if (WARN_ON_ONCE(IS_ERR(old)))
> > return PTR_ERR(old);
> > - }
> > +
> > + if (tp_funcs == old)
> > + /* Failed allocating new tp_funcs, replaced func with stub */
> > + return 0;
>
> { }
Even if it's just a comment that causes multiple lines? I could just move
the comment above the if.
This has already been through my test suite, and since the changes
requested are just formatting and non-functional, I'll just add a clean up
patch on top.
Thanks!
-- Steve
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure
2021-02-03 16:05 ` [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure Steven Rostedt
2021-02-03 17:05 ` Peter Zijlstra
2021-02-03 17:09 ` Peter Zijlstra
@ 2021-02-03 17:57 ` Mathieu Desnoyers
2021-02-04 16:50 ` Steven Rostedt
2021-02-03 17:57 ` [PATCH 1/1] tracepoint: cleanup: do not fail unregistration Mathieu Desnoyers
3 siblings, 1 reply; 22+ messages in thread
From: Mathieu Desnoyers @ 2021-02-03 17:57 UTC (permalink / raw)
To: rostedt
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Peter Zijlstra,
Josh Poimboeuf, Ingo Molnar, Alexei Starovoitov, Daniel Borkmann,
Dmitry Vyukov, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, John Fastabend, KP Singh, netdev, bpf,
Kees Cook, Florian Weimer, syzbot+83aa762ef23b6f0d1991,
syzbot+d29e58bb557324e55e5e, Matt Mullins
----- On Feb 3, 2021, at 11:05 AM, rostedt rostedt@goodmis.org wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> The list of tracepoint callbacks is managed by an array that is protected
> by RCU. To update this array, a new array is allocated, the updates are
> copied over to the new array, and then the list of functions for the
> tracepoint is switched over to the new array. After a completion of an RCU
> grace period, the old array is freed.
>
> This process happens for both adding a callback as well as removing one.
> But on removing a callback, if the new array fails to be allocated, the
> callback is not removed, and may be used after it is freed by the clients
> of the tracepoint.
>
> There's really no reason to fail if the allocation for a new array fails
> when removing a function. Instead, the function can simply be replaced by a
> stub function that could be cleaned up on the next modification of the
> array. That is, instead of calling the function registered to the
> tracepoint, it would call a stub function in its place.
>
> Link: https://lore.kernel.org/r/20201115055256.65625-1-mmullins@mmlx.us
> Link: https://lore.kernel.org/r/20201116175107.02db396d@gandalf.local.home
> Link: https://lore.kernel.org/r/20201117211836.54acaef2@oasis.local.home
> Link: https://lkml.kernel.org/r/20201118093405.7a6d2290@gandalf.local.home
>
> [ Note, this version does use undefined compiler behavior (assuming that
> a stub function with no parameters or return, can be called by a location
> that thinks it has parameters but still no return value. Static calls
> do the same thing, so this trick is not without precedent.
>
> There's another solution that uses RCU tricks and is more complex, but
> can be an alternative if this solution becomes an issue.
>
> Link: https://lore.kernel.org/lkml/20210127170721.58bce7cc@gandalf.local.home/
> ]
>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Martin KaFai Lau <kafai@fb.com>
> Cc: Song Liu <songliubraving@fb.com>
> Cc: Yonghong Song <yhs@fb.com>
> Cc: Andrii Nakryiko <andriin@fb.com>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: KP Singh <kpsingh@chromium.org>
> Cc: netdev <netdev@vger.kernel.org>
> Cc: bpf <bpf@vger.kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Florian Weimer <fw@deneb.enyo.de>
> Fixes: 97e1c18e8d17b ("tracing: Kernel Tracepoints")
> Reported-by: syzbot+83aa762ef23b6f0d1991@syzkaller.appspotmail.com
> Reported-by: syzbot+d29e58bb557324e55e5e@syzkaller.appspotmail.com
> Reported-by: Matt Mullins <mmullins@mmlx.us>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Tested-by: Matt Mullins <mmullins@mmlx.us>
> ---
> kernel/tracepoint.c | 80 ++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 64 insertions(+), 16 deletions(-)
>
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index 7261fa0f5e3c..e8f20ae29c18 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c
> @@ -53,6 +53,12 @@ struct tp_probes {
> struct tracepoint_func probes[];
> };
>
> +/* Called in removal of a func but failed to allocate a new tp_funcs */
> +static void tp_stub_func(void)
> +{
> + return;
> +}
> +
> static inline void *allocate_probes(int count)
> {
> struct tp_probes *p = kmalloc(struct_size(p, probes, count),
> @@ -131,6 +137,7 @@ func_add(struct tracepoint_func **funcs, struct
> tracepoint_func *tp_func,
> {
> struct tracepoint_func *old, *new;
> int nr_probes = 0;
> + int stub_funcs = 0;
> int pos = -1;
>
> if (WARN_ON(!tp_func->func))
> @@ -147,14 +154,34 @@ func_add(struct tracepoint_func **funcs, struct
> tracepoint_func *tp_func,
> if (old[nr_probes].func == tp_func->func &&
> old[nr_probes].data == tp_func->data)
> return ERR_PTR(-EEXIST);
> + if (old[nr_probes].func == tp_stub_func)
> + stub_funcs++;
> }
> }
> - /* + 2 : one for new probe, one for NULL func */
> - new = allocate_probes(nr_probes + 2);
> + /* + 2 : one for new probe, one for NULL func - stub functions */
> + new = allocate_probes(nr_probes + 2 - stub_funcs);
> if (new == NULL)
> return ERR_PTR(-ENOMEM);
> if (old) {
> - if (pos < 0) {
> + if (stub_funcs) {
Considering that we end up implementing a case where we carefully copy over
each item, I recommend we replace the two "memcpy" branches by a single item-wise
implementation. It's a slow-path anyway, and reducing the overall complexity
is a benefit for slow paths. Fewer bugs, less code to review, and it's easier to
reach a decent testing state-space coverage.
> + /* Need to copy one at a time to remove stubs */
> + int probes = 0;
> +
> + pos = -1;
> + for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> + if (old[nr_probes].func == tp_stub_func)
> + continue;
> + if (pos < 0 && old[nr_probes].prio < prio)
> + pos = probes++;
> + new[probes++] = old[nr_probes];
> + }
> + nr_probes = probes;
Repurposing "nr_probes" from accounting for the number of items in the old
array to counting the number of items in the new array in the middle of the
function is confusing.
> + if (pos < 0)
> + pos = probes;
> + else
> + nr_probes--; /* Account for insertion */
This is probably why you need to play tricks with nr_probes here.
> + } else if (pos < 0) {
> pos = nr_probes;
> memcpy(new, old, nr_probes * sizeof(struct tracepoint_func));
> } else {
> @@ -188,8 +215,9 @@ static void *func_remove(struct tracepoint_func **funcs,
> /* (N -> M), (N > 1, M >= 0) probes */
> if (tp_func->func) {
> for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> - if (old[nr_probes].func == tp_func->func &&
> - old[nr_probes].data == tp_func->data)
> + if ((old[nr_probes].func == tp_func->func &&
> + old[nr_probes].data == tp_func->data) ||
> + old[nr_probes].func == tp_stub_func)
> nr_del++;
> }
> }
> @@ -208,14 +236,32 @@ static void *func_remove(struct tracepoint_func **funcs,
> /* N -> M, (N > 1, M > 0) */
> /* + 1 for NULL */
> new = allocate_probes(nr_probes - nr_del + 1);
> - if (new == NULL)
> - return ERR_PTR(-ENOMEM);
> - for (i = 0; old[i].func; i++)
> - if (old[i].func != tp_func->func
> - || old[i].data != tp_func->data)
> - new[j++] = old[i];
> - new[nr_probes - nr_del].func = NULL;
> - *funcs = new;
> + if (new) {
> + for (i = 0; old[i].func; i++)
> + if ((old[i].func != tp_func->func
> + || old[i].data != tp_func->data)
> + && old[i].func != tp_stub_func)
> + new[j++] = old[i];
> + new[nr_probes - nr_del].func = NULL;
> + *funcs = new;
> + } else {
> + /*
> + * Failed to allocate, replace the old function
> + * with calls to tp_stub_func.
> + */
> + for (i = 0; old[i].func; i++)
> + if (old[i].func == tp_func->func &&
> + old[i].data == tp_func->data) {
> + old[i].func = tp_stub_func;
This updates "func" while readers are loading it concurrently. I would recommend
using WRITE_ONCE here paired with READ_ONCE within __traceiter_##_name.
> + /* Set the prio to the next event. */
I don't get why the priority needs to be changed here. Could it simply stay
at its original value ? It's already in the correct priority order anyway.
> + if (old[i + 1].func)
> + old[i].prio =
> + old[i + 1].prio;
> + else
> + old[i].prio = -1;
> + }
> + *funcs = old;
I'm not sure what setting *funcs to old achieves ? Isn't it already pointing
to old ?
I'll send a patch which applies on top of yours implementing my recommendations.
It shrinks the code complexity nicely:
include/linux/tracepoint.h | 2 +-
kernel/tracepoint.c | 80 +++++++++++++-------------------------
2 files changed, 28 insertions(+), 54 deletions(-)
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure
2021-02-03 17:57 ` Mathieu Desnoyers
@ 2021-02-04 16:50 ` Steven Rostedt
0 siblings, 0 replies; 22+ messages in thread
From: Steven Rostedt @ 2021-02-04 16:50 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Peter Zijlstra,
Josh Poimboeuf, Ingo Molnar, Alexei Starovoitov, Daniel Borkmann,
Dmitry Vyukov, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, John Fastabend, KP Singh, netdev, bpf,
Kees Cook, Florian Weimer, syzbot+83aa762ef23b6f0d1991,
syzbot+d29e58bb557324e55e5e, Matt Mullins
On Wed, 3 Feb 2021 12:57:24 -0500 (EST)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> > @@ -147,14 +154,34 @@ func_add(struct tracepoint_func **funcs, struct
> > tracepoint_func *tp_func,
> > if (old[nr_probes].func == tp_func->func &&
> > old[nr_probes].data == tp_func->data)
> > return ERR_PTR(-EEXIST);
> > + if (old[nr_probes].func == tp_stub_func)
> > + stub_funcs++;
> > }
> > }
> > - /* + 2 : one for new probe, one for NULL func */
> > - new = allocate_probes(nr_probes + 2);
> > + /* + 2 : one for new probe, one for NULL func - stub functions */
> > + new = allocate_probes(nr_probes + 2 - stub_funcs);
> > if (new == NULL)
> > return ERR_PTR(-ENOMEM);
> > if (old) {
> > - if (pos < 0) {
> > + if (stub_funcs) {
>
> Considering that we end up implementing a case where we carefully copy over
> each item, I recommend we replace the two "memcpy" branches by a single item-wise
> implementation. It's a slow-path anyway, and reducing the overall complexity
> is a benefit for slow paths. Fewer bugs, less code to review, and it's easier to
> reach a decent testing state-space coverage.
Sure.
>
> > + /* Need to copy one at a time to remove stubs */
> > + int probes = 0;
> > +
> > + pos = -1;
> > + for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> > + if (old[nr_probes].func == tp_stub_func)
> > + continue;
> > + if (pos < 0 && old[nr_probes].prio < prio)
> > + pos = probes++;
> > + new[probes++] = old[nr_probes];
> > + }
> > + nr_probes = probes;
>
> Repurposing "nr_probes" from accounting for the number of items in the old
> array to counting the number of items in the new array in the middle of the
> function is confusing.
>
> > + if (pos < 0)
> > + pos = probes;
> > + else
> > + nr_probes--; /* Account for insertion */
>
> This is probably why you need to play tricks with nr_probes here.
>
> > + } else if (pos < 0) {
> > pos = nr_probes;
> > memcpy(new, old, nr_probes * sizeof(struct tracepoint_func));
> > } else {
> > @@ -188,8 +215,9 @@ static void *func_remove(struct tracepoint_func **funcs,
> > /* (N -> M), (N > 1, M >= 0) probes */
> > if (tp_func->func) {
> > for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> > - if (old[nr_probes].func == tp_func->func &&
> > - old[nr_probes].data == tp_func->data)
> > + if ((old[nr_probes].func == tp_func->func &&
> > + old[nr_probes].data == tp_func->data) ||
> > + old[nr_probes].func == tp_stub_func)
> > nr_del++;
> > }
> > }
> > @@ -208,14 +236,32 @@ static void *func_remove(struct tracepoint_func **funcs,
> > /* N -> M, (N > 1, M > 0) */
> > /* + 1 for NULL */
> > new = allocate_probes(nr_probes - nr_del + 1);
> > - if (new == NULL)
> > - return ERR_PTR(-ENOMEM);
> > - for (i = 0; old[i].func; i++)
> > - if (old[i].func != tp_func->func
> > - || old[i].data != tp_func->data)
> > - new[j++] = old[i];
> > - new[nr_probes - nr_del].func = NULL;
> > - *funcs = new;
> > + if (new) {
> > + for (i = 0; old[i].func; i++)
> > + if ((old[i].func != tp_func->func
> > + || old[i].data != tp_func->data)
> > + && old[i].func != tp_stub_func)
> > + new[j++] = old[i];
> > + new[nr_probes - nr_del].func = NULL;
> > + *funcs = new;
> > + } else {
> > + /*
> > + * Failed to allocate, replace the old function
> > + * with calls to tp_stub_func.
> > + */
> > + for (i = 0; old[i].func; i++)
> > + if (old[i].func == tp_func->func &&
> > + old[i].data == tp_func->data) {
> > + old[i].func = tp_stub_func;
>
> This updates "func" while readers are loading it concurrently. I would recommend
> using WRITE_ONCE here paired with READ_ONCE within __traceiter_##_name.
I'm fine with this change, but it shouldn't make a difference. As we don't
change the data, it doesn't matter which function the compiler calls.
Unless you are worried about the compiler tearing the read. It shouldn't,
but I'm fine with doing things for paranoid sake (especially if it doesn't
affect the performance).
>
> > + /* Set the prio to the next event. */
>
> I don't get why the priority needs to be changed here. Could it simply stay
> at its original value ? It's already in the correct priority order anyway.
I think it was left over from one of the various changes. As I went to v5,
and then back to v3, I missed revisiting the code, as I was under the
assumption that I had cleaned it up :-/
>
> > + if (old[i + 1].func)
> > + old[i].prio =
> > + old[i + 1].prio;
> > + else
> > + old[i].prio = -1;
> > + }
> > + *funcs = old;
>
> I'm not sure what setting *funcs to old achieves ? Isn't it already pointing
> to old ?
Again, I think one iteration may have changed it. And I kinda remember
keeping it just to be consistent (*funcs gets updated in the other paths,
and figured it was good to be "safe" and updated it again, even though the
logic has it already set).
>
> I'll send a patch which applies on top of yours implementing my recommendations.
> It shrinks the code complexity nicely:
Looking at it now.
Thanks,
-- Steve
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/1] tracepoint: cleanup: do not fail unregistration
2021-02-03 16:05 ` [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure Steven Rostedt
` (2 preceding siblings ...)
2021-02-03 17:57 ` Mathieu Desnoyers
@ 2021-02-03 17:57 ` Mathieu Desnoyers
2021-02-04 17:47 ` Steven Rostedt
3 siblings, 1 reply; 22+ messages in thread
From: Mathieu Desnoyers @ 2021-02-03 17:57 UTC (permalink / raw)
To: rostedt; +Cc: linux-kernel, Mathieu Desnoyers
This patch is only compile-tested.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
include/linux/tracepoint.h | 2 +-
kernel/tracepoint.c | 80 +++++++++++++-------------------------
2 files changed, 28 insertions(+), 54 deletions(-)
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 0f21617f1a66..fae8d6d9588c 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -308,7 +308,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
it_func_ptr = \
rcu_dereference_raw((&__tracepoint_##_name)->funcs); \
do { \
- it_func = (it_func_ptr)->func; \
+ it_func = READ_ONCE((it_func_ptr)->func); \
__data = (it_func_ptr)->data; \
((void(*)(void *, proto))(it_func))(__data, args); \
} while ((++it_func_ptr)->func); \
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 3e261482296c..b1bec710f68a 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -136,9 +136,10 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
int prio)
{
struct tracepoint_func *old, *new;
- int nr_probes = 0;
- int stub_funcs = 0;
- int pos = -1;
+ int iter_probes = 0; /* Iterate over old probe array. */
+ int nr_old_probes = 0; /* Count non-stub functions in old. */
+ int nr_new_probes = 0; /* Count functions in new. */
+ int pos = -1; /* Insert position in new. */
if (WARN_ON(!tp_func->func))
return ERR_PTR(-EINVAL);
@@ -147,54 +148,34 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
old = *funcs;
if (old) {
/* (N -> N+1), (N != 0, 1) probes */
- for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
- /* Insert before probes of lower priority */
- if (pos < 0 && old[nr_probes].prio < prio)
- pos = nr_probes;
- if (old[nr_probes].func == tp_func->func &&
- old[nr_probes].data == tp_func->data)
+ for (iter_probes = 0; old[iter_probes].func; iter_probes++) {
+ if (old[iter_probes].func == tp_stub_func)
+ continue; /* Skip stub functions. */
+ if (old[iter_probes].func == tp_func->func &&
+ old[iter_probes].data == tp_func->data)
return ERR_PTR(-EEXIST);
- if (old[nr_probes].func == tp_stub_func)
- stub_funcs++;
+ /* Insert before probes of lower priority */
+ if (pos < 0 && old[iter_probes].prio < prio)
+ pos = nr_old_probes;
+ nr_old_probes++;
}
}
- /* + 2 : one for new probe, one for NULL func - stub functions */
- new = allocate_probes(nr_probes + 2 - stub_funcs);
+ /* + 2 : one for new probe, one for NULL func */
+ new = allocate_probes(nr_old_probes + 2);
if (new == NULL)
return ERR_PTR(-ENOMEM);
if (old) {
- if (stub_funcs) {
- /* Need to copy one at a time to remove stubs */
- int probes = 0;
-
- pos = -1;
- for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
- if (old[nr_probes].func == tp_stub_func)
- continue;
- if (pos < 0 && old[nr_probes].prio < prio)
- pos = probes++;
- new[probes++] = old[nr_probes];
- }
- nr_probes = probes;
- if (pos < 0)
- pos = probes;
- else
- nr_probes--; /* Account for insertion */
-
- } else if (pos < 0) {
- pos = nr_probes;
- memcpy(new, old, nr_probes * sizeof(struct tracepoint_func));
- } else {
- /* Copy higher priority probes ahead of the new probe */
- memcpy(new, old, pos * sizeof(struct tracepoint_func));
- /* Copy the rest after it. */
- memcpy(new + pos + 1, old + pos,
- (nr_probes - pos) * sizeof(struct tracepoint_func));
+ for (iter_probes = 0; old[iter_probes].func; iter_probes++) {
+ if (old[iter_probes].func == tp_stub_func)
+ continue; /* Skip stub functions. */
+ if (nr_new_probes != pos)
+ new[nr_new_probes] = old[iter_probes];
+ nr_new_probes++;
}
} else
pos = 0;
new[pos] = *tp_func;
- new[nr_probes + 1].func = NULL;
+ new[nr_new_probes + 1].func = NULL;
*funcs = new;
debug_print_probes(*funcs);
return old;
@@ -238,9 +219,9 @@ static void *func_remove(struct tracepoint_func **funcs,
new = allocate_probes(nr_probes - nr_del + 1);
if (new) {
for (i = 0; old[i].func; i++)
- if ((old[i].func != tp_func->func
- || old[i].data != tp_func->data)
- && old[i].func != tp_stub_func)
+ if ((old[i].func != tp_func->func ||
+ old[i].data != tp_func->data) &&
+ old[i].func != tp_stub_func)
new[j++] = old[i];
new[nr_probes - nr_del].func = NULL;
*funcs = new;
@@ -251,15 +232,8 @@ static void *func_remove(struct tracepoint_func **funcs,
*/
for (i = 0; old[i].func; i++)
if (old[i].func == tp_func->func &&
- old[i].data == tp_func->data) {
- old[i].func = tp_stub_func;
- /* Set the prio to the next event. */
- if (old[i + 1].func)
- old[i].prio =
- old[i + 1].prio;
- else
- old[i].prio = -1;
- }
+ old[i].data == tp_func->data)
+ WRITE_ONCE(old[i].func, tp_stub_func);
*funcs = old;
}
}
--
2.20.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 1/1] tracepoint: cleanup: do not fail unregistration
2021-02-03 17:57 ` [PATCH 1/1] tracepoint: cleanup: do not fail unregistration Mathieu Desnoyers
@ 2021-02-04 17:47 ` Steven Rostedt
0 siblings, 0 replies; 22+ messages in thread
From: Steven Rostedt @ 2021-02-04 17:47 UTC (permalink / raw)
To: Mathieu Desnoyers; +Cc: linux-kernel
On Wed, 3 Feb 2021 12:57:41 -0500
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> This patch is only compile-tested.
>
Yes it is. (crashed on boot)
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> ---
> include/linux/tracepoint.h | 2 +-
> kernel/tracepoint.c | 80 +++++++++++++-------------------------
> 2 files changed, 28 insertions(+), 54 deletions(-)
>
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 0f21617f1a66..fae8d6d9588c 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -308,7 +308,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> it_func_ptr = \
> rcu_dereference_raw((&__tracepoint_##_name)->funcs); \
> do { \
> - it_func = (it_func_ptr)->func; \
> + it_func = READ_ONCE((it_func_ptr)->func); \
> __data = (it_func_ptr)->data; \
> ((void(*)(void *, proto))(it_func))(__data, args); \
> } while ((++it_func_ptr)->func); \
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index 3e261482296c..b1bec710f68a 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c
> @@ -136,9 +136,10 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
> int prio)
> {
> struct tracepoint_func *old, *new;
> - int nr_probes = 0;
> - int stub_funcs = 0;
> - int pos = -1;
> + int iter_probes = 0; /* Iterate over old probe array. */
> + int nr_old_probes = 0; /* Count non-stub functions in old. */
> + int nr_new_probes = 0; /* Count functions in new. */
> + int pos = -1; /* Insert position in new. */
>
> if (WARN_ON(!tp_func->func))
> return ERR_PTR(-EINVAL);
> @@ -147,54 +148,34 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
> old = *funcs;
> if (old) {
> /* (N -> N+1), (N != 0, 1) probes */
> - for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> - /* Insert before probes of lower priority */
> - if (pos < 0 && old[nr_probes].prio < prio)
> - pos = nr_probes;
> - if (old[nr_probes].func == tp_func->func &&
> - old[nr_probes].data == tp_func->data)
> + for (iter_probes = 0; old[iter_probes].func; iter_probes++) {
> + if (old[iter_probes].func == tp_stub_func)
> + continue; /* Skip stub functions. */
> + if (old[iter_probes].func == tp_func->func &&
> + old[iter_probes].data == tp_func->data)
> return ERR_PTR(-EEXIST);
> - if (old[nr_probes].func == tp_stub_func)
> - stub_funcs++;
> + /* Insert before probes of lower priority */
> + if (pos < 0 && old[iter_probes].prio < prio)
> + pos = nr_old_probes;
> + nr_old_probes++;
> }
> }
> - /* + 2 : one for new probe, one for NULL func - stub functions */
> - new = allocate_probes(nr_probes + 2 - stub_funcs);
> + /* + 2 : one for new probe, one for NULL func */
> + new = allocate_probes(nr_old_probes + 2);
> if (new == NULL)
> return ERR_PTR(-ENOMEM);
> if (old) {
> - if (stub_funcs) {
> - /* Need to copy one at a time to remove stubs */
> - int probes = 0;
> -
> - pos = -1;
> - for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> - if (old[nr_probes].func == tp_stub_func)
> - continue;
> - if (pos < 0 && old[nr_probes].prio < prio)
> - pos = probes++;
> - new[probes++] = old[nr_probes];
> - }
> - nr_probes = probes;
> - if (pos < 0)
> - pos = probes;
> - else
> - nr_probes--; /* Account for insertion */
The above "trick" is to handle the case that we inserted a probe with the:
if (pos < 0 && old[nr_probes].prio < prio)
pos = probes++;
Which in the below code, you missed.
> -
> - } else if (pos < 0) {
> - pos = nr_probes;
> - memcpy(new, old, nr_probes * sizeof(struct tracepoint_func));
> - } else {
> - /* Copy higher priority probes ahead of the new probe */
> - memcpy(new, old, pos * sizeof(struct tracepoint_func));
> - /* Copy the rest after it. */
> - memcpy(new + pos + 1, old + pos,
> - (nr_probes - pos) * sizeof(struct tracepoint_func));
> + for (iter_probes = 0; old[iter_probes].func; iter_probes++) {
> + if (old[iter_probes].func == tp_stub_func)
> + continue; /* Skip stub functions. */
> + if (nr_new_probes != pos)
> + new[nr_new_probes] = old[iter_probes];
The problem is above, because we just skipped adding old[iter_probes].
> + nr_new_probes++;
I'm not sure this is any less confusing than my code. Because, I now have
to try and wrap my head around how to increment three different variables
instead of just two.
I agree that we can just make it one code, but I think I'll keep the
original logic, with slight modifications influenced by your code.
Instead of doing that "trick" that decrements nr_probes, I should increment
it if we are adding to the end (pos < 0):
if (pos < 0)
pos = nr_probes++;
/* nr_probes points to the end of the array */
} else { /* !old */
pos = 0;
nr_probes = 1; /* points to end of array */
}
new[pos] = *tp_func;
new[nr_probes].func = NULL;
> }
> } else
> pos = 0;
> new[pos] = *tp_func;
> - new[nr_probes + 1].func = NULL;
> + new[nr_new_probes + 1].func = NULL;
> *funcs = new;
> debug_print_probes(*funcs);
> return old;
> @@ -238,9 +219,9 @@ static void *func_remove(struct tracepoint_func **funcs,
> new = allocate_probes(nr_probes - nr_del + 1);
> if (new) {
> for (i = 0; old[i].func; i++)
> - if ((old[i].func != tp_func->func
> - || old[i].data != tp_func->data)
> - && old[i].func != tp_stub_func)
> + if ((old[i].func != tp_func->func ||
> + old[i].data != tp_func->data) &&
> + old[i].func != tp_stub_func)
> new[j++] = old[i];
> new[nr_probes - nr_del].func = NULL;
> *funcs = new;
> @@ -251,15 +232,8 @@ static void *func_remove(struct tracepoint_func **funcs,
> */
> for (i = 0; old[i].func; i++)
> if (old[i].func == tp_func->func &&
> - old[i].data == tp_func->data) {
> - old[i].func = tp_stub_func;
> - /* Set the prio to the next event. */
> - if (old[i + 1].func)
> - old[i].prio =
> - old[i + 1].prio;
> - else
> - old[i].prio = -1;
> - }
> + old[i].data == tp_func->data)
> + WRITE_ONCE(old[i].func, tp_stub_func);
I'll add this change too.
> *funcs = old;
> }
> }
Here's an updated patch:
(tested against tools/testing/selftests/ftrace/ftracetest)
Index: linux-trace.git/include/linux/tracepoint.h
===================================================================
--- linux-trace.git.orig/include/linux/tracepoint.h
+++ linux-trace.git/include/linux/tracepoint.h
@@ -309,7 +309,7 @@ static inline struct tracepoint *tracepo
rcu_dereference_raw((&__tracepoint_##_name)->funcs); \
if (it_func_ptr) { \
do { \
- it_func = (it_func_ptr)->func; \
+ it_func = READ_ONCE((it_func_ptr)->func); \
__data = (it_func_ptr)->data; \
((void(*)(void *, proto))(it_func))(__data, args); \
} while ((++it_func_ptr)->func); \
Index: linux-trace.git/kernel/tracepoint.c
===================================================================
--- linux-trace.git.orig/kernel/tracepoint.c
+++ linux-trace.git/kernel/tracepoint.c
@@ -136,9 +136,9 @@ func_add(struct tracepoint_func **funcs,
int prio)
{
struct tracepoint_func *old, *new;
- int nr_probes = 0;
- int stub_funcs = 0;
- int pos = -1;
+ int iter_probes; /* Iterate over old probe array. */
+ int nr_probes = 0; /* Counter for probes */
+ int pos = -1; /* New position */
if (WARN_ON(!tp_func->func))
return ERR_PTR(-EINVAL);
@@ -147,54 +147,39 @@ func_add(struct tracepoint_func **funcs,
old = *funcs;
if (old) {
/* (N -> N+1), (N != 0, 1) probes */
- for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
- /* Insert before probes of lower priority */
- if (pos < 0 && old[nr_probes].prio < prio)
- pos = nr_probes;
- if (old[nr_probes].func == tp_func->func &&
- old[nr_probes].data == tp_func->data)
+ for (iter_probes = 0; old[iter_probes].func; iter_probes++) {
+ if (old[iter_probes].func == tp_stub_func)
+ continue; /* Skip stub functions. */
+ if (old[iter_probes].func == tp_func->func &&
+ old[iter_probes].data == tp_func->data)
return ERR_PTR(-EEXIST);
- if (old[nr_probes].func == tp_stub_func)
- stub_funcs++;
+ nr_probes++;
}
}
- /* + 2 : one for new probe, one for NULL func - stub functions */
- new = allocate_probes(nr_probes + 2 - stub_funcs);
+ /* + 2 : one for new probe, one for NULL func */
+ new = allocate_probes(nr_probes + 2);
if (new == NULL)
return ERR_PTR(-ENOMEM);
if (old) {
- if (stub_funcs) {
- /* Need to copy one at a time to remove stubs */
- int probes = 0;
-
- pos = -1;
- for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
- if (old[nr_probes].func == tp_stub_func)
- continue;
- if (pos < 0 && old[nr_probes].prio < prio)
- pos = probes++;
- new[probes++] = old[nr_probes];
- }
- nr_probes = probes;
- if (pos < 0)
- pos = probes;
- else
- nr_probes--; /* Account for insertion */
-
- } else if (pos < 0) {
- pos = nr_probes;
- memcpy(new, old, nr_probes * sizeof(struct tracepoint_func));
- } else {
- /* Copy higher priority probes ahead of the new probe */
- memcpy(new, old, pos * sizeof(struct tracepoint_func));
- /* Copy the rest after it. */
- memcpy(new + pos + 1, old + pos,
- (nr_probes - pos) * sizeof(struct tracepoint_func));
+ pos = -1;
+ nr_probes = 0;
+ for (iter_probes = 0; old[iter_probes].func; iter_probes++) {
+ if (old[iter_probes].func == tp_stub_func)
+ continue;
+ /* Insert before probes of lower priority */
+ if (pos < 0 && old[iter_probes].prio < prio)
+ pos = nr_probes++;
+ new[nr_probes++] = old[iter_probes];
}
- } else
+ if (pos < 0)
+ pos = nr_probes++;
+ /* nr_probes now points to the end of the new array */
+ } else {
pos = 0;
+ nr_probes = 1; /* must point at end of array */
+ }
new[pos] = *tp_func;
- new[nr_probes + 1].func = NULL;
+ new[nr_probes].func = NULL;
*funcs = new;
debug_print_probes(*funcs);
return old;
@@ -237,11 +222,12 @@ static void *func_remove(struct tracepoi
/* + 1 for NULL */
new = allocate_probes(nr_probes - nr_del + 1);
if (new) {
- for (i = 0; old[i].func; i++)
- if ((old[i].func != tp_func->func
- || old[i].data != tp_func->data)
- && old[i].func != tp_stub_func)
+ for (i = 0; old[i].func; i++) {
+ if ((old[i].func != tp_func->func ||
+ old[i].data != tp_func->data) &&
+ old[i].func != tp_stub_func)
new[j++] = old[i];
+ }
new[nr_probes - nr_del].func = NULL;
*funcs = new;
} else {
@@ -249,17 +235,11 @@ static void *func_remove(struct tracepoi
* Failed to allocate, replace the old function
* with calls to tp_stub_func.
*/
- for (i = 0; old[i].func; i++)
+ for (i = 0; old[i].func; i++) {
if (old[i].func == tp_func->func &&
- old[i].data == tp_func->data) {
- old[i].func = tp_stub_func;
- /* Set the prio to the next event. */
- if (old[i + 1].func)
- old[i].prio =
- old[i + 1].prio;
- else
- old[i].prio = -1;
- }
+ old[i].data == tp_func->data)
+ WRITE_ONCE(old[i].func, tp_stub_func);
+ }
*funcs = old;
}
}
^ permalink raw reply [flat|nested] 22+ messages in thread
* [for-next][PATCH 15/15] kernel: trace: preemptirq_delay_test: add cpu affinity
2021-02-03 16:05 [for-next][PATCH 00/15] tracing: Updates for 5.12 Steven Rostedt
` (12 preceding siblings ...)
2021-02-03 16:05 ` [for-next][PATCH 14/15] tracepoint: Do not fail unregistering a probe due to memory failure Steven Rostedt
@ 2021-02-03 16:05 ` Steven Rostedt
13 siblings, 0 replies; 22+ messages in thread
From: Steven Rostedt @ 2021-02-03 16:05 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Song Chen
From: Song Chen <chensong_2000@189.cn>
The kernel thread executing test can run on any cpu, which might be
different cpu latency tracer is running on, as a result, the
big latency caused by preemptirq delay test can't be detected.
Therefore, the argument cpu_affinity is added to be passed to test,
ensure it's running on the same cpu with latency tracer.
e.g.
cyclictest -p 90 -m -c 0 -i 1000 -a 3
modprobe preemptirq_delay_test test_mode=preempt delay=500 \
burst_size=3 cpu_affinity=3
Link: https://lkml.kernel.org/r/1611797713-20965-1-git-send-email-chensong_2000@189.cn
Signed-off-by: Song Chen <chensong_2000@189.cn>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
kernel/trace/Kconfig | 4 ++++
kernel/trace/preemptirq_delay_test.c | 14 ++++++++++++++
2 files changed, 18 insertions(+)
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 4f976f8d9a38..799dbcfe65ad 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -886,6 +886,10 @@ config PREEMPTIRQ_DELAY_TEST
irq-disabled critical sections for 500us:
modprobe preemptirq_delay_test test_mode=irq delay=500 burst_size=3
+ What's more, if you want to attach the test on the cpu which the latency
+ tracer is running on, specify cpu_affinity=cpu_num at the end of the
+ command.
+
If unsure, say N
config SYNTH_EVENT_GEN_TEST
diff --git a/kernel/trace/preemptirq_delay_test.c b/kernel/trace/preemptirq_delay_test.c
index 312d1a0ca3b6..8c4ffd076162 100644
--- a/kernel/trace/preemptirq_delay_test.c
+++ b/kernel/trace/preemptirq_delay_test.c
@@ -21,13 +21,16 @@
static ulong delay = 100;
static char test_mode[12] = "irq";
static uint burst_size = 1;
+static int cpu_affinity = -1;
module_param_named(delay, delay, ulong, 0444);
module_param_string(test_mode, test_mode, 12, 0444);
module_param_named(burst_size, burst_size, uint, 0444);
+module_param_named(cpu_affinity, cpu_affinity, int, 0444);
MODULE_PARM_DESC(delay, "Period in microseconds (100 us default)");
MODULE_PARM_DESC(test_mode, "Mode of the test such as preempt, irq, or alternate (default irq)");
MODULE_PARM_DESC(burst_size, "The size of a burst (default 1)");
+MODULE_PARM_DESC(cpu_affinity, "Cpu num test is running on");
static struct completion done;
@@ -36,7 +39,9 @@ static struct completion done;
static void busy_wait(ulong time)
{
u64 start, end;
+
start = trace_clock_local();
+
do {
end = trace_clock_local();
if (kthread_should_stop())
@@ -47,6 +52,7 @@ static void busy_wait(ulong time)
static __always_inline void irqoff_test(void)
{
unsigned long flags;
+
local_irq_save(flags);
busy_wait(delay);
local_irq_restore(flags);
@@ -113,6 +119,14 @@ static int preemptirq_delay_run(void *data)
{
int i;
int s = MIN(burst_size, NR_TEST_FUNCS);
+ struct cpumask cpu_mask;
+
+ if (cpu_affinity > -1) {
+ cpumask_clear(&cpu_mask);
+ cpumask_set_cpu(cpu_affinity, &cpu_mask);
+ if (set_cpus_allowed_ptr(current, &cpu_mask))
+ pr_err("cpu_affinity:%d, failed\n", cpu_affinity);
+ }
for (i = 0; i < s; i++)
(testfuncs[i])(i);
--
2.29.2
^ permalink raw reply related [flat|nested] 22+ messages in thread