| Message ID | 4C724298.4050509@jp.fujitsu.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Aug 23, 2010 at 06:42:48PM +0900, Koki Sanagi wrote: > From: Lai Jiangshan <laijs@cn.fujitsu.com> > > Add a tracepoint for tracing when softirq action is raised. > > It and the existed tracepoints complete softirq's tracepoints: > softirq_raise, softirq_entry and softirq_exit. > > And when this tracepoint is used in combination with > the softirq_entry tracepoint we can determine > the softirq raise latency. > > Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> > Acked-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> > Acked-by: Frederic Weisbecker <fweisbec@gmail.com> > > [ factorize softirq events with DECLARE_EVENT_CLASS ] > Signed-off-by: Koki Sanagi <sanagi.koki@jp.fujitsu.com> > --- > include/linux/interrupt.h | 8 +++++++- > include/trace/events/irq.h | 26 ++++++++++++++++++++++++-- > 2 files changed, 31 insertions(+), 3 deletions(-) > > diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h > index a0384a4..d3e8e90 100644 > --- a/include/linux/interrupt.h > +++ b/include/linux/interrupt.h > @@ -18,6 +18,7 @@ > #include <asm/atomic.h> > #include <asm/ptrace.h> > #include <asm/system.h> > +#include <trace/events/irq.h> > > /* > * These correspond to the IORESOURCE_IRQ_* defines in > @@ -407,7 +408,12 @@ asmlinkage void do_softirq(void); > asmlinkage void __do_softirq(void); > extern void open_softirq(int nr, void (*action)(struct softirq_action *)); > extern void softirq_init(void); > -#define __raise_softirq_irqoff(nr) do { or_softirq_pending(1UL << (nr)); } while (0) > +static inline void __raise_softirq_irqoff(unsigned int nr) > +{ > + trace_softirq_raise((struct softirq_action *)&nr, NULL); > + or_softirq_pending(1UL << nr); > +} > + > extern void raise_softirq_irqoff(unsigned int nr); > extern void raise_softirq(unsigned int nr); > extern void wakeup_softirqd(void); > diff --git a/include/trace/events/irq.h b/include/trace/events/irq.h > index 0e4cfb6..3ddda02 100644 > --- a/include/trace/events/irq.h > +++ b/include/trace/events/irq.h > @@ -5,7 +5,9 @@ > #define _TRACE_IRQ_H > > #include <linux/tracepoint.h> > -#include <linux/interrupt.h> > + > +struct irqaction; > +struct softirq_action; > > #define softirq_name(sirq) { sirq##_SOFTIRQ, #sirq } > #define show_softirq_name(val) \ > @@ -93,7 +95,10 @@ DECLARE_EVENT_CLASS(softirq, > ), > > TP_fast_assign( > - __entry->vec = (int)(h - vec); > + if (vec) > + __entry->vec = (int)(h - vec); > + else > + __entry->vec = *((int *)h); > ), It seems that this will break softirq_entry/exit tracepoints. __entry->vec will deref vec->action() for these two, which is not what we want. If you can't have the same tracepoint signature for the three, just split the new one in a seperate TRACE_EVENT(). Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2010-09-03 at 17:29 +0200, Frederic Weisbecker wrote: > > #define softirq_name(sirq) { sirq##_SOFTIRQ, #sirq } > > #define show_softirq_name(val) \ > > @@ -93,7 +95,10 @@ DECLARE_EVENT_CLASS(softirq, > > ), > > > > TP_fast_assign( > > - __entry->vec = (int)(h - vec); > > + if (vec) > > + __entry->vec = (int)(h - vec); > > + else > > + __entry->vec = *((int *)h); > > ), > > > > It seems that this will break softirq_entry/exit tracepoints. > __entry->vec will deref vec->action() for these two, which is not > what we want. But for trace_softirq_entry and trace_softirq_exit, vec will not be NULL. > > If you can't have the same tracepoint signature for the three, just > split the new one in a seperate TRACE_EVENT(). It may be a bit of a hack, and questionable about adding another TRACE_EVENT(). There still is a pretty good space savings in using DEFINE_EVENT() over TRACE_EVENT() though. -- Steve -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 03, 2010 at 11:39:36AM -0400, Steven Rostedt wrote: > On Fri, 2010-09-03 at 17:29 +0200, Frederic Weisbecker wrote: > > > > #define softirq_name(sirq) { sirq##_SOFTIRQ, #sirq } > > > #define show_softirq_name(val) \ > > > @@ -93,7 +95,10 @@ DECLARE_EVENT_CLASS(softirq, > > > ), > > > > > > TP_fast_assign( > > > - __entry->vec = (int)(h - vec); > > > + if (vec) > > > + __entry->vec = (int)(h - vec); > > > + else > > > + __entry->vec = *((int *)h); > > > ), > > > > > > > > It seems that this will break softirq_entry/exit tracepoints. > > __entry->vec will deref vec->action() for these two, which is not > > what we want. > > But for trace_softirq_entry and trace_softirq_exit, vec will not be > NULL. Oh right... /me slaps his forehead > > > > > If you can't have the same tracepoint signature for the three, just > > split the new one in a seperate TRACE_EVENT(). > > It may be a bit of a hack, and questionable about adding another > TRACE_EVENT(). There still is a pretty good space savings in using > DEFINE_EVENT() over TRACE_EVENT() though. Yeah, let's keep it as is. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2010-09-03 at 17:29 +0200, Frederic Weisbecker wrote: > > /* > > * These correspond to the IORESOURCE_IRQ_* defines in > > @@ -407,7 +408,12 @@ asmlinkage void do_softirq(void); > > asmlinkage void __do_softirq(void); > > extern void open_softirq(int nr, void (*action)(struct softirq_action *)); > > extern void softirq_init(void); > > -#define __raise_softirq_irqoff(nr) do { or_softirq_pending(1UL << (nr)); } while (0) > > +static inline void __raise_softirq_irqoff(unsigned int nr) > > +{ > > + trace_softirq_raise((struct softirq_action *)&nr, NULL); Perhaps doing: trace_softirq_raise((struct softirq_action *)((unsigend long)nr), NULL); and ... > > + or_softirq_pending(1UL << nr); > > +} > > + > > extern void raise_softirq_irqoff(unsigned int nr); > > extern void raise_softirq(unsigned int nr); > > extern void wakeup_softirqd(void); > > diff --git a/include/trace/events/irq.h b/include/trace/events/irq.h > > index 0e4cfb6..3ddda02 100644 > > --- a/include/trace/events/irq.h > > +++ b/include/trace/events/irq.h > > @@ -5,7 +5,9 @@ > > #define _TRACE_IRQ_H > > > > #include <linux/tracepoint.h> > > -#include <linux/interrupt.h> > > + > > +struct irqaction; > > +struct softirq_action; > > > > #define softirq_name(sirq) { sirq##_SOFTIRQ, #sirq } > > #define show_softirq_name(val) \ > > @@ -93,7 +95,10 @@ DECLARE_EVENT_CLASS(softirq, > > ), > > > > TP_fast_assign( > > - __entry->vec = (int)(h - vec); > > + if (vec) > > + __entry->vec = (int)(h - vec); > > + else > > + __entry->vec = *((int *)h); __entry->vec = (int)h; would be better. > > ), > > > > It seems that this will break softirq_entry/exit tracepoints. > __entry->vec will deref vec->action() for these two, which is not > what we want. > > If you can't have the same tracepoint signature for the three, just > split the new one in a seperate TRACE_EVENT(). Doing the above will at least be a bit safer. -- Steve -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 03, 2010 at 11:43:12AM -0400, Steven Rostedt wrote: > On Fri, 2010-09-03 at 17:29 +0200, Frederic Weisbecker wrote: > > > > /* > > > * These correspond to the IORESOURCE_IRQ_* defines in > > > @@ -407,7 +408,12 @@ asmlinkage void do_softirq(void); > > > asmlinkage void __do_softirq(void); > > > extern void open_softirq(int nr, void (*action)(struct softirq_action *)); > > > extern void softirq_init(void); > > > -#define __raise_softirq_irqoff(nr) do { or_softirq_pending(1UL << (nr)); } while (0) > > > +static inline void __raise_softirq_irqoff(unsigned int nr) > > > +{ > > > + trace_softirq_raise((struct softirq_action *)&nr, NULL); > > Perhaps doing: > > trace_softirq_raise((struct softirq_action *)((unsigend long)nr), > NULL); > > and ... > > > > + or_softirq_pending(1UL << nr); > > > +} > > > + > > > extern void raise_softirq_irqoff(unsigned int nr); > > > extern void raise_softirq(unsigned int nr); > > > extern void wakeup_softirqd(void); > > > diff --git a/include/trace/events/irq.h b/include/trace/events/irq.h > > > index 0e4cfb6..3ddda02 100644 > > > --- a/include/trace/events/irq.h > > > +++ b/include/trace/events/irq.h > > > @@ -5,7 +5,9 @@ > > > #define _TRACE_IRQ_H > > > > > > #include <linux/tracepoint.h> > > > -#include <linux/interrupt.h> > > > + > > > +struct irqaction; > > > +struct softirq_action; > > > > > > #define softirq_name(sirq) { sirq##_SOFTIRQ, #sirq } > > > #define show_softirq_name(val) \ > > > @@ -93,7 +95,10 @@ DECLARE_EVENT_CLASS(softirq, > > > ), > > > > > > TP_fast_assign( > > > - __entry->vec = (int)(h - vec); > > > + if (vec) > > > + __entry->vec = (int)(h - vec); > > > + else > > > + __entry->vec = *((int *)h); > > __entry->vec = (int)h; > > would be better. > > > > > ), > > > > > > > > It seems that this will break softirq_entry/exit tracepoints. > > __entry->vec will deref vec->action() for these two, which is not > > what we want. > > > > If you can't have the same tracepoint signature for the three, just > > split the new one in a seperate TRACE_EVENT(). > > Doing the above will at least be a bit safer. Agreed, I'm going to change that in the patch. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
(2010/09/04 0:50), Frederic Weisbecker wrote: > On Fri, Sep 03, 2010 at 11:43:12AM -0400, Steven Rostedt wrote: >> On Fri, 2010-09-03 at 17:29 +0200, Frederic Weisbecker wrote: >> >>>> /* >>>> * These correspond to the IORESOURCE_IRQ_* defines in >>>> @@ -407,7 +408,12 @@ asmlinkage void do_softirq(void); >>>> asmlinkage void __do_softirq(void); >>>> extern void open_softirq(int nr, void (*action)(struct softirq_action *)); >>>> extern void softirq_init(void); >>>> -#define __raise_softirq_irqoff(nr) do { or_softirq_pending(1UL << (nr)); } while (0) >>>> +static inline void __raise_softirq_irqoff(unsigned int nr) >>>> +{ >>>> + trace_softirq_raise((struct softirq_action *)&nr, NULL); >> >> Perhaps doing: >> >> trace_softirq_raise((struct softirq_action *)((unsigend long)nr), >> NULL); >> >> and ... >> >>>> + or_softirq_pending(1UL << nr); >>>> +} >>>> + >>>> extern void raise_softirq_irqoff(unsigned int nr); >>>> extern void raise_softirq(unsigned int nr); >>>> extern void wakeup_softirqd(void); >>>> diff --git a/include/trace/events/irq.h b/include/trace/events/irq.h >>>> index 0e4cfb6..3ddda02 100644 >>>> --- a/include/trace/events/irq.h >>>> +++ b/include/trace/events/irq.h >>>> @@ -5,7 +5,9 @@ >>>> #define _TRACE_IRQ_H >>>> >>>> #include <linux/tracepoint.h> >>>> -#include <linux/interrupt.h> >>>> + >>>> +struct irqaction; >>>> +struct softirq_action; >>>> >>>> #define softirq_name(sirq) { sirq##_SOFTIRQ, #sirq } >>>> #define show_softirq_name(val) \ >>>> @@ -93,7 +95,10 @@ DECLARE_EVENT_CLASS(softirq, >>>> ), >>>> >>>> TP_fast_assign( >>>> - __entry->vec = (int)(h - vec); >>>> + if (vec) >>>> + __entry->vec = (int)(h - vec); >>>> + else >>>> + __entry->vec = *((int *)h); >> >> __entry->vec = (int)h; >> >> would be better. >> >> >>>> ), >>> >>> >>> >>> It seems that this will break softirq_entry/exit tracepoints. >>> __entry->vec will deref vec->action() for these two, which is not >>> what we want. >>> >>> If you can't have the same tracepoint signature for the three, just >>> split the new one in a seperate TRACE_EVENT(). >> >> Doing the above will at least be a bit safer. > > > Agreed, I'm going to change that in the patch. > > Thanks. > I agree. Thanks, Koki Sanagi. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h index a0384a4..d3e8e90 100644 --- a/include/linux/interrupt.h +++ b/include/linux/interrupt.h @@ -18,6 +18,7 @@ #include <asm/atomic.h> #include <asm/ptrace.h> #include <asm/system.h> +#include <trace/events/irq.h> /* * These correspond to the IORESOURCE_IRQ_* defines in @@ -407,7 +408,12 @@ asmlinkage void do_softirq(void); asmlinkage void __do_softirq(void); extern void open_softirq(int nr, void (*action)(struct softirq_action *)); extern void softirq_init(void); -#define __raise_softirq_irqoff(nr) do { or_softirq_pending(1UL << (nr)); } while (0) +static inline void __raise_softirq_irqoff(unsigned int nr) +{ + trace_softirq_raise((struct softirq_action *)&nr, NULL); + or_softirq_pending(1UL << nr); +} + extern void raise_softirq_irqoff(unsigned int nr); extern void raise_softirq(unsigned int nr); extern void wakeup_softirqd(void); diff --git a/include/trace/events/irq.h b/include/trace/events/irq.h index 0e4cfb6..3ddda02 100644 --- a/include/trace/events/irq.h +++ b/include/trace/events/irq.h @@ -5,7 +5,9 @@ #define _TRACE_IRQ_H #include <linux/tracepoint.h> -#include <linux/interrupt.h> + +struct irqaction; +struct softirq_action; #define softirq_name(sirq) { sirq##_SOFTIRQ, #sirq } #define show_softirq_name(val) \ @@ -93,7 +95,10 @@ DECLARE_EVENT_CLASS(softirq, ), TP_fast_assign( - __entry->vec = (int)(h - vec); + if (vec) + __entry->vec = (int)(h - vec); + else + __entry->vec = *((int *)h); ), TP_printk("vec=%d [action=%s]", __entry->vec, @@ -136,6 +141,23 @@ DEFINE_EVENT(softirq, softirq_exit, TP_ARGS(h, vec) ); +/** + * softirq_raise - called immediately when a softirq is raised + * @h: pointer to struct softirq_action + * @vec: pointer to first struct softirq_action in softirq_vec array + * + * The @h parameter contains a pointer to the softirq vector number which is + * raised. @vec is NULL and it means @h includes vector number not + * softirq_action. When used in combination with the softirq_entry tracepoint + * we can determine the softirq raise latency. + */ +DEFINE_EVENT(softirq, softirq_raise, + + TP_PROTO(struct softirq_action *h, struct softirq_action *vec), + + TP_ARGS(h, vec) +); + #endif /* _TRACE_IRQ_H */ /* This part must be outside protection */