[PATCH] mm: More __meminit annotations.

[PATCH] mm: More __meminit annotations.

[Paul Mundt]

Currently zone_spanned_pages_in_node() and zone_absent_pages_in_node()
are non-static for ARCH_POPULATES_NODE_MAP and static otherwise. However,
only the non-static versions are __meminit annotated, despite only being
called from __meminit functions in either case.

zone_init_free_lists() is currently non-static and not __meminit
annotated either, despite only being called once in the entire tree by
init_currently_empty_zone(), which too is __meminit. So make it static
and properly annotated.

Signed-off-by: Paul Mundt <lethal@linux-sh.org>

--

 mm/page_alloc.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index bd8e335..12dc471 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1953,8 +1953,8 @@ void __meminit memmap_init_zone(unsigned long size, int nid, unsigned long zone,
 	}
 }
 
-void zone_init_free_lists(struct pglist_data *pgdat, struct zone *zone,
-				unsigned long size)
+static void __meminit zone_init_free_lists(struct pglist_data *pgdat,
+				struct zone *zone, unsigned long size)
 {
 	int order;
 	for (order = 0; order < MAX_ORDER ; order++) {
@@ -2431,7 +2431,7 @@ void __meminit get_pfn_range_for_nid(unsigned int nid,
  * Return the number of pages a zone spans in a node, including holes
  * present_pages = zone_spanned_pages_in_node() - zone_absent_pages_in_node()
  */
-unsigned long __meminit zone_spanned_pages_in_node(int nid,
+static unsigned long __meminit zone_spanned_pages_in_node(int nid,
 					unsigned long zone_type,
 					unsigned long *ignored)
 {
@@ -2519,7 +2519,7 @@ unsigned long __init absent_pages_in_range(unsigned long start_pfn,
 }
 
 /* Return the number of page frames in holes in a zone on a node */
-unsigned long __meminit zone_absent_pages_in_node(int nid,
+static unsigned long __meminit zone_absent_pages_in_node(int nid,
 					unsigned long zone_type,
 					unsigned long *ignored)
 {
@@ -2536,14 +2536,14 @@ unsigned long __meminit zone_absent_pages_in_node(int nid,
 }
 
 #else
-static inline unsigned long zone_spanned_pages_in_node(int nid,
+static inline unsigned long __meminit zone_spanned_pages_in_node(int nid,
 					unsigned long zone_type,
 					unsigned long *zones_size)
 {
 	return zones_size[zone_type];
 }
 
-static inline unsigned long zone_absent_pages_in_node(int nid,
+static inline unsigned long __meminit zone_absent_pages_in_node(int nid,
 						unsigned long zone_type,
 						unsigned long *zholes_size)
 {

Re: [PATCH] mm: More __meminit annotations.

[Yasunori Goto]

Thanks for your checking.

> -void zone_init_free_lists(struct pglist_data *pgdat, struct zone *zone,
> -				unsigned long size)
> +static void __meminit zone_init_free_lists(struct pglist_data *pgdat,
> +				struct zone *zone, unsigned long size)
>  {
>  	int order;
>  	for (order = 0; order < MAX_ORDER ; order++) {
> @@ -2431,7 +2431,7 @@ void __meminit get_pfn_range_for_nid(unsigned int nid,
>   * Return the number of pages a zone spans in a node, including holes
>   * present_pages = zone_spanned_pages_in_node() - zone_absent_pages_in_node()
>   */
> -unsigned long __meminit zone_spanned_pages_in_node(int nid,
> +static unsigned long __meminit zone_spanned_pages_in_node(int nid,
>  					unsigned long zone_type,
>  					unsigned long *ignored)
>  {
> @@ -2519,7 +2519,7 @@ unsigned long __init absent_pages_in_range(unsigned long start_pfn,
>  }
>  
>  /* Return the number of page frames in holes in a zone on a node */
> -unsigned long __meminit zone_absent_pages_in_node(int nid,
> +static unsigned long __meminit zone_absent_pages_in_node(int nid,
>  					unsigned long zone_type,
>  					unsigned long *ignored)
>  {

Ah, Yes. Thanks. It is better.

> @@ -2536,14 +2536,14 @@ unsigned long __meminit zone_absent_pages_in_node(int nid,
>  }
>  
>  #else
> -static inline unsigned long zone_spanned_pages_in_node(int nid,
> +static inline unsigned long __meminit zone_spanned_pages_in_node(int nid,
>  					unsigned long zone_type,
>  					unsigned long *zones_size)
>  {
>  	return zones_size[zone_type];
>  }
>  
> -static inline unsigned long zone_absent_pages_in_node(int nid,
> +static inline unsigned long __meminit zone_absent_pages_in_node(int nid,
>  						unsigned long zone_type,
>  						unsigned long *zholes_size)
>  {

I thought __meminit is not effective for these static functions,
because they are inlined function. So, it depends on caller's 
defenition. Is it wrong? 

Bye.

-- 
Yasunori Goto 

Re: [PATCH] mm: More __meminit annotations.

[Paul Mundt]

On Mon, Jun 18, 2007 at 02:49:24PM +0900, Yasunori Goto wrote:
> > -static inline unsigned long zone_absent_pages_in_node(int nid,
> > +static inline unsigned long __meminit zone_absent_pages_in_node(int nid,
> >  						unsigned long zone_type,
> >  						unsigned long *zholes_size)
> >  {
> 
> I thought __meminit is not effective for these static functions,
> because they are inlined function. So, it depends on caller's 
> defenition. Is it wrong? 
> 
Ah, that's possible, I hadn't considered that. It seems to be a bit more
obvious what the intention is if it's annotated, especially as this is
the convention that's used by the rest of mm/page_alloc.c. A bit more
consistent, if nothing more.

Re: [PATCH] mm: More __meminit annotations.

[Yasunori Goto]

> On Mon, Jun 18, 2007 at 02:49:24PM +0900, Yasunori Goto wrote:
> > > -static inline unsigned long zone_absent_pages_in_node(int nid,
> > > +static inline unsigned long __meminit zone_absent_pages_in_node(int nid,
> > >  						unsigned long zone_type,
> > >  						unsigned long *zholes_size)
> > >  {
> > 
> > I thought __meminit is not effective for these static functions,
> > because they are inlined function. So, it depends on caller's 
> > defenition. Is it wrong? 
> > 
> Ah, that's possible, I hadn't considered that. It seems to be a bit more
> obvious what the intention is if it's annotated, especially as this is
> the convention that's used by the rest of mm/page_alloc.c. A bit more
> consistent, if nothing more.

I'm not sure which is intended. I found some functions define both
__init and inline in kernel tree. And probably, some functions don't
do it. So, it seems there is no convention.

I'm Okay if you prefer both defined. :-)


-- 
Yasunori Goto 

Re: [PATCH] mm: More __meminit annotations.

[Satyam Sharma]

Hi,

On 6/18/07, Yasunori Goto <y-goto@jp.fujitsu.com> wrote:
> > On Mon, Jun 18, 2007 at 02:49:24PM +0900, Yasunori Goto wrote:
> > > > -static inline unsigned long zone_absent_pages_in_node(int nid,
> > > > +static inline unsigned long __meminit zone_absent_pages_in_node(int nid,
> > > >                                           unsigned long zone_type,
> > > >                                           unsigned long *zholes_size)
> > > >  {
> > >
> > > I thought __meminit is not effective for these static functions,
> > > because they are inlined function. So, it depends on caller's
> > > defenition. Is it wrong?
> > >
> > Ah, that's possible, I hadn't considered that. It seems to be a bit more
> > obvious what the intention is if it's annotated, especially as this is
> > the convention that's used by the rest of mm/page_alloc.c. A bit more
> > consistent, if nothing more.
>
> I'm not sure which is intended. I found some functions define both
> __init and inline in kernel tree. And probably, some functions don't
> do it. So, it seems there is no convention.
>
> I'm Okay if you prefer both defined. :-)

Marking inline functions as __init (or __meminit etc) is quite insane,
IMHO. Note that all callers of the said inline function will also have to
be __init anyway (else modpost will barf) so the said function will
have all callsites in .init.text anyway, and hence would be inlined
in the same section as the caller (i.e. .init.text). [Note that kernel
uses always_inline.]

The annotation may still be a readability aid (which is subjective so
one can't really comment upon), but asking gcc to put into a separate
specified section, a function whose body would not be emitted by gcc
separately at all, doesn't really make much sense syntactically _or_
semantically -- gcc might not warn, of course, perhaps it's one of those
little things it takes care of by itself silently without complaining (like
taking pointers to inline functions).

Satyam

Re: [PATCH] mm: More __meminit annotations.

[Satyam Sharma]

On 6/18/07, Satyam Sharma <satyam.sharma@gmail.com> wrote:
> Hi,
>
> On 6/18/07, Yasunori Goto <y-goto@jp.fujitsu.com> wrote:
> > > On Mon, Jun 18, 2007 at 02:49:24PM +0900, Yasunori Goto wrote:
> > > > > -static inline unsigned long zone_absent_pages_in_node(int nid,
> > > > > +static inline unsigned long __meminit zone_absent_pages_in_node(int nid,
> > > > >                                           unsigned long zone_type,
> > > > >                                           unsigned long *zholes_size)
> > > > >  {
> > > >
> > > > I thought __meminit is not effective for these static functions,
> > > > because they are inlined function. So, it depends on caller's
> > > > defenition. Is it wrong?
> > > >
> > > Ah, that's possible, I hadn't considered that. It seems to be a bit more
> > > obvious what the intention is if it's annotated, especially as this is
> > > the convention that's used by the rest of mm/page_alloc.c. A bit more
> > > consistent, if nothing more.
> >
> > I'm not sure which is intended. I found some functions define both
> > __init and inline in kernel tree. And probably, some functions don't
> > do it. So, it seems there is no convention.
> >
> > I'm Okay if you prefer both defined. :-)
>
> Marking inline functions as __init (or __meminit etc) is quite insane,
> IMHO. Note that all callers of the said inline function will also have to
> be __init anyway (else modpost will barf)

Actually, modpost will _not_ complain precisely _because_ kernel
uses always_inline so a separate body for the function will never be
emitted at all. But all callers of said inline function will *still* need to
be in __init anyway, else if the said inline function itself calls some
__init function (which is likely) and the caller of the said inline function
is not __init *then* modpost will complain.

> so the said function will
> have all callsites in .init.text anyway, and hence would be inlined
> in the same section as the caller (i.e. .init.text). [Note that kernel
> uses always_inline.]
>
> The annotation may still be a readability aid (which is subjective so
> one can't really comment upon), but asking gcc to put into a separate
> specified section, a function whose body would not be emitted by gcc
> separately at all, doesn't really make much sense syntactically _or_
> semantically -- gcc might not warn, of course, perhaps it's one of those
> little things it takes care of by itself silently without complaining (like
> taking pointers to inline functions).

All this is valid, still. Perhaps sparse warns / can be made to warn about
such cases (which may not be bugs, but weird C, at least)?

Re: [PATCH] mm: More __meminit annotations.

[Sam Ravnborg]

On Mon, Jun 18, 2007 at 02:49:24PM +0900, Yasunori Goto wrote:
> >  }
> >  
> > -static inline unsigned long zone_absent_pages_in_node(int nid,
> > +static inline unsigned long __meminit zone_absent_pages_in_node(int nid,
> >  						unsigned long zone_type,
> >  						unsigned long *zholes_size)
> >  {
> 
> I thought __meminit is not effective for these static functions,
> because they are inlined function. So, it depends on caller's 
> defenition. Is it wrong? 

As we do not _know_ if a given function is inline or not it definitely
makes sense to mark them as __meminit.
If the compiler then decides to inline the function we are all clear and
no problems. If the compiler decides not to inline the function we will
properly discard the code after init has completed so again all clear.

And btw. some people (including myself) consider it a bug that gcc inline
a function that is forced to a specific section into a function that belongs
to another section. Now gcc people has another view but that may change.
So again defining a function as __meminit makes sense no matter the
section marker.

For the technical merit whay a function is marker inline in the first place.
It must be assumed this is a hot path where it is benificial to do so.

	Sam

Re: [PATCH] mm: More __meminit annotations.

[Sam Ravnborg]

On Mon, Jun 18, 2007 at 12:58:34PM +0530, Satyam Sharma wrote:
> 
> Actually, modpost will _not_ complain precisely _because_ kernel
> uses always_inline so a separate body for the function will never be
> emitted at all.
That has been threaten to change many times. Far far far too much
are marked inline today. There has been several longer threads about it.

Part of it is that some part MUST be inlined to work while other parts
may be inline but not needed (and often the wrong thing).

So a carefully added inline is good but the other 98% of inline
markings are just wrong and ougth to go.

	Sam

Re: [PATCH] mm: More __meminit annotations.

[Satyam Sharma]

Hi,

On 6/18/07, Sam Ravnborg <sam@ravnborg.org> wrote:
> On Mon, Jun 18, 2007 at 02:49:24PM +0900, Yasunori Goto wrote:
> > >  }
> > >
> > > -static inline unsigned long zone_absent_pages_in_node(int nid,
> > > +static inline unsigned long __meminit zone_absent_pages_in_node(int nid,
> > >                                             unsigned long zone_type,
> > >                                             unsigned long *zholes_size)
> > >  {
> >
> > I thought __meminit is not effective for these static functions,
> > because they are inlined function. So, it depends on caller's
> > defenition. Is it wrong?
>
> As we do not _know_ if a given function is inline or not it definitely
> makes sense to mark them as __meminit.
> If the compiler then decides to inline the function we are all clear and
> no problems. If the compiler decides not to inline the function we will
> properly discard the code after init has completed so again all clear.

The kernel uses always_inline (as of today), so I'd expect we do know
a function explicitly marked such would be inlined. But yes, if that
inline (or kernel's use of always_inline) is dropped from the code in
future, then we would need to add the __init at that time anyway,
so as long as gcc is doing the right thing given both inline and section,
we might introduce the __init now too as you suggest. [Might also help
readability / uniformity as Paul mentioned.]

> And btw. some people (including myself) consider it a bug that gcc inline
> a function that is forced to a specific section into a function that belongs
> to another section. Now gcc people has another view but that may change.
> So again defining a function as __meminit makes sense no matter the
> section marker.

Well, in-kernel, "inline __init" expands to (currently):
inline __attribute__((always_inline)) __attribute__((__section__(".init.text")))
which is weird, if not crazy, to say the least. So I'm not sure we can
find fault with gcc (regardless of what it does) given such usage :-)

On 6/18/07, Sam Ravnborg <sam@ravnborg.org> wrote:
> On Mon, Jun 18, 2007 at 12:58:34PM +0530, Satyam Sharma wrote:
> >
> > Actually, modpost will _not_ complain precisely _because_ kernel
> > uses always_inline so a separate body for the function will never be
> > emitted at all.
> That has been threaten to change many times. Far far far too much
> are marked inline today. There has been several longer threads about it.
>
> Part of it is that some part MUST be inlined to work while other parts
> may be inline but not needed (and often the wrong thing).
>
> So a carefully added inline is good but the other 98% of inline
> markings are just wrong and ougth to go.

Hmm, this is a bit orthogonal, and I have no strong opinion regarding
the kernel's use of always_inline itself. Those who don't like gcc
rewriting their code would want it to stay but forcing it for all cases
(by redefining a keyword as a macro) isn't best either. Perhaps "inline"
could be left alone by removing the always_inline, and "__always_inline"
used explicitly where we _really_ want stuff to be inlined. But then what
might happen is that everybody would think his particular use of inline
is correct and beneficial and all users of inline in kernel would end up
as __always_inline anyway. Given this, the best way to deal with
kernel bloat due to inlining appears to be to remove the inline marker
from stuff that shouldn't/needn't be inline in the first place, instead of
changing the kernel's default use of always_inline, IMO.

Satyam

Re: [PATCH] mm: More __meminit annotations.

[Sam Ravnborg]

> But then what
> might happen is that everybody would think his particular use of inline
> is correct and beneficial and all users of inline in kernel would end up
> as __always_inline anyway.

You miss that there is a big difference between "beneficial" and "needs".
The latter is used when some assembly code has a specific knowlegde of
how parameters are passed or that the function signature for other good
reasons must not change.
It has nothing to do with "beneficial".
Any use of __always_inline outside arch/* is highly question able.
And most use of *inline* in drivers/* today is due to bad behaving gcc in the past.

	Sam