Re: [PATCH v4] ftrace: add a tracepoint for __raise_softirq_irqoff()

[Ingo Molnar]

* Lai Jiangshan <laijs@xxxxxxxxxxxxxx> wrote:

> Ingo Molnar wrote:
> > * Zhaolei <zhaolei@xxxxxxxxxxxxxx> wrote:
> > 
> >> * From: "Xiao Guangrong" <xiaoguangrong@xxxxxxxxxxxxxx>
> >>> Steven Rostedt wrote:
> >>>> On Thu, 14 May 2009, Mathieu Desnoyers wrote:
> >>>>>> From: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxx>
> >>>>>>
> >>>>>> This patch is modified from Mathieu Desnoyers' patch. The original patch
> >>>>>> can be found here: 
> >>>>>> http://marc.info/?l=linux-kernel&m=123791201816245&w=2
> >>>>>> This tracepoint can trace the time stamp when softirq action is raised. 
> >>>>>>
> >>>>>> Changelog for v1 -> v2: 
> >>>>>> 1: Use TRACE_EVENT instead of DEFINE_TRACE
> >>>>>> 2: Move the tracepoint from raise_softirq_irqoff() to
> >>>>>>    __raise_softirq_irqoff()
> >>>>>>
> >>>>>> Changelog for v2 -> v3: 
> >>>>>> Move the definition of __raise_softifq_irqoff() to .c file when
> >>>>>> CONFIG_TRACEPOINTS is enabled, to avoid recursive includes
> >>>>>>
> >>>>>> Changelog for v3 -> v4: 
> >>>>>> 1: Come back to v2, and use forward declarations to avoid
> >>>>>>    recursive includes as Mathieu's suggestion
> >>>>>> 2: Modifiy the tracepoint name
> >>>>>> 3: Add comments for this tracepoint
> >>>>>>
> >>>>> This is a step in the right direction, but please see my email to Lai
> >>>>> about the fact that this assumes correct and undocumented include
> >>>>> dependencies in kernel/trace/events.c. Not explicitely stating the
> >>>>> include dependencies is a build error waiting to happen.
> >>>>>
> >>>>> Including interrupt.h under a ifdef would allow keeping track of
> >>>>> TRACE_EVENT specific build dependencies neatly on a per header basis.
> >>>> This is all moot, the events.c file no longer exists and as not an issue.
> >>>>
> >>> As Steve's says, use ftrace in ftrace.h not in events.c now. 
> >>> So, this mistake does not exist.
> >>> Dose this patch has other error? I expect for your views.
> >>>
> >>> Thanks for your review, is great help to me. ;-) 
> >> Hello,
> >>
> >> It seems Mathieu has no other comments on this patch now.
> >> Ingo, what is your opinion on this patch?
> > 
> > There's a complication: this area of the softirq code needs fixes 
> > (unrelated to tracing).
> >  
> > This API:
> > 
> > inline void raise_softirq_irqoff(unsigned int nr)
> > {
> >         __raise_softirq_irqoff(nr);
> > 
> >         /*
> >          * If we're in an interrupt or softirq, we're done
> >          * (this also catches softirq-disabled code). We will
> >          * actually run the softirq once we return from
> >          * the irq or softirq.
> >          *
> >          * Otherwise we wake up ksoftirqd to make sure we
> >          * schedule the softirq soon.
> >          */
> >         if (!in_interrupt())
> >                 wakeup_softirqd();
> > }
> > 
> > is broken with RT tasks (as recently reported to lkml), as when a 
> > real-time task wakes up ksoftirqd (which has lower priority) it wont 
> > execute and we starve softirq execution.
> > 
> > The proper solution would be to have a new API:
> > 
> > 	raise_softirq_check()
> > 
> > and to remove the wakeup_softirqd() hack from raise_softirq_irqoff() 
> > - and put raise_softirq_check() to all places that use 
> > raise_softirq*() from process context.
> >  
> > raise_softirq_check() would execute softirq handlers from process 
> > context, if there's any pending ones. It has to be called outside of 
> > bh critical sections - i.e. often a bit after the raise_softirq() 
> > has been done.
> > 
> > __raise_softirq_irqoff() would be made private to kernel/softirq.c, 
> > and we'd only have two public APIs to trigger softirqs: 
> > raise_softirq() and raise_softirq_irqoff(). Both just set the 
> > pending flag and dont do any wakeup.
> > 
> > As a side-effect of these fixes, the tracepoints will be sorted out 
> > as well - there wont be any need to hack into 
> > __raise_softirq_irqoff().
> > 
> > 	Ingo
> > 
> 
> Hi, Ingo
> 
> 	Could you give me the .config file that this bug occurs?

I havent reproduced this - it was reported in a recent thread on 
lkml. I dont have an URI for it - does anyone on the Cc: remember 
the details?

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/