[PATCH V3] trace/events: add chip name and hwirq to irq entry tracepoint

[PATCH V3] trace/events: add chip name and hwirq to irq entry tracepoint

[Ankit Gupta]

Add chip name and hw-irq number to the trace_irq_handler_entry()
tracepoint. When tracing interrupt events the chip-name and hw-irq
numbers are stable and known in advance. This makes them a better
choice as a filtering criteria for the trace buffer dump. On the
flipside, the os-irq numbers are dynamically allocated which makes
them difficult to use for the same purpose.

Dump messages will look like:
...irq_handler_entry: irq=22 name=msm_serial0 chip_name=GIC hwirq=140

Suggested-by: Stephen Boyd <sboyd@codeaurora.org>
Reviewed-by: Andy Gross <agross@codeaurora.org>
Signed-off-by: Gilad Avidov <gavidov@codeaurora.org>
Signed-off-by: Ankit Gupta <ankgupta@codeaurora.org>
---
Changes since V2:
- fixed dump message in commit text to reflect Chip name
  instead of domain.
---
Changes since V1:
- added reviewed by Andy Gross
---
 include/trace/events/irq.h | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/include/trace/events/irq.h b/include/trace/events/irq.h
index 3608beb..5370075 100644
--- a/include/trace/events/irq.h
+++ b/include/trace/events/irq.h
@@ -23,6 +23,17 @@ struct softirq_action;
 			 softirq_name(HRTIMER),		\
 			 softirq_name(RCU))
 
+
+#define show_chip_name(irq)					\
+	(irq_get_irq_data(irq)					\
+			 ? irq_get_irq_data(irq)->chip->name	\
+			 : "NULL")
+
+#define show_hwirq(irq)						\
+	(irq_get_irq_data(irq)					\
+			 ? irq_get_irq_data(irq)->hwirq		\
+			 : -ENODEV)
+
 /**
  * irq_handler_entry - called immediately before the irq action handler
  * @irq: irq number
@@ -50,7 +61,9 @@ TRACE_EVENT(irq_handler_entry,
 		__assign_str(name, action->name);
 	),
 
-	TP_printk("irq=%d name=%s", __entry->irq, __get_str(name))
+	TP_printk("irq=%d name=%s chip_name=%s hwirq=%ld", __entry->irq,
+	  __get_str(name), show_chip_name(__entry->irq),
+	  show_hwirq(__entry->irq))
 );
 
 /**
-- 
1.8.5.2

[PATCH V3] trace/events: add chip name and hwirq to irq entry tracepoint

[Ankit Gupta]

Add chip name and hw-irq number to the trace_irq_handler_entry()
tracepoint. When tracing interrupt events the chip-name and hw-irq
numbers are stable and known in advance. This makes them a better
choice as a filtering criteria for the trace buffer dump. On the
flipside, the os-irq numbers are dynamically allocated which makes
them difficult to use for the same purpose.

Dump messages will look like:
...irq_handler_entry: irq=22 name=msm_serial0 chip_name=GIC hwirq=140

Suggested-by: Stephen Boyd <sboyd@codeaurora.org>
Reviewed-by: Andy Gross <agross@codeaurora.org>
Signed-off-by: Gilad Avidov <gavidov@codeaurora.org>
Signed-off-by: Ankit Gupta <ankgupta@codeaurora.org>
---
Changes since V2:
- fixed dump message in commit text to reflect Chip name
  instead of domain.
---
Changes since V1:
- added reviewed by Andy Gross
---
 include/trace/events/irq.h | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/include/trace/events/irq.h b/include/trace/events/irq.h
index 3608beb..5370075 100644
--- a/include/trace/events/irq.h
+++ b/include/trace/events/irq.h
@@ -23,6 +23,17 @@ struct softirq_action;
 			 softirq_name(HRTIMER),		\
 			 softirq_name(RCU))
 
+
+#define show_chip_name(irq)					\
+	(irq_get_irq_data(irq)					\
+			 ? irq_get_irq_data(irq)->chip->name	\
+			 : "NULL")
+
+#define show_hwirq(irq)						\
+	(irq_get_irq_data(irq)					\
+			 ? irq_get_irq_data(irq)->hwirq		\
+			 : -ENODEV)
+
 /**
  * irq_handler_entry - called immediately before the irq action handler
  * @irq: irq number
@@ -50,7 +61,9 @@ TRACE_EVENT(irq_handler_entry,
 		__assign_str(name, action->name);
 	),
 
-	TP_printk("irq=%d name=%s", __entry->irq, __get_str(name))
+	TP_printk("irq=%d name=%s chip_name=%s hwirq=%ld", __entry->irq,
+	  __get_str(name), show_chip_name(__entry->irq),
+	  show_hwirq(__entry->irq))
 );
 
 /**
-- 
1.8.5.2

Re: [PATCH V3] trace/events: add chip name and hwirq to irq entry tracepoint

[Steven Rostedt]

On Wed, 24 Jun 2015 10:17:30 -0600
Ankit Gupta <ankgupta@codeaurora.org> wrote:

> Add chip name and hw-irq number to the trace_irq_handler_entry()
> tracepoint. When tracing interrupt events the chip-name and hw-irq
> numbers are stable and known in advance. This makes them a better
> choice as a filtering criteria for the trace buffer dump. On the
> flipside, the os-irq numbers are dynamically allocated which makes
> them difficult to use for the same purpose.
> 
> Dump messages will look like:
> ...irq_handler_entry: irq=22 name=msm_serial0 chip_name=GIC hwirq=140
> 
> Suggested-by: Stephen Boyd <sboyd@codeaurora.org>
> Reviewed-by: Andy Gross <agross@codeaurora.org>
> Signed-off-by: Gilad Avidov <gavidov@codeaurora.org>
> Signed-off-by: Ankit Gupta <ankgupta@codeaurora.org>
> ---
> Changes since V2:
> - fixed dump message in commit text to reflect Chip name
>   instead of domain.
> ---
> Changes since V1:
> - added reviewed by Andy Gross
> ---
>  include/trace/events/irq.h | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/include/trace/events/irq.h b/include/trace/events/irq.h
> index 3608beb..5370075 100644
> --- a/include/trace/events/irq.h
> +++ b/include/trace/events/irq.h
> @@ -23,6 +23,17 @@ struct softirq_action;
>  			 softirq_name(HRTIMER),		\
>  			 softirq_name(RCU))
>  
> +
> +#define show_chip_name(irq)					\
> +	(irq_get_irq_data(irq)					\
> +			 ? irq_get_irq_data(irq)->chip->name	\
> +			 : "NULL")
> +
> +#define show_hwirq(irq)						\
> +	(irq_get_irq_data(irq)					\
> +			 ? irq_get_irq_data(irq)->hwirq		\
> +			 : -ENODEV)

Note these magic functions will only be useful for the tracefs reads of
the trace files. Userspace tools that extract the data (like perf and
trace-cmd), will have no idea of how to parse it.

I'm not against doing this, but I'm just letting you know what the
effect of this change will be.

-- Steve

> +
>  /**
>   * irq_handler_entry - called immediately before the irq action handler
>   * @irq: irq number
> @@ -50,7 +61,9 @@ TRACE_EVENT(irq_handler_entry,
>  		__assign_str(name, action->name);
>  	),
>  
> -	TP_printk("irq=%d name=%s", __entry->irq, __get_str(name))
> +	TP_printk("irq=%d name=%s chip_name=%s hwirq=%ld", __entry->irq,
> +	  __get_str(name), show_chip_name(__entry->irq),
> +	  show_hwirq(__entry->irq))
>  );
>  
>  /**

[PATCH V3] trace/events: add chip name and hwirq to irq entry tracepoint

[Steven Rostedt]

On Wed, 24 Jun 2015 10:17:30 -0600
Ankit Gupta <ankgupta@codeaurora.org> wrote:

> Add chip name and hw-irq number to the trace_irq_handler_entry()
> tracepoint. When tracing interrupt events the chip-name and hw-irq
> numbers are stable and known in advance. This makes them a better
> choice as a filtering criteria for the trace buffer dump. On the
> flipside, the os-irq numbers are dynamically allocated which makes
> them difficult to use for the same purpose.
> 
> Dump messages will look like:
> ...irq_handler_entry: irq=22 name=msm_serial0 chip_name=GIC hwirq=140
> 
> Suggested-by: Stephen Boyd <sboyd@codeaurora.org>
> Reviewed-by: Andy Gross <agross@codeaurora.org>
> Signed-off-by: Gilad Avidov <gavidov@codeaurora.org>
> Signed-off-by: Ankit Gupta <ankgupta@codeaurora.org>
> ---
> Changes since V2:
> - fixed dump message in commit text to reflect Chip name
>   instead of domain.
> ---
> Changes since V1:
> - added reviewed by Andy Gross
> ---
>  include/trace/events/irq.h | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/include/trace/events/irq.h b/include/trace/events/irq.h
> index 3608beb..5370075 100644
> --- a/include/trace/events/irq.h
> +++ b/include/trace/events/irq.h
> @@ -23,6 +23,17 @@ struct softirq_action;
>  			 softirq_name(HRTIMER),		\
>  			 softirq_name(RCU))
>  
> +
> +#define show_chip_name(irq)					\
> +	(irq_get_irq_data(irq)					\
> +			 ? irq_get_irq_data(irq)->chip->name	\
> +			 : "NULL")
> +
> +#define show_hwirq(irq)						\
> +	(irq_get_irq_data(irq)					\
> +			 ? irq_get_irq_data(irq)->hwirq		\
> +			 : -ENODEV)

Note these magic functions will only be useful for the tracefs reads of
the trace files. Userspace tools that extract the data (like perf and
trace-cmd), will have no idea of how to parse it.

I'm not against doing this, but I'm just letting you know what the
effect of this change will be.

-- Steve

> +
>  /**
>   * irq_handler_entry - called immediately before the irq action handler
>   * @irq: irq number
> @@ -50,7 +61,9 @@ TRACE_EVENT(irq_handler_entry,
>  		__assign_str(name, action->name);
>  	),
>  
> -	TP_printk("irq=%d name=%s", __entry->irq, __get_str(name))
> +	TP_printk("irq=%d name=%s chip_name=%s hwirq=%ld", __entry->irq,
> +	  __get_str(name), show_chip_name(__entry->irq),
> +	  show_hwirq(__entry->irq))
>  );
>  
>  /**

Re: [PATCH V3] trace/events: add chip name and hwirq to irq entry tracepoint

[Thomas Gleixner]

On Wed, 24 Jun 2015, Steven Rostedt wrote:
> > +
> > +#define show_chip_name(irq)					\
> > +	(irq_get_irq_data(irq)					\
> > +			 ? irq_get_irq_data(irq)->chip->name	\
> > +			 : "NULL")
> > +
> > +#define show_hwirq(irq)						\
> > +	(irq_get_irq_data(irq)					\
> > +			 ? irq_get_irq_data(irq)->hwirq		\
> > +			 : -ENODEV)
> 
> Note these magic functions will only be useful for the tracefs reads of
> the trace files. Userspace tools that extract the data (like perf and
> trace-cmd), will have no idea of how to parse it.
> 
> I'm not against doing this, but I'm just letting you know what the
> effect of this change will be.

What's worse is, that they are racy against a concurrent teardown of
the interrupt. Not a good idea ...

Thanks,

	tglx

[PATCH V3] trace/events: add chip name and hwirq to irq entry tracepoint

[Thomas Gleixner]

On Wed, 24 Jun 2015, Steven Rostedt wrote:
> > +
> > +#define show_chip_name(irq)					\
> > +	(irq_get_irq_data(irq)					\
> > +			 ? irq_get_irq_data(irq)->chip->name	\
> > +			 : "NULL")
> > +
> > +#define show_hwirq(irq)						\
> > +	(irq_get_irq_data(irq)					\
> > +			 ? irq_get_irq_data(irq)->hwirq		\
> > +			 : -ENODEV)
> 
> Note these magic functions will only be useful for the tracefs reads of
> the trace files. Userspace tools that extract the data (like perf and
> trace-cmd), will have no idea of how to parse it.
> 
> I'm not against doing this, but I'm just letting you know what the
> effect of this change will be.

What's worse is, that they are racy against a concurrent teardown of
the interrupt. Not a good idea ...

Thanks,

	tglx

Re: [PATCH V3] trace/events: add chip name and hwirq to irq entry tracepoint

[Ankit Gupta]

> On Wed, 24 Jun 2015, Steven Rostedt wrote:
>> > +
>> > +#define show_chip_name(irq)					\
>> > +	(irq_get_irq_data(irq)					\
>> > +			 ? irq_get_irq_data(irq)->chip->name	\
>> > +			 : "NULL")
>> > +
>> > +#define show_hwirq(irq)						\
>> > +	(irq_get_irq_data(irq)					\
>> > +			 ? irq_get_irq_data(irq)->hwirq		\
>> > +			 : -ENODEV)
>>
>> Note these magic functions will only be useful for the tracefs reads of
>> the trace files. Userspace tools that extract the data (like perf and
>> trace-cmd), will have no idea of how to parse it.
>>
>> I'm not against doing this, but I'm just letting you know what the
>> effect of this change will be.
>
> What's worse is, that they are racy against a concurrent teardown of
> the interrupt. Not a good idea ...
>

Agree, I'll save the chip-name and hwirq in the struct. This technique
using the same macros to save the chip-name and hwirq in the struct also
works well with trace-cmd.

Thanks,
Ankit and Gilad

> Thanks,
>
> 	tglx
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH V3] trace/events: add chip name and hwirq to irq entry tracepoint

[Ankit Gupta]

> On Wed, 24 Jun 2015, Steven Rostedt wrote:
>> > +
>> > +#define show_chip_name(irq)					\
>> > +	(irq_get_irq_data(irq)					\
>> > +			 ? irq_get_irq_data(irq)->chip->name	\
>> > +			 : "NULL")
>> > +
>> > +#define show_hwirq(irq)						\
>> > +	(irq_get_irq_data(irq)					\
>> > +			 ? irq_get_irq_data(irq)->hwirq		\
>> > +			 : -ENODEV)
>>
>> Note these magic functions will only be useful for the tracefs reads of
>> the trace files. Userspace tools that extract the data (like perf and
>> trace-cmd), will have no idea of how to parse it.
>>
>> I'm not against doing this, but I'm just letting you know what the
>> effect of this change will be.
>
> What's worse is, that they are racy against a concurrent teardown of
> the interrupt. Not a good idea ...
>

Agree, I'll save the chip-name and hwirq in the struct. This technique
using the same macros to save the chip-name and hwirq in the struct also
works well with trace-cmd.

Thanks,
Ankit and Gilad

> Thanks,
>
> 	tglx
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm"
> in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH V3] trace/events: add chip name and hwirq to irq entry tracepoint

[Thomas Gleixner]

On Wed, 8 Jul 2015, Ankit Gupta wrote:
> > On Wed, 24 Jun 2015, Steven Rostedt wrote:
> >> > +
> >> > +#define show_chip_name(irq)					\
> >> > +	(irq_get_irq_data(irq)					\
> >> > +			 ? irq_get_irq_data(irq)->chip->name	\
> >> > +			 : "NULL")
> >> > +
> >> > +#define show_hwirq(irq)						\
> >> > +	(irq_get_irq_data(irq)					\
> >> > +			 ? irq_get_irq_data(irq)->hwirq		\
> >> > +			 : -ENODEV)
> >>
> >> Note these magic functions will only be useful for the tracefs reads of
> >> the trace files. Userspace tools that extract the data (like perf and
> >> trace-cmd), will have no idea of how to parse it.
> >>
> >> I'm not against doing this, but I'm just letting you know what the
> >> effect of this change will be.
> >
> > What's worse is, that they are racy against a concurrent teardown of
> > the interrupt. Not a good idea ...
> >
> 
> Agree, I'll save the chip-name and hwirq in the struct. This technique
> using the same macros to save the chip-name and hwirq in the struct also
> works well with trace-cmd.

Though I really doubt that this is the proper way to go. Why do you want
to store redundant information on every tracepoint?

That does not make any sense.

What we really want is an interface to query the interrupt association
(linux interrupt number, chip, domain, hwirq, ...) in one go when the
tracer starts and have tracepoints which keep track of irq setup and
teardown.

That way we keep the hotpath tracepoints tiny and the analysis tools
can create rich tracepoints for postprocessing. That's how perf is
working as well.

Thanks,

	tglx

[PATCH V3] trace/events: add chip name and hwirq to irq entry tracepoint

[Thomas Gleixner]

On Wed, 8 Jul 2015, Ankit Gupta wrote:
> > On Wed, 24 Jun 2015, Steven Rostedt wrote:
> >> > +
> >> > +#define show_chip_name(irq)					\
> >> > +	(irq_get_irq_data(irq)					\
> >> > +			 ? irq_get_irq_data(irq)->chip->name	\
> >> > +			 : "NULL")
> >> > +
> >> > +#define show_hwirq(irq)						\
> >> > +	(irq_get_irq_data(irq)					\
> >> > +			 ? irq_get_irq_data(irq)->hwirq		\
> >> > +			 : -ENODEV)
> >>
> >> Note these magic functions will only be useful for the tracefs reads of
> >> the trace files. Userspace tools that extract the data (like perf and
> >> trace-cmd), will have no idea of how to parse it.
> >>
> >> I'm not against doing this, but I'm just letting you know what the
> >> effect of this change will be.
> >
> > What's worse is, that they are racy against a concurrent teardown of
> > the interrupt. Not a good idea ...
> >
> 
> Agree, I'll save the chip-name and hwirq in the struct. This technique
> using the same macros to save the chip-name and hwirq in the struct also
> works well with trace-cmd.

Though I really doubt that this is the proper way to go. Why do you want
to store redundant information on every tracepoint?

That does not make any sense.

What we really want is an interface to query the interrupt association
(linux interrupt number, chip, domain, hwirq, ...) in one go when the
tracer starts and have tracepoints which keep track of irq setup and
teardown.

That way we keep the hotpath tracepoints tiny and the analysis tools
can create rich tracepoints for postprocessing. That's how perf is
working as well.

Thanks,

	tglx