lkml.org 
[lkml]   [2010]   [Oct]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] tracing: Cleanup the convoluted softirq tracepoints
    * Thomas Gleixner (tglx@linutronix.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@efficios.com>

    Thanks,

    Mathieu

    >
    > Signed-off-by: Thomas Gleixner <tglx@linutronix.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


    \
     
     \ /
      Last update: 2010-10-19 15:25    [W:0.037 / U:31.072 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site