Part of the energy aware scheduler work is concerned by test and
measurement tools such as idlestat[1]. This relies on the ability to
trace wake-up events, mainly IRQs and IPIs.
While IRQs are already well instrumented with tracepoints, IPIs are rather
lacking on that front, and completely invisible on ARM.
This series defines generic IPI tracepoints and adds them to ARM and
ARM64. An attempt at adding them to X86 is also included for comments.
[1] https://wiki.linaro.org/WorkingGroups/PowerManagement/Resources/Tools/Idlestat
diffstat:
arch/arm/kernel/smp.c | 72 ++++++++++++++++++++------------
arch/arm64/kernel/smp.c | 67 +++++++++++++++++++-----------
arch/x86/kernel/smp.c | 16 ++++++++
include/trace/events/ipi.h | 89 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 192 insertions(+), 52 deletions(-)
The strings used to list IPIs in /proc/interrupts are reused for tracing
purposes.
While at it, the code is slightly cleaned up so the ipi_types array
indices are no longer offset by IPI_RESCHEDULE whose value is 0 anyway.
Signed-off-by: Nicolas Pitre <[email protected]>
---
arch/arm64/kernel/smp.c | 67 +++++++++++++++++++++++++++++++------------------
1 file changed, 42 insertions(+), 25 deletions(-)
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 40f38f46c8..78a5904994 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -50,6 +50,9 @@
#include <asm/tlbflush.h>
#include <asm/ptrace.h>
+#define CREATE_TRACE_POINTS
+#include <trace/events/ipi.h>
+
/*
* as from 2.5, kernels no longer have an init_tasks structure
* so we need some other way of telling a new secondary core
@@ -307,8 +310,6 @@ void __init smp_prepare_boot_cpu(void)
set_my_cpu_offset(per_cpu_offset(smp_processor_id()));
}
-static void (*smp_cross_call)(const struct cpumask *, unsigned int);
-
/*
* Enumerate the possible CPU set from the device tree and build the
* cpu logical map array containing MPIDR values related to logical
@@ -463,32 +464,19 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
}
}
+static void (*__smp_cross_call)(const struct cpumask *, unsigned int);
void __init set_smp_cross_call(void (*fn)(const struct cpumask *, unsigned int))
{
- smp_cross_call = fn;
-}
-
-void arch_send_call_function_ipi_mask(const struct cpumask *mask)
-{
- smp_cross_call(mask, IPI_CALL_FUNC);
-}
-
-void arch_send_call_function_single_ipi(int cpu)
-{
- smp_cross_call(cpumask_of(cpu), IPI_CALL_FUNC_SINGLE);
+ __smp_cross_call = fn;
}
-#ifdef CONFIG_IRQ_WORK
-void arch_irq_work_raise(void)
-{
- if (smp_cross_call)
- smp_cross_call(cpumask_of(smp_processor_id()), IPI_IRQ_WORK);
-}
+static const char *ipi_types[NR_IPI]
+#ifdef CONFIG_TRACING
+__tracepoint_string
#endif
-
-static const char *ipi_types[NR_IPI] = {
-#define S(x,s) [x - IPI_RESCHEDULE] = s
+= {
+#define S(x,s) [x] = s
S(IPI_RESCHEDULE, "Rescheduling interrupts"),
S(IPI_CALL_FUNC, "Function call interrupts"),
S(IPI_CALL_FUNC_SINGLE, "Single function call interrupts"),
@@ -497,12 +485,18 @@ static const char *ipi_types[NR_IPI] = {
S(IPI_IRQ_WORK, "IRQ work interrupts"),
};
+static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
+{
+ trace_ipi_raise(target, ipi_types[ipinr]);
+ __smp_cross_call(target, ipinr);
+}
+
void show_ipi_list(struct seq_file *p, int prec)
{
unsigned int cpu, i;
for (i = 0; i < NR_IPI; i++) {
- seq_printf(p, "%*s%u:%s", prec - 1, "IPI", i + IPI_RESCHEDULE,
+ seq_printf(p, "%*s%u:%s", prec - 1, "IPI", i,
prec >= 4 ? " " : "");
for_each_online_cpu(cpu)
seq_printf(p, "%10u ",
@@ -522,6 +516,24 @@ u64 smp_irq_stat_cpu(unsigned int cpu)
return sum;
}
+void arch_send_call_function_ipi_mask(const struct cpumask *mask)
+{
+ smp_cross_call(mask, IPI_CALL_FUNC);
+}
+
+void arch_send_call_function_single_ipi(int cpu)
+{
+ smp_cross_call(cpumask_of(cpu), IPI_CALL_FUNC_SINGLE);
+}
+
+#ifdef CONFIG_IRQ_WORK
+void arch_irq_work_raise(void)
+{
+ if (__smp_cross_call)
+ smp_cross_call(cpumask_of(smp_processor_id()), IPI_IRQ_WORK);
+}
+#endif
+
static DEFINE_RAW_SPINLOCK(stop_lock);
/*
@@ -553,8 +565,10 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
unsigned int cpu = smp_processor_id();
struct pt_regs *old_regs = set_irq_regs(regs);
- if (ipinr >= IPI_RESCHEDULE && ipinr < IPI_RESCHEDULE + NR_IPI)
- __inc_irq_stat(cpu, ipi_irqs[ipinr - IPI_RESCHEDULE]);
+ if ((unsigned)ipinr < NR_IPI) {
+ trace_ipi_entry(ipi_types[ipinr]);
+ __inc_irq_stat(cpu, ipi_irqs[ipinr]);
+ }
switch (ipinr) {
case IPI_RESCHEDULE:
@@ -599,6 +613,9 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
pr_crit("CPU%u: Unknown IPI message 0x%x\n", cpu, ipinr);
break;
}
+
+ if ((unsigned)ipinr < NR_IPI)
+ trace_ipi_exit(ipi_types[ipinr]);
set_irq_regs(old_regs);
}
--
1.8.4.108.g55ea5f6
The strings used to list IPIs in /proc/interrupts are reused for tracing
purposes.
Signed-off-by: Nicolas Pitre <[email protected]>
---
arch/arm/kernel/smp.c | 72 ++++++++++++++++++++++++++++++++-------------------
1 file changed, 45 insertions(+), 27 deletions(-)
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 7c4fada440..daedff319b 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -47,6 +47,9 @@
#include <asm/mach/arch.h>
#include <asm/mpu.h>
+#define CREATE_TRACE_POINTS
+#include <trace/events/ipi.h>
+
/*
* as from 2.5, kernels no longer have an init_tasks structure
* so we need some other way of telling a new secondary core
@@ -430,38 +433,19 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
}
}
-static void (*smp_cross_call)(const struct cpumask *, unsigned int);
+static void (*__smp_cross_call)(const struct cpumask *, unsigned int);
void __init set_smp_cross_call(void (*fn)(const struct cpumask *, unsigned int))
{
- if (!smp_cross_call)
- smp_cross_call = fn;
-}
-
-void arch_send_call_function_ipi_mask(const struct cpumask *mask)
-{
- smp_cross_call(mask, IPI_CALL_FUNC);
-}
-
-void arch_send_wakeup_ipi_mask(const struct cpumask *mask)
-{
- smp_cross_call(mask, IPI_WAKEUP);
-}
-
-void arch_send_call_function_single_ipi(int cpu)
-{
- smp_cross_call(cpumask_of(cpu), IPI_CALL_FUNC_SINGLE);
+ if (!__smp_cross_call)
+ __smp_cross_call = fn;
}
-#ifdef CONFIG_IRQ_WORK
-void arch_irq_work_raise(void)
-{
- if (is_smp())
- smp_cross_call(cpumask_of(smp_processor_id()), IPI_IRQ_WORK);
-}
+static const char *ipi_types[NR_IPI]
+#ifdef CONFIG_TRACING
+__tracepoint_string
#endif
-
-static const char *ipi_types[NR_IPI] = {
+= {
#define S(x,s) [x] = s
S(IPI_WAKEUP, "CPU wakeup interrupts"),
S(IPI_TIMER, "Timer broadcast interrupts"),
@@ -473,6 +457,12 @@ static const char *ipi_types[NR_IPI] = {
S(IPI_COMPLETION, "completion interrupts"),
};
+static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
+{
+ trace_ipi_raise(target, ipi_types[ipinr]);
+ __smp_cross_call(target, ipinr);
+}
+
void show_ipi_list(struct seq_file *p, int prec)
{
unsigned int cpu, i;
@@ -499,6 +489,29 @@ u64 smp_irq_stat_cpu(unsigned int cpu)
return sum;
}
+void arch_send_call_function_ipi_mask(const struct cpumask *mask)
+{
+ smp_cross_call(mask, IPI_CALL_FUNC);
+}
+
+void arch_send_wakeup_ipi_mask(const struct cpumask *mask)
+{
+ smp_cross_call(mask, IPI_WAKEUP);
+}
+
+void arch_send_call_function_single_ipi(int cpu)
+{
+ smp_cross_call(cpumask_of(cpu), IPI_CALL_FUNC_SINGLE);
+}
+
+#ifdef CONFIG_IRQ_WORK
+void arch_irq_work_raise(void)
+{
+ if (is_smp())
+ smp_cross_call(cpumask_of(smp_processor_id()), IPI_IRQ_WORK);
+}
+#endif
+
#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
void tick_broadcast(const struct cpumask *mask)
{
@@ -556,8 +569,10 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
unsigned int cpu = smp_processor_id();
struct pt_regs *old_regs = set_irq_regs(regs);
- if (ipinr < NR_IPI)
+ if ((unsigned)ipinr < NR_IPI) {
+ trace_ipi_entry(ipi_types[ipinr]);
__inc_irq_stat(cpu, ipi_irqs[ipinr]);
+ }
switch (ipinr) {
case IPI_WAKEUP:
@@ -612,6 +627,9 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
cpu, ipinr);
break;
}
+
+ if ((unsigned)ipinr < NR_IPI)
+ trace_ipi_exit(ipi_types[ipinr]);
set_irq_regs(old_regs);
}
--
1.8.4.108.g55ea5f6
On X86 there are already tracepoints for IRQ vectors through which IPIs
are handled. However this is highly X86 specific, and the IPI signaling
is not currently traced.
This is an attempt at adding generic IPI tracepoints to X86.
Signed-off-by: Nicolas Pitre <[email protected]>
---
arch/x86/kernel/smp.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
index be8e1bde07..e154d176cf 100644
--- a/arch/x86/kernel/smp.c
+++ b/arch/x86/kernel/smp.c
@@ -31,6 +31,12 @@
#include <asm/apic.h>
#include <asm/nmi.h>
#include <asm/trace/irq_vectors.h>
+
+#define CREATE_TRACE_POINTS
+#undef TRACE_INCLUDE_PATH
+#undef TRACE_INCLUDE_FILE
+#include <trace/events/ipi.h>
+
/*
* Some notes on x86 processor bugs affecting SMP operation:
*
@@ -124,11 +130,13 @@ static void native_smp_send_reschedule(int cpu)
WARN_ON(1);
return;
}
+ trace_ipi_raise(cpumask_of(cpu), tracepoint_string("RESCHEDULE"));
apic->send_IPI_mask(cpumask_of(cpu), RESCHEDULE_VECTOR);
}
void native_send_call_func_single_ipi(int cpu)
{
+ trace_ipi_raise(cpumask_of(cpu), tracepoint_string("CALL_FUNCTION_SINGLE"));
apic->send_IPI_mask(cpumask_of(cpu), CALL_FUNCTION_SINGLE_VECTOR);
}
@@ -136,6 +144,8 @@ void native_send_call_func_ipi(const struct cpumask *mask)
{
cpumask_var_t allbutself;
+ trace_ipi_raise(mask, tracepoint_string("CALL_FUNCTION"));
+
if (!alloc_cpumask_var(&allbutself, GFP_ATOMIC)) {
apic->send_IPI_mask(mask, CALL_FUNCTION_VECTOR);
return;
@@ -252,8 +262,10 @@ finish:
*/
static inline void __smp_reschedule_interrupt(void)
{
+ trace_ipi_entry(tracepoint_string("RESCHEDULE"));
inc_irq_stat(irq_resched_count);
scheduler_ipi();
+ trace_ipi_exit(tracepoint_string("RESCHEDULE"));
}
__visible void smp_reschedule_interrupt(struct pt_regs *regs)
@@ -291,8 +303,10 @@ __visible void smp_trace_reschedule_interrupt(struct pt_regs *regs)
static inline void __smp_call_function_interrupt(void)
{
+ trace_ipi_entry(tracepoint_string("CALL_FUNCTION"));
generic_smp_call_function_interrupt();
inc_irq_stat(irq_call_count);
+ trace_ipi_exit(tracepoint_string("CALL_FUNCTION"));
}
__visible void smp_call_function_interrupt(struct pt_regs *regs)
@@ -313,8 +327,10 @@ __visible void smp_trace_call_function_interrupt(struct pt_regs *regs)
static inline void __smp_call_function_single_interrupt(void)
{
+ trace_ipi_entry(tracepoint_string("CALL_FUNCTION_SINGLE"));
generic_smp_call_function_single_interrupt();
inc_irq_stat(irq_call_count);
+ trace_ipi_exit(tracepoint_string("CALL_FUNCTION_SINGLE"));
}
__visible void smp_call_function_single_interrupt(struct pt_regs *regs)
--
1.8.4.108.g55ea5f6
The Inter Processor Interrupt is used to make another processor do a
specific action such as rescheduling tasks, signal a timer event or
execute something in another CPU's context. IRQs are already traceable
but IPIs were not. Tracing them is useful for monitoring IPI latency,
or to verify when they are the source of CPU wake-ups with power
management implications.
Three trace hooks are defined: ipi_raise, ipi_entry and ipi_exit. To make
them portable, a string is used to identify them and correlate related
events. Additionally, ipi_raise records a bitmask representing targeted
CPUs.
Signed-off-by: Nicolas Pitre <[email protected]>
---
include/trace/events/ipi.h | 89 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 89 insertions(+)
create mode 100644 include/trace/events/ipi.h
diff --git a/include/trace/events/ipi.h b/include/trace/events/ipi.h
new file mode 100644
index 0000000000..834a7362a6
--- /dev/null
+++ b/include/trace/events/ipi.h
@@ -0,0 +1,89 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM ipi
+
+#if !defined(_TRACE_IPI_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_IPI_H
+
+#include <linux/tracepoint.h>
+
+/**
+ * ipi_raise - called when a smp cross call is made
+ *
+ * @mask: mask of recipient CPUs for the IPI
+ * @reason: string identifying the IPI purpose
+ *
+ * It is necessary for @reason to be a static string declared with
+ * __tracepoint_string.
+ */
+TRACE_EVENT(ipi_raise,
+
+ TP_PROTO(const struct cpumask *mask, const char *reason),
+
+ TP_ARGS(mask, reason),
+
+ TP_STRUCT__entry(
+ __bitmask(target_cpus, nr_cpumask_bits)
+ __field(const char *, reason)
+ ),
+
+ TP_fast_assign(
+ __assign_bitmask(target_cpus, cpumask_bits(mask), nr_cpumask_bits);
+ __entry->reason = reason;
+ ),
+
+ TP_printk("target_mask=%s (%s)", __get_bitmask(target_cpus), __entry->reason)
+);
+
+DECLARE_EVENT_CLASS(ipi_handler,
+
+ TP_PROTO(const char *reason),
+
+ TP_ARGS(reason),
+
+ TP_STRUCT__entry(
+ __field(const char *, reason)
+ ),
+
+ TP_fast_assign(
+ __entry->reason = reason;
+ ),
+
+ TP_printk("(%s)", __entry->reason)
+);
+
+/**
+ * ipi_entry - called immediately before the IPI handler
+ *
+ * @reason: string identifying the IPI purpose
+ *
+ * It is necessary for @reason to be a static string declared with
+ * __tracepoint_string, ideally the same as used with trace_ipi_raise
+ * for that IPI.
+ */
+DEFINE_EVENT(ipi_handler, ipi_entry,
+
+ TP_PROTO(const char *reason),
+
+ TP_ARGS(reason)
+);
+
+/**
+ * ipi_exit - called immediately after the IPI handler returns
+ *
+ * @reason: string identifying the IPI purpose
+ *
+ * It is necessary for @reason to be a static string declared with
+ * __tracepoint_string, ideally the same as used with trace_ipi_raise for
+ * that IPI.
+ */
+DEFINE_EVENT(ipi_handler, ipi_exit,
+
+ TP_PROTO(const char *reason),
+
+ TP_ARGS(reason)
+);
+
+#endif /* _TRACE_IPI_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
--
1.8.4.108.g55ea5f6
On Fri, 18 Jul 2014 01:18:53 -0400
Nicolas Pitre <[email protected]> wrote:
> -#ifdef CONFIG_IRQ_WORK
> -void arch_irq_work_raise(void)
> -{
> - if (is_smp())
> - smp_cross_call(cpumask_of(smp_processor_id()), IPI_IRQ_WORK);
> -}
> +static const char *ipi_types[NR_IPI]
> +#ifdef CONFIG_TRACING
> +__tracepoint_string
> #endif
Oh, this is ugly. I should probably add a define when !CONFIG_TRACING
is set. Something like:
#ifdef CONFIG_TRACING
...
#else
# define __tracepoint_string
#endif
Such that users of __tracepoint_string don't need to add ugly ifdefs in
the code.
If you want to add that to ftrace_event.h to this series, I'll ack it.
-- Steve
> -
> -static const char *ipi_types[NR_IPI] = {
> += {
> #define S(x,s) [x] = s
> S(IPI_WAKEUP, "CPU wakeup interrupts"),
> S(IPI_TIMER, "Timer broadcast interrupts"),
> @@ -473,6 +457,12 @@ static const char *ipi_types[NR_IPI] = {
> S(IPI_COMPLETION, "completion interrupts"),
> };
>
On Fri, 18 Jul 2014 01:18:55 -0400
Nicolas Pitre <[email protected]> wrote:
> diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
> index be8e1bde07..e154d176cf 100644
> --- a/arch/x86/kernel/smp.c
> +++ b/arch/x86/kernel/smp.c
> @@ -31,6 +31,12 @@
> #include <asm/apic.h>
> #include <asm/nmi.h>
> #include <asm/trace/irq_vectors.h>
> +
> +#define CREATE_TRACE_POINTS
> +#undef TRACE_INCLUDE_PATH
> +#undef TRACE_INCLUDE_FILE
I'm curious to why you added the #undefs. I wouldn't think they were
needed.
-- Steve
> +#include <trace/events/ipi.h>
> +
> /*
> * Some notes on x86 processor bugs affecting SMP operation:
> *
On Fri, 18 Jul 2014, Steven Rostedt wrote:
> On Fri, 18 Jul 2014 01:18:53 -0400
> Nicolas Pitre <[email protected]> wrote:
>
>
> > -#ifdef CONFIG_IRQ_WORK
> > -void arch_irq_work_raise(void)
> > -{
> > - if (is_smp())
> > - smp_cross_call(cpumask_of(smp_processor_id()), IPI_IRQ_WORK);
> > -}
> > +static const char *ipi_types[NR_IPI]
> > +#ifdef CONFIG_TRACING
> > +__tracepoint_string
> > #endif
>
> Oh, this is ugly. I should probably add a define when !CONFIG_TRACING
> is set. Something like:
>
> #ifdef CONFIG_TRACING
> ...
> #else
> # define __tracepoint_string
> #endif
>
> Such that users of __tracepoint_string don't need to add ugly ifdefs in
> the code.
>
> If you want to add that to ftrace_event.h to this series, I'll ack it.
Here's the patch I have at the head of the series now, with the above
ugliness changed to an unconditional __tracepoint_string attribute.
From: Nicolas Pitre <[email protected]>
Date: Fri, 18 Jul 2014 16:34:39 -0400
Subject: [PATCH] trace: don't refer __tracepoint_string to an undefined linker section
When CONFIG_TRACING is not set, the linker script doesn't specify any
__tracepoint_str section. Let those __tracepoint_string marked strings
live in the default rodata section in that case.
Signed-off-by: Nicolas Pitre <[email protected]>
diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index cff3106ffe..d6346607e4 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -606,7 +606,11 @@ do { \
static const char *___tp_str __tracepoint_string = str; \
___tp_str; \
})
+#ifdef CONFIG_TRACING
#define __tracepoint_string __attribute__((section("__tracepoint_str")))
+#else
+#define __tracepoint_string
+#endif
#ifdef CONFIG_PERF_EVENTS
struct perf_event;
On Fri, 18 Jul 2014, Steven Rostedt wrote:
> On Fri, 18 Jul 2014 01:18:55 -0400
> Nicolas Pitre <[email protected]> wrote:
>
>
> > diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
> > index be8e1bde07..e154d176cf 100644
> > --- a/arch/x86/kernel/smp.c
> > +++ b/arch/x86/kernel/smp.c
> > @@ -31,6 +31,12 @@
> > #include <asm/apic.h>
> > #include <asm/nmi.h>
> > #include <asm/trace/irq_vectors.h>
> > +
> > +#define CREATE_TRACE_POINTS
> > +#undef TRACE_INCLUDE_PATH
> > +#undef TRACE_INCLUDE_FILE
>
> I'm curious to why you added the #undefs. I wouldn't think they were
> needed.
They are needed because asm/trace/irq_vectors.h included prior that
point defines them for itself and that makes the inclusion of
trace/events/ipi.h fail.
> -- Steve
>
> > +#include <trace/events/ipi.h>
> > +
> > /*
> > * Some notes on x86 processor bugs affecting SMP operation:
> > *
>
>
On Fri, 18 Jul 2014 16:55:42 -0400 (EDT)
Nicolas Pitre <[email protected]> wrote:
>
> Here's the patch I have at the head of the series now, with the above
> ugliness changed to an unconditional __tracepoint_string attribute.
>
I was thinking of something like this. Feel free to add this to your
series.
-- Steve
From: Steven Rostedt <[email protected]>
Subject: [PATCH] tracing: Do not do anything special with tracepoint_string when tracing is disabled
When CONFIG_TRACING is not enabled, there's no reason to save the trace
strings either by the linker or as a static variable that can be
referenced later. Simply pass back the string that is given to
tracepoint_string().
Signed-off-by: Steven Rostedt <[email protected]>
---
diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index cff3106..b296363 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -574,6 +574,7 @@ do { \
__trace_printk(ip, fmt, ##args); \
} while (0)
+#ifdef CONFIG_TRACING
/**
* tracepoint_string - register constant persistent string to trace system
* @str - a constant persistent string that will be referenced in tracepoints
@@ -607,6 +608,15 @@ do { \
___tp_str; \
})
#define __tracepoint_string __attribute__((section("__tracepoint_str")))
+#else
+/*
+ * tracepoint_string() is used to save the string address for userspace
+ * tracing tools. When tracing isn't configured, there's no need to save
+ * anything.
+ */
+# define tracepoint_string(str) str
+# define __tracepoint_string
+#endif
#ifdef CONFIG_PERF_EVENTS
struct perf_event;
On Fri, 18 Jul 2014, Steven Rostedt wrote:
> On Fri, 18 Jul 2014 16:55:42 -0400 (EDT)
> Nicolas Pitre <[email protected]> wrote:
>
> >
> > Here's the patch I have at the head of the series now, with the above
> > ugliness changed to an unconditional __tracepoint_string attribute.
> >
>
> I was thinking of something like this. Feel free to add this to your
> series.
OK. Same end result, but much clearer. Thanks.
Any comments / ACKs on the other patches? I'd like to see 1/4 to 3/4
(and your patch) merged upstream during the next window. 4/4 is up for
debate.
> -- Steve
>
> From: Steven Rostedt <[email protected]>
> Subject: [PATCH] tracing: Do not do anything special with tracepoint_string when tracing is disabled
>
> When CONFIG_TRACING is not enabled, there's no reason to save the trace
> strings either by the linker or as a static variable that can be
> referenced later. Simply pass back the string that is given to
> tracepoint_string().
>
> Signed-off-by: Steven Rostedt <[email protected]>
> ---
> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> index cff3106..b296363 100644
> --- a/include/linux/ftrace_event.h
> +++ b/include/linux/ftrace_event.h
> @@ -574,6 +574,7 @@ do { \
> __trace_printk(ip, fmt, ##args); \
> } while (0)
>
> +#ifdef CONFIG_TRACING
> /**
> * tracepoint_string - register constant persistent string to trace system
> * @str - a constant persistent string that will be referenced in tracepoints
> @@ -607,6 +608,15 @@ do { \
> ___tp_str; \
> })
> #define __tracepoint_string __attribute__((section("__tracepoint_str")))
> +#else
> +/*
> + * tracepoint_string() is used to save the string address for userspace
> + * tracing tools. When tracing isn't configured, there's no need to save
> + * anything.
> + */
> +# define tracepoint_string(str) str
> +# define __tracepoint_string
> +#endif
>
> #ifdef CONFIG_PERF_EVENTS
> struct perf_event;
>
>
On Fri, 18 Jul 2014 22:55:12 -0400 (EDT)
Nicolas Pitre <[email protected]> wrote:
> Any comments / ACKs on the other patches? I'd like to see 1/4 to 3/4
> (and your patch) merged upstream during the next window. 4/4 is up for
> debate.
You can add my Acked-by for patches 1,2 and 3, after the clean up of
the #ifdefs there.
I still don't like the fact that you need to add the #undef in patch 4.
I'm looking at other ways to fix that. I tried to do a few different
clean ups in the tracing infrastructure, but it seems that it may be
required. The other method I may try is to move the #undefs into the
ipi.h header itself.
I'll play more with this on Monday.
-- Steve
On 18 July 2014 23:22, Steven Rostedt <[email protected]> wrote:
> On Fri, 18 Jul 2014 16:55:42 -0400 (EDT)
> Nicolas Pitre <[email protected]> wrote:
>
>>
>> Here's the patch I have at the head of the series now, with the above
>> ugliness changed to an unconditional __tracepoint_string attribute.
>>
>
> I was thinking of something like this. Feel free to add this to your
> series.
>
> -- Steve
>
Nico,
If this patch addresses the issue where 3 RCU related tracepoint
strings turn up /after/ _edata on !CONFIG_TRACING, there is already a
patch queued up here
http://marc.info/?l=linux-kernel&m=140518452623148&w=2
As far as In know, these were the only occurrences using a __used
modifier, which is why they weren't dropped by the compiler in the
!CONFIG_TRACING case.
Regards,
Ard.
> From: Steven Rostedt <[email protected]>
> Subject: [PATCH] tracing: Do not do anything special with tracepoint_string when tracing is disabled
>
> When CONFIG_TRACING is not enabled, there's no reason to save the trace
> strings either by the linker or as a static variable that can be
> referenced later. Simply pass back the string that is given to
> tracepoint_string().
>
> Signed-off-by: Steven Rostedt <[email protected]>
> ---
> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> index cff3106..b296363 100644
> --- a/include/linux/ftrace_event.h
> +++ b/include/linux/ftrace_event.h
> @@ -574,6 +574,7 @@ do { \
> __trace_printk(ip, fmt, ##args); \
> } while (0)
>
> +#ifdef CONFIG_TRACING
> /**
> * tracepoint_string - register constant persistent string to trace system
> * @str - a constant persistent string that will be referenced in tracepoints
> @@ -607,6 +608,15 @@ do { \
> ___tp_str; \
> })
> #define __tracepoint_string __attribute__((section("__tracepoint_str")))
> +#else
> +/*
> + * tracepoint_string() is used to save the string address for userspace
> + * tracing tools. When tracing isn't configured, there's no need to save
> + * anything.
> + */
> +# define tracepoint_string(str) str
> +# define __tracepoint_string
> +#endif
>
> #ifdef CONFIG_PERF_EVENTS
> struct perf_event;
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Sat, 19 Jul 2014 21:10:37 +0200
Ard Biesheuvel <[email protected]> wrote:
> On 18 July 2014 23:22, Steven Rostedt <[email protected]> wrote:
> > On Fri, 18 Jul 2014 16:55:42 -0400 (EDT)
> > Nicolas Pitre <[email protected]> wrote:
> >
> >>
> >> Here's the patch I have at the head of the series now, with the above
> >> ugliness changed to an unconditional __tracepoint_string attribute.
> >>
> >
> > I was thinking of something like this. Feel free to add this to your
> > series.
> >
> > -- Steve
> >
>
> Nico,
>
> If this patch addresses the issue where 3 RCU related tracepoint
> strings turn up /after/ _edata on !CONFIG_TRACING, there is already a
> patch queued up here
>
> http://marc.info/?l=linux-kernel&m=140518452623148&w=2
>
> As far as In know, these were the only occurrences using a __used
> modifier, which is why they weren't dropped by the compiler in the
> !CONFIG_TRACING case.
>
Ard,
Similar but different problem. Nicolas's problem was with new use cases
for tracepoint_string. My patch fixes the issue for the general case.
-- Steve
On 19 July 2014 22:28, Steven Rostedt <[email protected]> wrote:
> On Sat, 19 Jul 2014 21:10:37 +0200
> Ard Biesheuvel <[email protected]> wrote:
>
>> On 18 July 2014 23:22, Steven Rostedt <[email protected]> wrote:
>> > On Fri, 18 Jul 2014 16:55:42 -0400 (EDT)
>> > Nicolas Pitre <[email protected]> wrote:
>> >
>> >>
>> >> Here's the patch I have at the head of the series now, with the above
>> >> ugliness changed to an unconditional __tracepoint_string attribute.
>> >>
>> >
>> > I was thinking of something like this. Feel free to add this to your
>> > series.
>> >
>> > -- Steve
>> >
>>
>> Nico,
>>
>> If this patch addresses the issue where 3 RCU related tracepoint
>> strings turn up /after/ _edata on !CONFIG_TRACING, there is already a
>> patch queued up here
>>
>> http://marc.info/?l=linux-kernel&m=140518452623148&w=2
>>
>> As far as In know, these were the only occurrences using a __used
>> modifier, which is why they weren't dropped by the compiler in the
>> !CONFIG_TRACING case.
>>
>
> Ard,
>
> Similar but different problem. Nicolas's problem was with new use cases
> for tracepoint_string. My patch fixes the issue for the general case.
>
OK, so if the general case has been fixed, perhaps we should ask Paul
to drop my patch?
--
Ard.
On Sat, 19 Jul 2014 22:50:16 +0200
Ard Biesheuvel <[email protected]> wrote:
> OK, so if the general case has been fixed, perhaps we should ask Paul
> to drop my patch?
>
No, for a few reasons. One, this patch still needs to get in to fix the
problem for RCU. Two, RCU basically open codes the creation of the
string, thus this wont solve it for RCU.
-- Steve
On Sat, 19 Jul 2014, Ard Biesheuvel wrote:
> On 18 July 2014 23:22, Steven Rostedt <[email protected]> wrote:
> > On Fri, 18 Jul 2014 16:55:42 -0400 (EDT)
> > Nicolas Pitre <[email protected]> wrote:
> >
> >>
> >> Here's the patch I have at the head of the series now, with the above
> >> ugliness changed to an unconditional __tracepoint_string attribute.
> >>
> >
> > I was thinking of something like this. Feel free to add this to your
> > series.
> >
> > -- Steve
> >
>
> Nico,
>
> If this patch addresses the issue where 3 RCU related tracepoint
> strings turn up /after/ _edata on !CONFIG_TRACING, there is already a
> patch queued up here
>
> http://marc.info/?l=linux-kernel&m=140518452623148&w=2
No, that doesn't help my case. Please see the initial comment from
Steven in this thread and you'll understand.
Nicolas
On Fri, 2014-07-18 at 01:18 -0400, Nicolas Pitre wrote:
> On X86 there are already tracepoints for IRQ vectors through which IPIs
> are handled. However this is highly X86 specific, and the IPI signaling
> is not currently traced.
>
> This is an attempt at adding generic IPI tracepoints to X86.
I welcome this, and fwiw have been trying out this patch. One thing I
would like to see, but due to overhead would probably be better suited
in userspace (trace-cmd, maybe?), is a more packed description of the
IPI. Ie: unifying ipi_init and ipi_exit and show overall cost of the
IPI.
Thanks,
Davidlohr
On Fri, Jul 18, 2014 at 06:18:54AM +0100, Nicolas Pitre wrote:
> The strings used to list IPIs in /proc/interrupts are reused for tracing
> purposes.
>
> While at it, the code is slightly cleaned up so the ipi_types array
> indices are no longer offset by IPI_RESCHEDULE whose value is 0 anyway.
>
> Signed-off-by: Nicolas Pitre <[email protected]>
Acked-by: Catalin Marinas <[email protected]>
On Sun, 20 Jul 2014, Davidlohr Bueso wrote:
> On Fri, 2014-07-18 at 01:18 -0400, Nicolas Pitre wrote:
> > On X86 there are already tracepoints for IRQ vectors through which IPIs
> > are handled. However this is highly X86 specific, and the IPI signaling
> > is not currently traced.
> >
> > This is an attempt at adding generic IPI tracepoints to X86.
>
> I welcome this, and fwiw have been trying out this patch. One thing I
> would like to see, but due to overhead would probably be better suited
> in userspace (trace-cmd, maybe?), is a more packed description of the
> IPI. Ie: unifying ipi_init and ipi_exit and show overall cost of the
> IPI.
That's best suited for the tool consuming the log in user space. The
same is done with IRQ events already.
Nicolas
On Fri, 18 Jul 2014 16:59:50 -0400 (EDT)
Nicolas Pitre <[email protected]> wrote:
> On Fri, 18 Jul 2014, Steven Rostedt wrote:
>
> > On Fri, 18 Jul 2014 01:18:55 -0400
> > Nicolas Pitre <[email protected]> wrote:
> >
> >
> > > diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
> > > index be8e1bde07..e154d176cf 100644
> > > --- a/arch/x86/kernel/smp.c
> > > +++ b/arch/x86/kernel/smp.c
> > > @@ -31,6 +31,12 @@
> > > #include <asm/apic.h>
> > > #include <asm/nmi.h>
> > > #include <asm/trace/irq_vectors.h>
> > > +
> > > +#define CREATE_TRACE_POINTS
> > > +#undef TRACE_INCLUDE_PATH
> > > +#undef TRACE_INCLUDE_FILE
> >
> > I'm curious to why you added the #undefs. I wouldn't think they were
> > needed.
>
> They are needed because asm/trace/irq_vectors.h included prior that
> point defines them for itself and that makes the inclusion of
> trace/events/ipi.h fail.
>
Bah, I tried to get rid of the need for those, but it seems that the
macro magic is getting a bit out of hand. I need a new macro magic
hat :-p
What you got here will have to do.
-- Steve
On 07/18/2014 07:18 AM, Nicolas Pitre wrote:
> The Inter Processor Interrupt is used to make another processor do a
> specific action such as rescheduling tasks, signal a timer event or
> execute something in another CPU's context. IRQs are already traceable
> but IPIs were not. Tracing them is useful for monitoring IPI latency,
> or to verify when they are the source of CPU wake-ups with power
> management implications.
>
> Three trace hooks are defined: ipi_raise, ipi_entry and ipi_exit. To make
> them portable, a string is used to identify them and correlate related
> events. Additionally, ipi_raise records a bitmask representing targeted
> CPUs.
>
> Signed-off-by: Nicolas Pitre <[email protected]>
Acked-by: Daniel Lezcano <[email protected]>
> ---
> include/trace/events/ipi.h | 89 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 89 insertions(+)
> create mode 100644 include/trace/events/ipi.h
>
> diff --git a/include/trace/events/ipi.h b/include/trace/events/ipi.h
> new file mode 100644
> index 0000000000..834a7362a6
> --- /dev/null
> +++ b/include/trace/events/ipi.h
> @@ -0,0 +1,89 @@
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM ipi
> +
> +#if !defined(_TRACE_IPI_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_IPI_H
> +
> +#include <linux/tracepoint.h>
> +
> +/**
> + * ipi_raise - called when a smp cross call is made
> + *
> + * @mask: mask of recipient CPUs for the IPI
> + * @reason: string identifying the IPI purpose
> + *
> + * It is necessary for @reason to be a static string declared with
> + * __tracepoint_string.
> + */
> +TRACE_EVENT(ipi_raise,
> +
> + TP_PROTO(const struct cpumask *mask, const char *reason),
> +
> + TP_ARGS(mask, reason),
> +
> + TP_STRUCT__entry(
> + __bitmask(target_cpus, nr_cpumask_bits)
> + __field(const char *, reason)
> + ),
> +
> + TP_fast_assign(
> + __assign_bitmask(target_cpus, cpumask_bits(mask), nr_cpumask_bits);
> + __entry->reason = reason;
> + ),
> +
> + TP_printk("target_mask=%s (%s)", __get_bitmask(target_cpus), __entry->reason)
> +);
> +
> +DECLARE_EVENT_CLASS(ipi_handler,
> +
> + TP_PROTO(const char *reason),
> +
> + TP_ARGS(reason),
> +
> + TP_STRUCT__entry(
> + __field(const char *, reason)
> + ),
> +
> + TP_fast_assign(
> + __entry->reason = reason;
> + ),
> +
> + TP_printk("(%s)", __entry->reason)
> +);
> +
> +/**
> + * ipi_entry - called immediately before the IPI handler
> + *
> + * @reason: string identifying the IPI purpose
> + *
> + * It is necessary for @reason to be a static string declared with
> + * __tracepoint_string, ideally the same as used with trace_ipi_raise
> + * for that IPI.
> + */
> +DEFINE_EVENT(ipi_handler, ipi_entry,
> +
> + TP_PROTO(const char *reason),
> +
> + TP_ARGS(reason)
> +);
> +
> +/**
> + * ipi_exit - called immediately after the IPI handler returns
> + *
> + * @reason: string identifying the IPI purpose
> + *
> + * It is necessary for @reason to be a static string declared with
> + * __tracepoint_string, ideally the same as used with trace_ipi_raise for
> + * that IPI.
> + */
> +DEFINE_EVENT(ipi_handler, ipi_exit,
> +
> + TP_PROTO(const char *reason),
> +
> + TP_ARGS(reason)
> +);
> +
> +#endif /* _TRACE_IPI_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On Mon, 21 Jul 2014, Steven Rostedt wrote:
> On Fri, 18 Jul 2014 16:59:50 -0400 (EDT)
> Nicolas Pitre <[email protected]> wrote:
>
> > On Fri, 18 Jul 2014, Steven Rostedt wrote:
> >
> > > On Fri, 18 Jul 2014 01:18:55 -0400
> > > Nicolas Pitre <[email protected]> wrote:
> > >
> > >
> > > > diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
> > > > index be8e1bde07..e154d176cf 100644
> > > > --- a/arch/x86/kernel/smp.c
> > > > +++ b/arch/x86/kernel/smp.c
> > > > @@ -31,6 +31,12 @@
> > > > #include <asm/apic.h>
> > > > #include <asm/nmi.h>
> > > > #include <asm/trace/irq_vectors.h>
> > > > +
> > > > +#define CREATE_TRACE_POINTS
> > > > +#undef TRACE_INCLUDE_PATH
> > > > +#undef TRACE_INCLUDE_FILE
> > >
> > > I'm curious to why you added the #undefs. I wouldn't think they were
> > > needed.
> >
> > They are needed because asm/trace/irq_vectors.h included prior that
> > point defines them for itself and that makes the inclusion of
> > trace/events/ipi.h fail.
> >
>
> Bah, I tried to get rid of the need for those, but it seems that the
> macro magic is getting a bit out of hand. I need a new macro magic
> hat :-p
>
> What you got here will have to do.
OK.
Now the real question: should I submit it for mainline? I'm a little
bothered by the fact that all exception vectors already have tracepoints
of their own, albeit deeply tied to X86 nomenclature. But nothing that
relates to IPI sources. This patch fixes that by adding some
tracepoints alongside existing ones which may go a long way in making
their instrumentation with generic (arch independent) tools.
What do people think?
Nicolas
@Russell: Can I have your ACK on this patch before I send the series
upstream? Here's the revised version accounting for the changes the
discussion in this thread brought about
Author: Nicolas Pitre <[email protected]>
Date: Fri Jul 18 00:07:05 2014 -0400
ARM: add IPI tracepoints
The strings used to list IPIs in /proc/interrupts are reused for tracing
purposes.
While at it, prevent a negative ipinr from escaping the range check
in handle_IPI().
Signed-off-by: Nicolas Pitre <[email protected]>
Acked-by: Steven Rostedt <[email protected]>
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 7c4fada440..9388a3d479 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -47,6 +47,9 @@
#include <asm/mach/arch.h>
#include <asm/mpu.h>
+#define CREATE_TRACE_POINTS
+#include <trace/events/ipi.h>
+
/*
* as from 2.5, kernels no longer have an init_tasks structure
* so we need some other way of telling a new secondary core
@@ -430,38 +433,15 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
}
}
-static void (*smp_cross_call)(const struct cpumask *, unsigned int);
+static void (*__smp_cross_call)(const struct cpumask *, unsigned int);
void __init set_smp_cross_call(void (*fn)(const struct cpumask *, unsigned int))
{
- if (!smp_cross_call)
- smp_cross_call = fn;
-}
-
-void arch_send_call_function_ipi_mask(const struct cpumask *mask)
-{
- smp_cross_call(mask, IPI_CALL_FUNC);
-}
-
-void arch_send_wakeup_ipi_mask(const struct cpumask *mask)
-{
- smp_cross_call(mask, IPI_WAKEUP);
-}
-
-void arch_send_call_function_single_ipi(int cpu)
-{
- smp_cross_call(cpumask_of(cpu), IPI_CALL_FUNC_SINGLE);
+ if (!__smp_cross_call)
+ __smp_cross_call = fn;
}
-#ifdef CONFIG_IRQ_WORK
-void arch_irq_work_raise(void)
-{
- if (is_smp())
- smp_cross_call(cpumask_of(smp_processor_id()), IPI_IRQ_WORK);
-}
-#endif
-
-static const char *ipi_types[NR_IPI] = {
+static const char *ipi_types[NR_IPI] __tracepoint_string = {
#define S(x,s) [x] = s
S(IPI_WAKEUP, "CPU wakeup interrupts"),
S(IPI_TIMER, "Timer broadcast interrupts"),
@@ -473,6 +453,12 @@ static const char *ipi_types[NR_IPI] = {
S(IPI_COMPLETION, "completion interrupts"),
};
+static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
+{
+ trace_ipi_raise(target, ipi_types[ipinr]);
+ __smp_cross_call(target, ipinr);
+}
+
void show_ipi_list(struct seq_file *p, int prec)
{
unsigned int cpu, i;
@@ -499,6 +485,29 @@ u64 smp_irq_stat_cpu(unsigned int cpu)
return sum;
}
+void arch_send_call_function_ipi_mask(const struct cpumask *mask)
+{
+ smp_cross_call(mask, IPI_CALL_FUNC);
+}
+
+void arch_send_wakeup_ipi_mask(const struct cpumask *mask)
+{
+ smp_cross_call(mask, IPI_WAKEUP);
+}
+
+void arch_send_call_function_single_ipi(int cpu)
+{
+ smp_cross_call(cpumask_of(cpu), IPI_CALL_FUNC_SINGLE);
+}
+
+#ifdef CONFIG_IRQ_WORK
+void arch_irq_work_raise(void)
+{
+ if (is_smp())
+ smp_cross_call(cpumask_of(smp_processor_id()), IPI_IRQ_WORK);
+}
+#endif
+
#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
void tick_broadcast(const struct cpumask *mask)
{
@@ -556,8 +565,10 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
unsigned int cpu = smp_processor_id();
struct pt_regs *old_regs = set_irq_regs(regs);
- if (ipinr < NR_IPI)
+ if ((unsigned)ipinr < NR_IPI) {
+ trace_ipi_entry(ipi_types[ipinr]);
__inc_irq_stat(cpu, ipi_irqs[ipinr]);
+ }
switch (ipinr) {
case IPI_WAKEUP:
@@ -612,6 +623,9 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
cpu, ipinr);
break;
}
+
+ if ((unsigned)ipinr < NR_IPI)
+ trace_ipi_exit(ipi_types[ipinr]);
set_irq_regs(old_regs);
}