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

[Lai Jiangshan <laijs@cn.fujitsu.com>]

Ingo Molnar wrote:
> * Zhaolei <zhaolei@cn.fujitsu.com> wrote:
> 
>> * From: "Xiao Guangrong" <xiaoguangrong@cn.fujitsu.com>
>>> Steven Rostedt wrote:
>>>> On Thu, 14 May 2009, Mathieu Desnoyers wrote:
>>>>>> From: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
>>>>>>
>>>>>> 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.

It's a nice solution. But I think it would be nicer when it is changed a little.

The new API raise_softirq_check() will become a very hard _burden_ to the users of raise_softirq*(). They have to find out a proper place to place the "raise_softirq_check();". It's not an easy things, functions are complicated and hard to be determined WHERE is the process context and WHEN(a function may be called from multi kinds of context).

Instead, I prefer that raise_softirq_check() is hidden from users. We call raise_softirq_check() from schedule(), it will handle the un-handle softirqs in time(if un-handle softirqs are too much, it'll wakeup the ksoftirqd).


Lai


>  
> 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
> 
> 
>