Re: [NET]: Shut up warnings in net/core/flow.c

Re: [NET]: Shut up warnings in net/core/flow.c

[Andrew Morton]

Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote:
>
> tree e7ba0f1bc8764c36859e2cfa9421bb1d86f2e7f4
> parent b3a5225f31180322fd7d692fd4cf786702826b94
> author Russell King <rmk+kernel@arm.linux.org.uk> Wed, 23 Nov 2005 06:38:04 -0800
> committer David S. Miller <davem@davemloft.net> Wed, 23 Nov 2005 06:38:04 -0800
> 
> [NET]: Shut up warnings in net/core/flow.c
> 
> Not really a network problem, more a !SMP issue.
> 
> net/core/flow.c:295: warning: statement with no effect
> 
> flow.c:295:        smp_call_function(flow_cache_flush_per_cpu, &info, 1, 0);
> 
> Fix this by converting the macro to an inline function, which
> also increases the typechecking for !SMP builds.

Nope, this will break !CONFIG_SMP builds.  Quite a few places in the kernel
do not implement the ipi handler if !CONFIG_SMP.

diff -puN include/linux/smp.h~smp_call_function-must-be-a-macro include/linux/smp.h
--- devel/include/linux/smp.h~smp_call_function-must-be-a-macro	2005-11-23 00:14:19.000000000 -0800
+++ devel-akpm/include/linux/smp.h	2005-11-23 00:20:54.000000000 -0800
@@ -94,13 +94,7 @@ void smp_prepare_boot_cpu(void);
  */
 #define raw_smp_processor_id()			0
 #define hard_smp_processor_id()			0
-
-static inline int smp_call_function(void (*func) (void *info), void *info,
-				    int retry, int wait)
-{
-	return 0;
-}
-
+#define smp_call_function(func,info,retry,wait)	({ 0; })
 #define on_each_cpu(func,info,retry,wait)	({ func(info); 0; })
 static inline void smp_send_reschedule(int cpu) { }
 #define num_booting_cpus()			1
diff -puN net/core/flow.c~smp_call_function-must-be-a-macro net/core/flow.c
--- devel/net/core/flow.c~smp_call_function-must-be-a-macro	2005-11-23 00:17:40.000000000 -0800
+++ devel-akpm/net/core/flow.c	2005-11-23 00:17:47.000000000 -0800
@@ -292,7 +292,7 @@ void flow_cache_flush(void)
 	init_completion(&info.completion);
 
 	local_bh_disable();
-	smp_call_function(flow_cache_flush_per_cpu, &info, 1, 0);
+	(void)smp_call_function(flow_cache_flush_per_cpu, &info, 1, 0);
 	flow_cache_flush_tasklet((unsigned long)&info);
 	local_bh_enable();
 
_

Re: [NET]: Shut up warnings in net/core/flow.c

[David S. Miller]

From: Andrew Morton <akpm@osdl.org>
Date: Wed, 23 Nov 2005 00:21:34 -0800

> Nope, this will break !CONFIG_SMP builds.  Quite a few places in the
> kernel do not implement the ipi handler if !CONFIG_SMP.

Ho hum, nothing is ever easy eh? :-) I think your patch is fine for
now, but in the long term the !CONFIG_SMP ifdefs for those ipi
handlers should probably just get removed.  If GCC can't optimize
those things away, I'd be really surprised.

Re: [NET]: Shut up warnings in net/core/flow.c

[Arjan van de Ven]

On Wed, 2005-11-23 at 00:55 -0800, David S. Miller wrote:
> From: Andrew Morton <akpm@osdl.org>
> Date: Wed, 23 Nov 2005 00:21:34 -0800
> 
> > Nope, this will break !CONFIG_SMP builds.  Quite a few places in the
> > kernel do not implement the ipi handler if !CONFIG_SMP.
> 
> Ho hum, nothing is ever easy eh? :-) I think your patch is fine for
> now, but in the long term the !CONFIG_SMP ifdefs for those ipi
> handlers should probably just get removed.  If GCC can't optimize
> those things away, I'd be really surprised.

it can.. but only if we start using -ffunction-sections in the CFLAGS
(or make all of these functions static I suppose and reenable
-funit-at-a-time, which can be done for gcc 4.x only)

Re: [NET]: Shut up warnings in net/core/flow.c

[Linus Torvalds]

On Wed, 23 Nov 2005, David S. Miller wrote:
>
> From: Andrew Morton <akpm@osdl.org>
> Date: Wed, 23 Nov 2005 00:21:34 -0800
> 
> > Nope, this will break !CONFIG_SMP builds.  Quite a few places in the
> > kernel do not implement the ipi handler if !CONFIG_SMP.
> 
> Ho hum, nothing is ever easy eh? :-) I think your patch is fine for
> now, but in the long term the !CONFIG_SMP ifdefs for those ipi
> handlers should probably just get removed.  If GCC can't optimize
> those things away, I'd be really surprised.

I just reverted the whole commit.

We've had this exact thing before, and it's easy enough to handle, but you 
have to do it right.

The way to handle it is to do

	static inline int maybe_ignored(int arg, ...)
	{
		return arg;
	}

	#define smp_call_function(func,info,retry,wait) \
		maybe_ignored(0, info, retry, wait)

which is a very useful way to say: we don't care about "func", but we want 
to avoid unused warnings for "info", "retry" and "wait", and we want to 
return 0 regardless and compile it all away.

If somebody tests this, puts the "maybe_ignored()" function in some nice 
generic header file, I'll apply it.

I _refuse_ to apply the patch from Andrew that adds "(void)" to shut up 
the compiler. That's a piece of crap, and we should never do things like 
that. Bad C style.

		Linus

Re: [NET]: Shut up warnings in net/core/flow.c

[David S. Miller]

From: Arjan van de Ven <arjan@infradead.org>
Date: Wed, 23 Nov 2005 10:11:24 +0100

> it can.. but only if we start using -ffunction-sections in the CFLAGS
> (or make all of these functions static I suppose and reenable
> -funit-at-a-time, which can be done for gcc 4.x only)

I actually just scanned the tree, and outside of files that
only get built on CONFIG_SMP (namely, arch/${ARCH}/kernel/smp{,boot}.c)
the IPI functions were %99 marked static already and the remaining
%1 should be marked static.  The cases in that %1 group are:

arch/mips/sibyte/sb1250/prom.c:prom_cpu0_exit()
arch/powerpc/kernel/machine_kexec_64.c:kexec_smp_down()

And as stated, those two can just be marked static right now.

So we could very easily remove the CONFIG_SMP ifdefs, but the
-funit-at-a-time requirement to get gcc to not emit unused static
functions is very unfortunate.

Even tricks like marking the IPI function "inline" don't work since
we're taking the address of the function.

Re: [NET]: Shut up warnings in net/core/flow.c

[Arjan van de Ven]

On Wed, 2005-11-23 at 13:24 -0800, David S. Miller wrote:
> From: Arjan van de Ven <arjan@infradead.org>
> Date: Wed, 23 Nov 2005 10:11:24 +0100
> 
> > it can.. but only if we start using -ffunction-sections in the CFLAGS
> > (or make all of these functions static I suppose and reenable
> > -funit-at-a-time, which can be done for gcc 4.x only)
> 
> I actually just scanned the tree, and outside of files that
> only get built on CONFIG_SMP (namely, arch/${ARCH}/kernel/smp{,boot}.c)
> the IPI functions were %99 marked static already and the remaining
> %1 should be marked static.  The cases in that %1 group are:

static is good anyway :)

> 
> arch/mips/sibyte/sb1250/prom.c:prom_cpu0_exit()
> arch/powerpc/kernel/machine_kexec_64.c:kexec_smp_down()
> 
> And as stated, those two can just be marked static right now.
> 
> So we could very easily remove the CONFIG_SMP ifdefs, but the
> -funit-at-a-time requirement to get gcc to not emit unused static
> functions is very unfortunate.

I'm no gcc expert but afaik this really needs unit-at-a-time. (someone
who knows more about gcc please correct me if I'm wrong).

On the good news side:
-f-unit-at-a-time can be enabled for gcc 4.x; the stack bug that caused
it to be disabled is fixed in gcc 4.x

and .. -ffunction-sections may well be a good thing anyway; that works
even for non-statics.
(but iirc it needs some linker script changes because sections change
name)

Re: [NET]: Shut up warnings in net/core/flow.c

[David S. Miller]

From: Arjan van de Ven <arjan@infradead.org>
Date: Wed, 23 Nov 2005 22:28:19 +0100

> I'm no gcc expert but afaik this really needs unit-at-a-time. (someone
> who knows more about gcc please correct me if I'm wrong).

Yes, it has to parse the whole file before it can determine
entirely that the static function is indeed not referenced.

Re: [NET]: Shut up warnings in net/core/flow.c

[David S. Miller]

From: Linus Torvalds <torvalds@osdl.org>
Date: Wed, 23 Nov 2005 08:54:46 -0800 (PST)

> The way to handle it is to do
> 
> 	static inline int maybe_ignored(int arg, ...)
> 	{
> 		return arg;
> 	}
> 
> 	#define smp_call_function(func,info,retry,wait) \
> 		maybe_ignored(0, info, retry, wait)
> 
> which is a very useful way to say: we don't care about "func", but we want 
> to avoid unused warnings for "info", "retry" and "wait", and we want to 
> return 0 regardless and compile it all away.
> 
> If somebody tests this, puts the "maybe_ignored()" function in some nice 
> generic header file, I'll apply it.

I quickly hacked this up and did a UP test build (patch at the end),
but there is another consequence to consider.

With this, we have to either:

1) Mark all IPI functions with ifdef CONFIG_SMP, or
2) Mark them with __attribute__((__unused__))) which is what
   the net/core/flow.c case does

Because if we just leave the static IPI functions there without the
CONFIG_SMP ifdef and without the unused attribute, this new
smp_call_function() will generate an unused static function warning.

What we could do is hide that detail behind some kind of
"DEFINE_IPI_FUNC()" macro, and put the gore into a header file.

I'm sure there are other clean ways of handling it.

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index b1e407a..1876d3c 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -286,6 +286,11 @@ extern void dump_stack(void);
 	1; \
 })
 
+static inline int maybe_ignored(int arg, ...)
+{
+	return arg;
+}
+
 #endif /* __KERNEL__ */
 
 #define SI_LOAD_SHIFT	16
diff --git a/include/linux/smp.h b/include/linux/smp.h
index 9dfa3ee..2a513fe 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -94,7 +94,8 @@ void smp_prepare_boot_cpu(void);
  */
 #define raw_smp_processor_id()			0
 #define hard_smp_processor_id()			0
-#define smp_call_function(func,info,retry,wait)	({ 0; })
+#define smp_call_function(func,info,retry,wait)	\
+	maybe_ignored(0, info, retry, wait)
 #define on_each_cpu(func,info,retry,wait)	({ func(info); 0; })
 static inline void smp_send_reschedule(int cpu) { }
 #define num_booting_cpus()			1

Re: [NET]: Shut up warnings in net/core/flow.c

[Linus Torvalds]

On Wed, 23 Nov 2005, David S. Miller wrote:
> 
> 1) Mark all IPI functions with ifdef CONFIG_SMP, or
> 2) Mark them with __attribute__((__unused__))) which is what
>    the net/core/flow.c case does

We could certainly do some of both.

Add a new "__smp_only__" thing, and do something like

	#ifdef CONFIG_SMP
	  #define __smp_only__
	#else
	  #define __smp_only \
		__attribute__((__unused__, section("discard")))
	#endif

(Yeah, I didn't look up the section syntax, because I'm lazy, but you get 
the point - put it explicitly in some section that will be thrown away, 
and that we can make the build-checking tools verify isn't linked to).

How does that feel? It would waste a bit of compiler time to even look at 
the function, but at least it wouldn't warn, and they'd get thrown away 
_without_ having to rely on a smart compiler.

Quite frankly, every time we rely on some really smart gcc feature, we're 
burnt.

		Linus

Re: [NET]: Shut up warnings in net/core/flow.c

[David S. Miller]

From: Linus Torvalds <torvalds@osdl.org>
Date: Wed, 23 Nov 2005 15:29:53 -0800 (PST)

> Add a new "__smp_only__" thing, and do something like
> 
> 	#ifdef CONFIG_SMP
> 	  #define __smp_only__
> 	#else
> 	  #define __smp_only \
> 		__attribute__((__unused__, section("discard")))
> 	#endif
> 
> (Yeah, I didn't look up the section syntax, because I'm lazy, but you get 
> the point - put it explicitly in some section that will be thrown away, 
> and that we can make the build-checking tools verify isn't linked to).

If we mark it correctly, the linker will not even try to link it in to
the final object, and we'll get a flat out link failure.

Yes, something like this would be great.

Re: [NET]: Shut up warnings in net/core/flow.c

[Arjan van de Ven]

> Quite frankly, every time we rely on some really smart gcc feature, we're 
> burnt.


-ffunction-sections is a linker feature though ;)