[lkml]   [2009]   [Jun]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH v4] ftrace: add a tracepoint for __raise_softirq_irqoff()
    Ingo Molnar wrote:
    > * Zhaolei <> wrote:
    >> * From: "Xiao Guangrong" <>
    >>> Steven Rostedt wrote:
    >>>> On Thu, 14 May 2009, Mathieu Desnoyers wrote:
    >>>>>> From: Mathieu Desnoyers <>
    >>>>>> This patch is modified from Mathieu Desnoyers' patch. The original patch
    >>>>>> can be found here:
    >>>>>> 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).


    > 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    [W:0.029 / U:0.060 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site