lkml.org 
[lkml]   [2009]   [Jun]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4] ftrace: add a tracepoint for __raise_softirq_irqoff()
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
>
>
>




\
 
 \ /
  Last update: 2009-06-12 04:37    [from the cache]
©2003-2011 Jasper Spaans