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]
Date:	Thu, 14 May 2009 08:36:06 -0400
From:	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
To:	Lai Jiangshan <laijs@...fujitsu.com>
Cc:	Xiao Guangrong <xiaoguangrong@...fujitsu.com>,
	linux-kernel@...r.kernel.org, mingo@...e.hu, fweisbec@...il.com,
	rostedt@...dmis.org, zhaolei@...fujitsu.com,
	Li Zefan <lizf@...fujitsu.com>
Subject: Re: [PATCH v3] ftrace: add a tracepoint for
	__raise_softirq_irqoff()

* Lai Jiangshan (laijs@...fujitsu.com) wrote:
> Mathieu Desnoyers wrote:
> > * Lai Jiangshan (laijs@...fujitsu.com) wrote:
> >> Mathieu Desnoyers wrote:
> >>> I partially agree with you :
> >>>
> >>> Yes, we should try to fix TRACE_EVENT, but we should fix it _before_ we
> >>> start using it widely. Circular header dependencies is a real problem
> >>> with TRACE_EVENT right now.
> >>>
> >>> Until we fix this, I will be tempted to stay with a known-good solution,
> >>> which is DECLARE/DEFINE_TRACE.
> >>>
> >>>
> >> I partially agree with you:
> >>
> >> Yes, Circular header dependencies is a real problem with TRACE_EVENT
> >> right now. It is also a problem with DECLARE_TRACE. It's a stubborn
> >> disease with C-Language (for complex headers). Can we fix C-Language?
> >>
> >> o Macros in header (!CREATE_TRACE_POINTS)
> >>
> >> When CREATE_TRACE_POINTS is not defined, TRACE_EVENT is definitely
> >> the same as DECLARE_TRACE. Actually, TRACE_EVENT is:
> >>
> >> #define TRACE_EVENT(name, proto, args, struct, assign, print)	\
> >> 	DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
> >>
> >> So TRACE_EVENT and DECLARE_TRACE are the same in header files.
> >> And so TRACE_EVENT and DECLARE_TRACE have the same advantages and
> >> disadvantages. More TRACE_EVENT equals to a known-good solution.
> >>
> >> o Macros in c-file
> >>
> >> tracepoint uses DEFINE_TRACE only.
> >>
> >> ftrace uses CREATE_TRACE_POINTS + TRACE_EVENT:
> >> 	#define CREATE_TRACE_POINTS
> >> 	#include <trace/events/sched.h> (which uses TRACE_EVENT)
> >>
> >> ftrace generates more code which uses the tracepoints.
> >>
> >>> Then add a forward declaration of
> >>>
> >>> struct softirqaction;
> >>>
> >>> At the top of trace/irq.h. I did it in quite a few places in the LTTng
> >>> tree. TP_PROTO just needs a forward declaration, not the full structure
> >>> declaration.
> >>>
> >> Thank you for your valuable suggestions.
> >>
> >> You are the father of tracepoint and LTTng, your experience in
> >> LTTng is very useful for ftrace.
> >>
> >> I'm glad for your suggestions.
> >>
> >>
> >> Xiao Guangrong, could you add forward declarations of
> >>
> >> struct irqaction;
> >> struct softirq_action;
> >>
> >> at the top of trace/irq.h as Mathieu's suggestions.
> >> (and remove "#include <linux/interrupt.h>")
> >>
> > 
> > You will probably still need something like :
> > 
> > #ifdef CREATE_TRACE_POINTS
> > #include <linux/interrupt.h>
> > #else
> > struct irqaction;
> > struct softirq_action;
> > #endif
> > 
> 
> It's not needed for trace/events/irq.h
> 
> Yes, it's a solution.
> 
> But I don't think we have to do this, CREATE_TRACE_POINTS is
> needed only _once_ for every <trace/events/xxxx.h>
> 
> The .c file which defines CREATE_TRACE_POINTS can provide
> (had provided likely) things like "#include <linux/interrupt.h>"
> 
> See kernel/softirq.c:
> #include <linux/interrupt.h>
> ......
> ......
> #define CREATE_TRACE_POINTS
> #include <trace/events/irq.h>
> 

Isn't it all included under kernel/trace/events.c ? e.g. :

#include <trace/trace_events.h>

#include "trace_output.h"

#include "trace_events_stage_1.h"
#include "trace_events_stage_2.h"
#include "trace_events_stage_3.h"

Therefore, adding a #include <linux/interrupt.h> would be just weird
here. We would have to do that for every tracepoint headers, and we
would just lose the ability to keep track of which tracepoint header has
which include dependency.

I thought the goal of TRACE_EVENT() was exactly the opposite : to
declare everything in one location....

Therefore, if you agree that it's good to have TRACE_EVENT() declaring
everything in one location, then I don't see why you would argue to move
the include dependencies in the very remote kernel/trace/events.c file ?

Mathieu

> I don't think it's a problem, CREATE_TRACE_POINTS is defined only
> once for a <trace/events/xxxx.h>.
> 
> Lai.
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
--
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/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ