linux-kernel - Re: [PATCH v4] ftrace: add a tracepoint for __raise_softirq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090519082435.GB15286@elte.hu>
Date:	Tue, 19 May 2009 10:24:35 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Zhaolei <zhaolei@...fujitsu.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Cc:	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>,
	Steven Rostedt <rostedt@...dmis.org>,
	linux-kernel@...r.kernel.org, fweisbec@...il.com,
	laijs@...fujitsu.com, Li Zefan <lizf@...fujitsu.com>,
	Xiao Guangrong <xiaoguangrong@...fujitsu.com>
Subject: Re: [PATCH v4] ftrace: add a tracepoint for
	__raise_softirq_irqoff()


* Zhaolei <zhaolei@...fujitsu.com> wrote:

> * From: "Xiao Guangrong" <xiaoguangrong@...fujitsu.com>
> > Steven Rostedt wrote:
> >> On Thu, 14 May 2009, Mathieu Desnoyers wrote:
> >>>> From: Mathieu Desnoyers <mathieu.desnoyers@...ymtl.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.
 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/