Running sparse on page_alloc.c today, it errors out:
mm/page_alloc.c:96:5: warning: symbol 'percpu_pagelist_fraction' was not declared. Should it be static?
mm/page_alloc.c:175:5: warning: symbol 'min_free_kbytes' was not declared. Should it be static?
include/linux/gfp.h:254:17: error: bad constant expression
include/linux/gfp.h:254:17: error: cannot size expression
which is a line in gfp_zone():
BUILD_BUG_ON((GFP_ZONE_BAD >> bit) & 1);
That's really unfortunate, because it ends up hiding all of the other
legitimate sparse messages (like I introduced with the appended patch):
mm/page_alloc.c:96:5: warning: symbol 'percpu_pagelist_fraction' was not declared. Should it be static?
mm/page_alloc.c:175:5: warning: symbol 'min_free_kbytes' was not declared. Should it be static?
mm/page_alloc.c:3692:15: warning: symbol '__early_pfn_to_nid' was not declared. Should it be static?
mm/page_alloc.c:5315:59: warning: incorrect type in argument 1 (different base types)
mm/page_alloc.c:5315:59: expected unsigned long [unsigned] [usertype] size
mm/page_alloc.c:5315:59: got restricted gfp_t [usertype] <noident>
...
Is sparse broken, or is that ? Even if it is, should we be working
around this somehow? It looks like we've basically crippled sparse in
some spots.
---
linux-2.6.git-dave/include/linux/gfp.h | 3 ++-
linux-2.6.git-dave/mm/page_alloc.c | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff -puN include/linux/gfp.h~fix-gfp_h-sparse include/linux/gfp.h
--- linux-2.6.git/include/linux/gfp.h~fix-gfp_h-sparse 2011-04-14 08:23:33.402280424 -0700
+++ linux-2.6.git-dave/include/linux/gfp.h 2011-04-14 08:32:33.782224008 -0700
@@ -249,7 +249,7 @@ static inline enum zone_type gfp_zone(gf
z = (GFP_ZONE_TABLE >> (bit * ZONES_SHIFT)) &
((1 << ZONES_SHIFT) - 1);
-
+/*
if (__builtin_constant_p(bit))
BUILD_BUG_ON((GFP_ZONE_BAD >> bit) & 1);
else {
@@ -257,6 +257,7 @@ static inline enum zone_type gfp_zone(gf
BUG_ON((GFP_ZONE_BAD >> bit) & 1);
#endif
}
+ */
return z;
}
diff -puN mm/page_alloc.c~fix-gfp_h-sparse mm/page_alloc.c
--- linux-2.6.git/mm/page_alloc.c~fix-gfp_h-sparse 2011-04-14 08:24:20.806271357 -0700
+++ linux-2.6.git-dave/mm/page_alloc.c 2011-04-14 08:24:35.554268529 -0700
@@ -5312,6 +5312,7 @@ void *__init alloc_large_system_hash(con
*/
if (get_order(size) < MAX_ORDER) {
table = alloc_pages_exact(size, GFP_ATOMIC);
+ table = alloc_pages_exact(GFP_ATOMIC, size);
kmemleak_alloc(table, size, 1, GFP_ATOMIC);
}
}
_
-- Dave
On Thu, 14 Apr 2011 08:41:35 -0700
Dave Hansen <[email protected]> wrote:
> Running sparse on page_alloc.c today, it errors out:
>
> mm/page_alloc.c:96:5: warning: symbol 'percpu_pagelist_fraction' was not declared. Should it be static?
> mm/page_alloc.c:175:5: warning: symbol 'min_free_kbytes' was not declared. Should it be static?
> include/linux/gfp.h:254:17: error: bad constant expression
> include/linux/gfp.h:254:17: error: cannot size expression
>
> which is a line in gfp_zone():
>
> BUILD_BUG_ON((GFP_ZONE_BAD >> bit) & 1);
>
> That's really unfortunate, because it ends up hiding all of the other
> legitimate sparse messages (like I introduced with the appended patch):
>
> mm/page_alloc.c:96:5: warning: symbol 'percpu_pagelist_fraction' was not declared. Should it be static?
> mm/page_alloc.c:175:5: warning: symbol 'min_free_kbytes' was not declared. Should it be static?
> mm/page_alloc.c:3692:15: warning: symbol '__early_pfn_to_nid' was not declared. Should it be static?
> mm/page_alloc.c:5315:59: warning: incorrect type in argument 1 (different base types)
> mm/page_alloc.c:5315:59: expected unsigned long [unsigned] [usertype] size
> mm/page_alloc.c:5315:59: got restricted gfp_t [usertype] <noident>
> ...
>
> Is sparse broken, or is that ? Even if it is, should we be working
> around this somehow? It looks like we've basically crippled sparse in
> some spots.
Is sparse having conniptions over that monster expression for
GFP_ZONE_BAD?
The kernel calls gfp_zone() with a constant arg in very few places.
This?
--- a/include/linux/gfp.h~a
+++ a/include/linux/gfp.h
@@ -249,14 +249,9 @@ static inline enum zone_type gfp_zone(gf
z = (GFP_ZONE_TABLE >> (bit * ZONES_SHIFT)) &
((1 << ZONES_SHIFT) - 1);
-
- if (__builtin_constant_p(bit))
- BUILD_BUG_ON((GFP_ZONE_BAD >> bit) & 1);
- else {
#ifdef CONFIG_DEBUG_VM
- BUG_ON((GFP_ZONE_BAD >> bit) & 1);
+ BUG_ON((GFP_ZONE_BAD >> bit) & 1);
#endif
- }
return z;
}
_
On Thu, 2011-04-14 at 13:22 -0700, Andrew Morton wrote:
> The kernel calls gfp_zone() with a constant arg in very few places.
> This?
>
> --- a/include/linux/gfp.h~a
> +++ a/include/linux/gfp.h
> @@ -249,14 +249,9 @@ static inline enum zone_type gfp_zone(gf
>
> z = (GFP_ZONE_TABLE >> (bit * ZONES_SHIFT)) &
> ((1 << ZONES_SHIFT) - 1);
> -
> - if (__builtin_constant_p(bit))
> - BUILD_BUG_ON((GFP_ZONE_BAD >> bit) & 1);
> - else {
> #ifdef CONFIG_DEBUG_VM
> - BUG_ON((GFP_ZONE_BAD >> bit) & 1);
> + BUG_ON((GFP_ZONE_BAD >> bit) & 1);
> #endif
> - }
> return z;
> }
That definitely makes sparse happier. I hope the folks on cc will chime
in if they wanted something special at build time.
-- Dave
On Thu, 14 Apr 2011, Dave Hansen wrote:
> On Thu, 2011-04-14 at 13:22 -0700, Andrew Morton wrote:
> > The kernel calls gfp_zone() with a constant arg in very few places.
> > This?
> >
> > --- a/include/linux/gfp.h~a
> > +++ a/include/linux/gfp.h
> > @@ -249,14 +249,9 @@ static inline enum zone_type gfp_zone(gf
> >
> > z = (GFP_ZONE_TABLE >> (bit * ZONES_SHIFT)) &
> > ((1 << ZONES_SHIFT) - 1);
> > -
> > - if (__builtin_constant_p(bit))
> > - BUILD_BUG_ON((GFP_ZONE_BAD >> bit) & 1);
> > - else {
> > #ifdef CONFIG_DEBUG_VM
> > - BUG_ON((GFP_ZONE_BAD >> bit) & 1);
> > + BUG_ON((GFP_ZONE_BAD >> bit) & 1);
> > #endif
> > - }
> > return z;
> > }
>
> That definitely makes sparse happier. I hope the folks on cc will chime
> in if they wanted something special at build time.
You can also remove the #ifdef. Use VM_BUG_ON.
On Fri, 2011-04-15 at 09:45 -0500, Christoph Lameter wrote:
> You can also remove the #ifdef. Use VM_BUG_ON.
Gotcha.
--
This goes on top of
include-linux-gfph-work-around-apparent-sparse-confusion.patch
already in the -mm tree.
VM_BUG_ON() if effectively a BUG_ON() undef #ifdef CONFIG_DEBUG_VM.
That is exactly what we have here now, and two different folks have
suggested doing it this way.
Signed-off-by: Dave Hansen <[email protected]>
---
linux-2.6.git-dave/include/linux/gfp.h | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff -puN include/linux/gfp.h~make-gfp_h-BUG_ON-in-too-VM_BUG_ON include/linux/gfp.h
--- linux-2.6.git/include/linux/gfp.h~make-gfp_h-BUG_ON-in-too-VM_BUG_ON 2011-04-15 10:59:24.192432223 -0700
+++ linux-2.6.git-dave/include/linux/gfp.h 2011-04-15 10:59:39.384429223 -0700
@@ -249,9 +249,7 @@ static inline enum zone_type gfp_zone(gf
z = (GFP_ZONE_TABLE >> (bit * ZONES_SHIFT)) &
((1 << ZONES_SHIFT) - 1);
-#ifdef CONFIG_DEBUG_VM
- BUG_ON((GFP_ZONE_BAD >> bit) & 1);
-#endif
+ VM_BUG_ON((GFP_ZONE_BAD >> bit) & 1);
return z;
}
_
-- Dave