| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20101019132236.GA19197@Krystal>
Date: Tue, 19 Oct 2010 09:22:37 -0400
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Koki Sanagi <sanagi.koki@...fujitsu.com>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...e.hu>,
Frederic Weisbecker <fweisbec@...il.com>,
Steven Rostedt <rostedt@...dmis.org>, nhorman@...driver.com,
scott.a.mcmillan@...el.com, laijs@...fujitsu.com,
"H. Peter Anvin" <hpa@...or.com>,
LKML <linux-kernel@...r.kernel.org>, eric.dumazet@...il.com,
kaneshige.kenji@...fujitsu.com, David Miller <davem@...emloft.net>,
izumi.taku@...fujitsu.com, kosaki.motohiro@...fujitsu.com,
Heiko Carstens <heiko.carstens@...ibm.com>,
"Luck, Tony" <tony.luck@...el.com>
Subject: Re: [PATCH] tracing: Cleanup the convoluted softirq tracepoints
* Thomas Gleixner (tglx@...utronix.de) wrote:
> With the addition of trace_softirq_raise() the softirq tracepoint got
> even more convoluted. Why the tracepoints take two pointers to assign
> an integer is beyond my comprehension.
>
> But adding an extra case which treats the first pointer as an unsigned
> long when the second pointer is NULL including the back and forth
> type casting is just horrible.
>
> Convert the softirq tracepoints to take a single unsigned int argument
> for the softirq vector number and fix the call sites.
Well, there was originally a reason for this oddness. The in __do_softirq(),
"h - softirq_ve"c computation was not needed outside of the tracepoint handler
in the past, but it now seems to be required with the new inlined
"kstat_incr_softirqs_this_cpu()".
So yes, thanks to this recent change, it now makes sense to pull this
computation out of the tracepoints and do it unconditionally in the kernel code.
Feel free to put my:
Acked-by: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Thanks,
Mathieu
>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> ---
> include/linux/interrupt.h | 2 -
> include/trace/events/irq.h | 54 ++++++++++++++++-----------------------------
> kernel/softirq.c | 14 ++++++-----
> 3 files changed, 29 insertions(+), 41 deletions(-)
>
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index 531495d..0ac1949 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -410,7 +410,7 @@ extern void open_softirq(int nr, void (*action)(struct softirq_action *));
> extern void softirq_init(void);
> static inline void __raise_softirq_irqoff(unsigned int nr)
> {
> - trace_softirq_raise((struct softirq_action *)(unsigned long)nr, NULL);
> + trace_softirq_raise(nr);
> or_softirq_pending(1UL << nr);
> }
>
> diff --git a/include/trace/events/irq.h b/include/trace/events/irq.h
> index 6fa7cba..1c09820 100644
> --- a/include/trace/events/irq.h
> +++ b/include/trace/events/irq.h
> @@ -86,76 +86,62 @@ TRACE_EVENT(irq_handler_exit,
>
> DECLARE_EVENT_CLASS(softirq,
>
> - TP_PROTO(struct softirq_action *h, struct softirq_action *vec),
> + TP_PROTO(unsigned int vec_nr),
>
> - TP_ARGS(h, vec),
> + TP_ARGS(vec_nr),
>
> TP_STRUCT__entry(
> - __field( int, vec )
> + __field( unsigned int, vec )
> ),
>
> TP_fast_assign(
> - if (vec)
> - __entry->vec = (int)(h - vec);
> - else
> - __entry->vec = (int)(long)h;
> + __entry->vec = vec_nr;
> ),
>
> - TP_printk("vec=%d [action=%s]", __entry->vec,
> + TP_printk("vec=%u [action=%s]", __entry->vec,
> show_softirq_name(__entry->vec))
> );
>
> /**
> * softirq_entry - called immediately before the softirq handler
> - * @h: pointer to struct softirq_action
> - * @vec: pointer to first struct softirq_action in softirq_vec array
> + * @vec_nr: softirq vector number
> *
> - * The @h parameter, contains a pointer to the struct softirq_action
> - * which has a pointer to the action handler that is called. By subtracting
> - * the @vec pointer from the @h pointer, we can determine the softirq
> - * number. Also, when used in combination with the softirq_exit tracepoint
> - * we can determine the softirq latency.
> + * When used in combination with the softirq_exit tracepoint
> + * we can determine the softirq handler runtine.
> */
> DEFINE_EVENT(softirq, softirq_entry,
>
> - TP_PROTO(struct softirq_action *h, struct softirq_action *vec),
> + TP_PROTO(unsigned int vec_nr),
>
> - TP_ARGS(h, vec)
> + TP_ARGS(vec_nr)
> );
>
> /**
> * softirq_exit - called immediately after the softirq handler returns
> - * @h: pointer to struct softirq_action
> - * @vec: pointer to first struct softirq_action in softirq_vec array
> + * @vec_nr: softirq vector number
> *
> - * The @h parameter contains a pointer to the struct softirq_action
> - * that has handled the softirq. By subtracting the @vec pointer from
> - * the @h pointer, we can determine the softirq number. Also, when used in
> - * combination with the softirq_entry tracepoint we can determine the softirq
> - * latency.
> + * When used in combination with the softirq_entry tracepoint
> + * we can determine the softirq handler runtine.
> */
> DEFINE_EVENT(softirq, softirq_exit,
>
> - TP_PROTO(struct softirq_action *h, struct softirq_action *vec),
> + TP_PROTO(unsigned int vec_nr),
>
> - TP_ARGS(h, vec)
> + TP_ARGS(vec_nr)
> );
>
> /**
> * 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
> + * @vec_nr: softirq vector number
> *
> - * 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.
> + * When used in combination with the softirq_entry tracepoint
> + * we can determine the softirq raise to run latency.
> */
> DEFINE_EVENT(softirq, softirq_raise,
>
> - TP_PROTO(struct softirq_action *h, struct softirq_action *vec),
> + TP_PROTO(unsigned int vec_nr),
>
> - TP_ARGS(h, vec)
> + TP_ARGS(vec_nr)
> );
>
> #endif /* _TRACE_IRQ_H */
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index 07b4f1b..c0a9ea5 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -212,18 +212,20 @@ restart:
>
> do {
> if (pending & 1) {
> + unsigned int vec_nr = h - softirq_vec;
> int prev_count = preempt_count();
> - kstat_incr_softirqs_this_cpu(h - softirq_vec);
>
> - trace_softirq_entry(h, softirq_vec);
> + kstat_incr_softirqs_this_cpu(vec_nr);
> +
> + trace_softirq_entry(vec_nr);
> h->action(h);
> - trace_softirq_exit(h, softirq_vec);
> + trace_softirq_exit(vec_nr);
> if (unlikely(prev_count != preempt_count())) {
> printk(KERN_ERR "huh, entered softirq %td %s %p"
> "with preempt_count %08x,"
> - " exited with %08x?\n", h - softirq_vec,
> - softirq_to_name[h - softirq_vec],
> - h->action, prev_count, preempt_count());
> + " exited with %08x?\n", vec_nr,
> + softirq_to_name[vec_nr], h->action,
> + prev_count, preempt_count());
> preempt_count() = prev_count;
> }
>
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists