[PATCH -next] trace_skb: fix build when CONFIG_NET is not enabled

[PATCH -next] trace_skb: fix build when CONFIG_NET is not enabled

[Randy Dunlap]

From: Randy Dunlap <randy.dunlap@oracle.com>

Fix trace_skb_sources build when CONFIG_NET is not enabled:

kernel/built-in.o: In function `probe_skb_dequeue':
trace_skb_sources.c:(.text+0xd9152): undefined reference to `init_net'
trace_skb_sources.c:(.text+0xd9188): undefined reference to `dev_get_by_index'

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
---
 kernel/trace/Kconfig |    1 +
 1 file changed, 1 insertion(+)

--- linux-next-20090817.orig/kernel/trace/Kconfig
+++ linux-next-20090817/kernel/trace/Kconfig
@@ -236,6 +236,7 @@ config BOOT_TRACER
 
 config SKB_SOURCES_TRACER
 	bool "Trace skb source information"
+	depends on NET
 	select GENERIC_TRACER
 	help
 	   This tracer helps developers/sysadmins correlate skb allocation and



-- 
~Randy
LPC 2009, Sept. 23-25, Portland, Oregon
http://linuxplumbersconf.org/2009/

Re: [PATCH -next] trace_skb: fix build when CONFIG_NET is not enabled

[Ingo Molnar]

* Randy Dunlap <randy.dunlap@oracle.com> wrote:

> From: Randy Dunlap <randy.dunlap@oracle.com>
> 
> Fix trace_skb_sources build when CONFIG_NET is not enabled:
> 
> kernel/built-in.o: In function `probe_skb_dequeue':
> trace_skb_sources.c:(.text+0xd9152): undefined reference to `init_net'
> trace_skb_sources.c:(.text+0xd9188): undefined reference to `dev_get_by_index'
> 
> Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
> Cc: Neil Horman <nhorman@tuxdriver.com>
> ---
>  kernel/trace/Kconfig |    1 +
>  1 file changed, 1 insertion(+)
> 
> --- linux-next-20090817.orig/kernel/trace/Kconfig
> +++ linux-next-20090817/kernel/trace/Kconfig
> @@ -236,6 +236,7 @@ config BOOT_TRACER
>  
>  config SKB_SOURCES_TRACER
>  	bool "Trace skb source information"
> +	depends on NET
>  	select GENERIC_TRACER
>  	help
>  	   This tracer helps developers/sysadmins correlate skb allocation and

Hm, there's nothing like this in the tracing tree.

Could we please move kernel/trace/* commits to the tracing tree, so 
that it gets adequate testing and review, etc?

	Ingo

Re: [PATCH -next] trace_skb: fix build when CONFIG_NET is not enabled

[David Miller]

From: Ingo Molnar <mingo@elte.hu>
Date: Mon, 17 Aug 2009 23:58:08 +0200

>> --- linux-next-20090817.orig/kernel/trace/Kconfig
>> +++ linux-next-20090817/kernel/trace/Kconfig
>> @@ -236,6 +236,7 @@ config BOOT_TRACER
>>  
>>  config SKB_SOURCES_TRACER
>>  	bool "Trace skb source information"
>> +	depends on NET
>>  	select GENERIC_TRACER
>>  	help
>>  	   This tracer helps developers/sysadmins correlate skb allocation and
> 
> Hm, there's nothing like this in the tracing tree.
> 
> Could we please move kernel/trace/* commits to the tracing tree, so 
> that it gets adequate testing and review, etc?

This one (like previous networking tracing changes Neil has
made) touched a decent amount of networking code, and thus
we integrated it into net-next-2.6

Re: [PATCH -next] trace_skb: fix build when CONFIG_NET is not enabled

[Ingo Molnar]

* David Miller <davem@davemloft.net> wrote:

> From: Ingo Molnar <mingo@elte.hu>
> Date: Mon, 17 Aug 2009 23:58:08 +0200
> 
> >> --- linux-next-20090817.orig/kernel/trace/Kconfig
> >> +++ linux-next-20090817/kernel/trace/Kconfig
> >> @@ -236,6 +236,7 @@ config BOOT_TRACER
> >>  
> >>  config SKB_SOURCES_TRACER
> >>  	bool "Trace skb source information"
> >> +	depends on NET
> >>  	select GENERIC_TRACER
> >>  	help
> >>  	   This tracer helps developers/sysadmins correlate skb allocation and
> > 
> > Hm, there's nothing like this in the tracing tree.
> > 
> > Could we please move kernel/trace/* commits to the tracing tree, so 
> > that it gets adequate testing and review, etc?
> 
> This one (like previous networking tracing changes Neil has made) 
> touched a decent amount of networking code, and thus we 
> integrated it into net-next-2.6

the three skb-sources-tracer patches i saw submitted were:

 include/trace/events/skb.h |   20 ++++++++++++++++++++
 net/core/datagram.c        |    3 +++
 2 files changed, 23 insertions(+)

 kernel/traceKconfig |   10 ++++++++++

 kernel/trace/Makefile            |    1
 kernel/trace/trace.h             |   19 ++++++
 kernel/trace/trace_skb_sources.c |  154 ++++++++++++++++++++++++++++++++++++++++++++++++++++

only touched the networking code for 3 lines in 
net/core/datagram.c.

Think about it: how would you react if i added a new file to 
net/core/ and modified net/Kconfig, and then broke the build? You'd 
quite likely insist on it being done via net-next-2.6, right? You'd 
also likely be upset about that kind of change, wouldnt you?

Also, has the review feedback from the tracing folks been 
addressed? Please separate these patches out and lets do this 
properly, this approach is not acceptable.

	Ingo

Re: [PATCH -next] trace_skb: fix build when CONFIG_NET is not enabled

[David Miller]

From: Ingo Molnar <mingo@elte.hu>
Date: Tue, 18 Aug 2009 00:47:27 +0200

> Also, has the review feedback from the tracing folks been 
> addressed? Please separate these patches out and lets do this 
> properly, this approach is not acceptable.

Fine, will keep you guys in the loop next time :-)

Re: [PATCH -next] trace_skb: fix build when CONFIG_NET is not enabled

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> 
> * David Miller <davem@davemloft.net> wrote:
> 
> > From: Ingo Molnar <mingo@elte.hu>
> > Date: Mon, 17 Aug 2009 23:58:08 +0200
> > 
> > >> --- linux-next-20090817.orig/kernel/trace/Kconfig
> > >> +++ linux-next-20090817/kernel/trace/Kconfig
> > >> @@ -236,6 +236,7 @@ config BOOT_TRACER
> > >>  
> > >>  config SKB_SOURCES_TRACER
> > >>  	bool "Trace skb source information"
> > >> +	depends on NET
> > >>  	select GENERIC_TRACER
> > >>  	help
> > >>  	   This tracer helps developers/sysadmins correlate skb allocation and
> > > 
> > > Hm, there's nothing like this in the tracing tree.
> > > 
> > > Could we please move kernel/trace/* commits to the tracing tree, so 
> > > that it gets adequate testing and review, etc?
> > 
> > This one (like previous networking tracing changes Neil has made) 
> > touched a decent amount of networking code, and thus we 
> > integrated it into net-next-2.6
> 
> the three skb-sources-tracer patches i saw submitted were:
> 
>  include/trace/events/skb.h |   20 ++++++++++++++++++++
>  net/core/datagram.c        |    3 +++
>  2 files changed, 23 insertions(+)
> 
>  kernel/traceKconfig |   10 ++++++++++
> 
>  kernel/trace/Makefile            |    1
>  kernel/trace/trace.h             |   19 ++++++
>  kernel/trace/trace_skb_sources.c |  154 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 
> only touched the networking code for 3 lines in 
> net/core/datagram.c.
> 
> Think about it: how would you react if i added a new file to 
> net/core/ and modified net/Kconfig, and then broke the build? 
> You'd quite likely insist on it being done via net-next-2.6, 
> right? You'd also likely be upset about that kind of change, 
> wouldnt you?
> 
> Also, has the review feedback from the tracing folks been 
> addressed? Please separate these patches out and lets do this 
> properly, this approach is not acceptable.

btw., this isnt just a question of patch logistics - these patches 
look unreviewed to me. Why is a new ftrace plugin needed? Why arent 
they defined in a TRACE_EVENT() form? That way they'd be generally 
usable together with other tracepoints, and would likely be enabled 
in most distros. That way they could also be added via other trees, 
as TRACE_EVENT() is a generic facility.

We try to migrate ftrace plugins to TRACE_EVENT() tracepoints.

	Ingo

Re: [PATCH -next] trace_skb: fix build when CONFIG_NET is not enabled

[David Miller]

From: Randy Dunlap <randy.dunlap@oracle.com>
Date: Mon, 17 Aug 2009 13:37:36 -0700

> From: Randy Dunlap <randy.dunlap@oracle.com>
> 
> Fix trace_skb_sources build when CONFIG_NET is not enabled:
> 
> kernel/built-in.o: In function `probe_skb_dequeue':
> trace_skb_sources.c:(.text+0xd9152): undefined reference to `init_net'
> trace_skb_sources.c:(.text+0xd9188): undefined reference to `dev_get_by_index'
> 
> Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>

Applied, thanks Randy.

Re: [PATCH -next] trace_skb: fix build when CONFIG_NET is not enabled

[Ingo Molnar]

* David Miller <davem@davemloft.net> wrote:

> From: Ingo Molnar <mingo@elte.hu>
> Date: Tue, 18 Aug 2009 00:47:27 +0200
> 
> > Also, has the review feedback from the tracing folks been 
> > addressed? Please separate these patches out and lets do this 
> > properly, this approach is not acceptable.
> 
> Fine, will keep you guys in the loop next time :-)

thanks

If the TRACE_EVENT() form is used then it can be done just fine 
from the networking tree too by enhancing 
include/trace/events/skb.h - TRACE_EVENT() is a decentralized 
facility.

We try not to add new ftrace plugins but extend the set of generic 
tracepoints.

	Ingo

Re: [PATCH -next] trace_skb: fix build when CONFIG_NET is not enabled

[Ingo Molnar]

* David Miller <davem@davemloft.net> wrote:

> From: Randy Dunlap <randy.dunlap@oracle.com>
> Date: Mon, 17 Aug 2009 13:37:36 -0700
> 
> > From: Randy Dunlap <randy.dunlap@oracle.com>
> > 
> > Fix trace_skb_sources build when CONFIG_NET is not enabled:
> > 
> > kernel/built-in.o: In function `probe_skb_dequeue':
> > trace_skb_sources.c:(.text+0xd9152): undefined reference to `init_net'
> > trace_skb_sources.c:(.text+0xd9188): undefined reference to `dev_get_by_index'
> > 
> > Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
> 
> Applied, thanks Randy.

... but these are the wrong patches, they should be removed or 
reverted and redone properly.

It's not just about keeping kernel/trace/* changes in the tracing 
tree (which we can relax on-demand given agreement), it's that 
these patches are also _wrong_ and we cannot relax anything about 
that.

	Ingo

Re: [PATCH -next] trace_skb: fix build when CONFIG_NET is not enabled

[David Miller]

From: Ingo Molnar <mingo@elte.hu>
Date: Tue, 18 Aug 2009 01:06:54 +0200

> ... but these are the wrong patches, they should be removed or 
> reverted and redone properly.

Given the number of users of net-next-2.6, removal via rebasing is
simply not an option.  The other two posibilities, sure...

> It's not just about keeping kernel/trace/* changes in the tracing 
> tree (which we can relax on-demand given agreement), it's that 
> these patches are also _wrong_ and we cannot relax anything about 
> that.

Why don't we give Neil a chance to review the situation and fix things
up?

If revert is the final decision, that's fine, I'll revert everything.

But in the mean time at least give Neil a chance to read all of your
feedback and coordinate a way to fix things with everyone.

Re: [PATCH -next] trace_skb: fix build when CONFIG_NET is not enabled

[Ingo Molnar]

* David Miller <davem@davemloft.net> wrote:

> From: Ingo Molnar <mingo@elte.hu>
> Date: Tue, 18 Aug 2009 01:06:54 +0200
> 
> > ... but these are the wrong patches, they should be removed or 
> > reverted and redone properly.
> 
> Given the number of users of net-next-2.6, removal via rebasing 
> is simply not an option.  The other two posibilities, sure...
> 
> > It's not just about keeping kernel/trace/* changes in the 
> > tracing tree (which we can relax on-demand given agreement), 
> > it's that these patches are also _wrong_ and we cannot relax 
> > anything about that.
> 
> Why don't we give Neil a chance to review the situation and fix 
> things up?
> 
> If revert is the final decision, that's fine, I'll revert 
> everything.
> 
> But in the mean time at least give Neil a chance to read all of 
> your feedback and coordinate a way to fix things with everyone.

Oh, sure. Conversion to TRACE_EVENT() is equivalent to the revert 
of the current patches plus introduction of the TRACE_EVENT() 
defines.

	Ingo