----- Forwarded message from Sam Ravnborg <[email protected]> -----
Forgot lkml in first mail...
Sam
Subject: [RFC PATCH] kbuild: silence section mismatch warnings
From: Sam Ravnborg <[email protected]>
Date: Fri, 11 May 2007 23:03:46 +0200
User-Agent: Mutt/1.4.2.1i
To: Chris Wedgwood <[email protected]>, Andrew Morton <[email protected]>,
"David S. Miller" <[email protected]>,
Russell King <[email protected]>,
Satyam Sharma <[email protected]>
Cc: [email protected]
Following patch allow us in specific places to silence section mismatch warnings.
There is a few legitime places that modpost does not yet recognize where
reference from .text to .init.text (likewise for data) are legitime.
This allow us to spot the few places and annotate them so we do not
get false warnings that in the end will let real warnings pass.
The annotation is simple to grep for so revieing all uses in a few
months time are trivial. It is assumed that a few places will
use this to shut up the warning as replacement for the real fix.
But these cases are esay to spot and to fix up.
With this and the following two patches I have a section mismatch free
build.
The plan is that a section mismatch soon will graduate from a warning to an error.
This will hurt a few drives but then they have an incentive to fix it up.
Comments welcome.
Sam
diff --git a/include/linux/init.h b/include/linux/init.h
index 8bc32bb..3b63be5 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -45,6 +45,17 @@
#define __exitdata __attribute__ ((__section__(".exit.data")))
#define __exit_call __attribute_used__ __attribute__ ((__section__ (".exitcall.exit")))
+/* modpost check for references from .text to .init.text and likewise
+ * from .data to .init.data. They are in most cases sign of bugs but
+ * in a few places this is OK. The following can be used to tell
+ * modpost that such a reference is OK.
+ * For references to .exit.text and .exit.data the same annotation
+ * will silence warnings from modpost.
+ */
+#define __init_refok noinline __attribute__ ((__section__ (".text_initrefok")))
+#define __initdata_refok noinline __attribute__ ((__section__ (".data_initrefok")))
+
+
#ifdef MODULE
#define __exit __attribute__ ((__section__(".exit.text")))
#else
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 113dc77..986200b 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -582,6 +582,14 @@ static int strrcmp(const char *s, const char *sub)
/**
* Whitelist to allow certain references to pass with no warning.
+ *
+ * Pattern 0:
+ * Functions tagged with __init_refok => section .text_initrefok
+ * and data tagged with __initdata_refok => section .data_initrefok
+ * are marked so to tell modpost that they indeed may reference
+ * functions/data in the init and exit sections.
+ * So do not warn in these cases.
+ *
* Pattern 1:
* If a module parameter is declared __initdata and permissions=0
* then this is legal despite the warning generated.
@@ -683,6 +691,10 @@ static int secref_whitelist(const char *modname, const char *tosec,
"zone_wait_table_init",
NULL
};
+ /* Check for pattern 0 */
+ if ((strcmp(fromsec, ".text_initrefok") == 0) ||
+ (strcmp(fromsec, ".data_initrefok") == 0))
+ return 1;
/* Check for pattern 1 */
if (strcmp(tosec, ".init.data") != 0)
----- End forwarded message -----
Use the new markers to avoid the warnings from init/main (arm + i386)
and mm/slab (all archs).
With this the noise level is dratiscally reduced.
Sam
diff --git a/init/main.c b/init/main.c
index 1940fa7..1d8ea31 100644
--- a/init/main.c
+++ b/init/main.c
@@ -423,7 +423,7 @@ static void __init setup_command_line(char *command_line)
* gcc-3.4 accidentally inlines this function, so use noinline.
*/
-static void noinline rest_init(void)
+static void __init_refok noinline rest_init(void)
__releases(kernel_lock)
{
int pid;
diff --git a/mm/slab.c b/mm/slab.c
index 944b205..ef60834 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1960,7 +1960,7 @@ static void slab_destroy(struct kmem_cache *cachep, struct slab *slabp)
* For setting up all the kmem_list3s for cache whose buffer_size is same as
* size of kmem_list3.
*/
-static void __init set_up_list3s(struct kmem_cache *cachep, int index)
+static void __init_refok set_up_list3s(struct kmem_cache *cachep, int index)
{
int node;
On May 11, 2007, at 4:08 PM, Sam Ravnborg wrote:
> ----- Forwarded message from Sam Ravnborg <[email protected]> -----
>
> Forgot lkml in first mail...
>
> Sam
>
> Subject: [RFC PATCH] kbuild: silence section mismatch warnings
> From: Sam Ravnborg <[email protected]>
> Date: Fri, 11 May 2007 23:03:46 +0200
> User-Agent: Mutt/1.4.2.1i
> To: Chris Wedgwood <[email protected]>, Andrew Morton <[email protected]>,
> "David S. Miller" <[email protected]>,
> Russell King <[email protected]>,
> Satyam Sharma <[email protected]>
> Cc: [email protected]
>
> Following patch allow us in specific places to silence section
> mismatch warnings.
> There is a few legitime places that modpost does not yet recognize
> where
> reference from .text to .init.text (likewise for data) are legitime.
> This allow us to spot the few places and annotate them so we do not
> get false warnings that in the end will let real warnings pass.
>
> The annotation is simple to grep for so revieing all uses in a few
> months time are trivial. It is assumed that a few places will
> use this to shut up the warning as replacement for the real fix.
> But these cases are esay to spot and to fix up.
Its unclear if you expect that some things will be tagged
__init_refok/__initdata_refok forever or if we'll find some way to
fix/change the code so the things tagged no longer need it.
> With this and the following two patches I have a section mismatch free
> build.
> The plan is that a section mismatch soon will graduate from a
> warning to an error.
> This will hurt a few drives but then they have an incentive to fix
> it up.
>
> Comments welcome.
- k
Hi Sam,
> On May 11, 2007, at 4:08 PM, Sam Ravnborg wrote:
> > Following patch allow us in specific places to silence section
> > mismatch warnings.
Well, I had spelled out my reservations about this earlier, but I don't feel
too strongly. Most people probably do not want / prefer to see warnings
(even if they know they're false positives), and this also helps reduce
unnecessary reports of known FPs on lkml.
> > The annotation is simple to grep for so revieing all uses in a few
> > months time are trivial. It is assumed that a few places will
> > use this to shut up the warning as replacement for the real fix.
> > But these cases are esay to spot and to fix up.
Yes, so we have to be careful with its use.
On 5/12/07, Kumar Gala <[email protected]> wrote:
> Its unclear if you expect that some things will be tagged
> __init_refok/__initdata_refok forever or if we'll find some way to
> fix/change the code so the things tagged no longer need it.
We'll _have_ to fix those bugs that use the whitelisting in modpost
merely to kill off a warning, need to fix binutils for some others,
and may have to live with this for still others (mm/sl*b.c suffer
from a chicken-and-egg problem, for example).
> On May 11, 2007, at 4:08 PM, Sam Ravnborg wrote:
> With this and the following two patches I have a section mismatch free
> build.
> The plan is that a section mismatch soon will graduate from a
> warning to an error.
Yes, that's only sane.
> diff --git a/include/linux/init.h b/include/linux/init.h
> [...]
> +/* modpost check for references from .text to .init.text and likewise
> + * from .data to .init.data. They are in most cases sign of bugs but
You may want to list all illegal combinations in the comment above
check_sec_ref instead, and only introduce __init{data}_refok here.
> + * in a few places this is OK. The following can be used to tell
> + * modpost that such a reference is OK.
> + * For references to .exit.text and .exit.data the same annotation
> + * will silence warnings from modpost.
> + */
> +#define __init_refok noinline __attribute__ ((__section__ (".text_initrefok")))
> +#define __initdata_refok noinline __attribute__ ((__section__ (".data_initrefok")))
Actually, for a second there I got confused you had done this the other
way round. __init_refok sounds similar to __init (almost a "variant" of
__init) so I thought you were annotating the _callees_ and not the
_callers_.
BTW, I wonder if there would be any relative merits of doing things that
way. Did you consider this "reversed" approach?
Hmmm ... we would be annotating lesser functions, for one. With
the current __init_refok-for-callers semantics, we mark the callee __init
_and_ the caller __init_refok, which is unnecessary double-work, and
will only get worse if the same __init callee is called by multiple
callers in .text.
For the case where a caller references multiple __init callees? I don't
see any relative advantage of either scheme, or is there ...
Also, it's easier to spot a function that _is_ (or should be) __init
in the code already, than see if it is being referenced from .text,
and if so, make it __init_whatever (__init_refok is an equally
good name for callee-semantics).
(looking at code in 21-mm2)
I looked at scripts/mod/modpost.c only briefly, but it seems to me
shifting the semantics of __init_refok to refer to callees could also
subsume (and make redundant) patterns #1, 2, 6, 7 and 9 of
secref_whitelist, no?
[BTW I noticed _three_ whitelisting functions in there -- could it be
possible for us to do what init_section_ref_ok and
exit_section_ref_ok do in secref_whitelist itself? Those three
whitelists are beginning to look darn ugly.]
Anyway, somehow the __init_refok-callee scheme seems saner to
me -- please do consider it. Note, in that case, however:
1. We'll have to invent separate __exit_refok and __exitdata_refok,
of course. But that's a good thing for me, compared to giving a
multiplexed definition to __init_refok to mean
caller-can-safely-call-__init && caller-can-safely-call-__exit.
2. The init section freeing code in the kernel would need to be patched
to free sections marked as __init_refok, __initdata_refok, __exit_refok
and __exitdata_refok too. But that would most likely be a trivial patch.
3. When the exception / bug is fixed, we would convert such
__init_refok annotations to __init.
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 113dc77..986200b 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -582,6 +582,14 @@ static int strrcmp(const char *s, const char *sub)
>
> /**
^^^ should be simply /*
That's a doc-book-style comment header for a comment that's actually not
doc-book-style. Randy gets angry.
Thanks,
Satyam
On Fri, May 11, 2007 at 04:22:28PM -0500, Kumar Gala wrote:
>
> On May 11, 2007, at 4:08 PM, Sam Ravnborg wrote:
>
> >----- Forwarded message from Sam Ravnborg <[email protected]> -----
> >
> >Forgot lkml in first mail...
> >
> > Sam
> >
> >Subject: [RFC PATCH] kbuild: silence section mismatch warnings
> >From: Sam Ravnborg <[email protected]>
> >Date: Fri, 11 May 2007 23:03:46 +0200
> >User-Agent: Mutt/1.4.2.1i
> >To: Chris Wedgwood <[email protected]>, Andrew Morton <[email protected]>,
> > "David S. Miller" <[email protected]>,
> > Russell King <[email protected]>,
> > Satyam Sharma <[email protected]>
> >Cc: [email protected]
> >
> >Following patch allow us in specific places to silence section
> >mismatch warnings.
> >There is a few legitime places that modpost does not yet recognize
> >where
> >reference from .text to .init.text (likewise for data) are legitime.
> >This allow us to spot the few places and annotate them so we do not
> >get false warnings that in the end will let real warnings pass.
> >
> >The annotation is simple to grep for so revieing all uses in a few
> >months time are trivial. It is assumed that a few places will
> >use this to shut up the warning as replacement for the real fix.
> >But these cases are esay to spot and to fix up.
>
> Its unclear if you expect that some things will be tagged
> __init_refok/__initdata_refok forever or if we'll find some way to
> fix/change the code so the things tagged no longer need it.
A few places will need the __init_refok tag forever.
But as Satyam points out it will likely be misused.
So the __init_refok is introduced to stay.
akpm pointed out in private mail that I need to update the
linker scripts too - and running out of time this weekend so that
will be later.
Sam