mm: More __meminit annotations.

PATCH In review Paul Mundt 4 participants 10 messages Jun 18, 2007

Subsystems (2)

Memory management Page allocator

lore

Share & tools

lore

Paul Mundt

author

2007-06-18 04:52 UTC #0 lore raw

PATCH 1/1 mm: More __meminit annotations.

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 <[email protected]>

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

mm/page_alloc.c +6 -6

index bd8e335..12dc471 100644
@@ -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)
 {

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Yasunori Goto

2007-06-18 05:49 UTC #1 lore raw

Thanks for your checking.

Quoted 17 lines

mm/page_alloc.c +3 -3

-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)
 {

mm/page_alloc.c unsigned long __init absent_pages_in_range(unsigned long start_pfn, · line 2525

Quoted 8 lines

mm/page_alloc.c +1 -1

@@ -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.

Quoted 7 lines

mm/page_alloc.c +1 -1

@@ -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,

mm/page_alloc.c unsigned long __meminit zone_absent_pages_in_node(int nid, · line 2549

Quoted 10 lines

mm/page_alloc.c +1 -1

 					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.

Signature · 8 lines

Yasunori Goto 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Paul Mundt

author

2007-06-18 05:58 UTC #2 lore raw

Quoted 10 lines · Yasunori Goto

mm/page_alloc.c unsigned long __meminit zone_absent_pages_in_node(int nid, · line 2549
Quoted 5 lines
mm/page_alloc.c +1 -1
-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.

Yasunori Goto

2007-06-18 06:33 UTC #3 lore raw

Quoted 11 lines · Yasunori Goto

Quoted 10 lines
mm/page_alloc.c unsigned long __meminit zone_absent_pages_in_node(int nid, · line 2549
Quoted 5 lines
mm/page_alloc.c +1 -1
-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?

Quoted 4 lines

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. :-)

Signature · 8 lines

Yasunori Goto 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Satyam Sharma

2007-06-18 06:57 UTC #4 lore raw

Hi,

Quoted 16 lines · Yasunori Goto

Quoted 11 lines · Yasunori Goto
Quoted 10 lines
mm/page_alloc.c unsigned long __meminit zone_absent_pages_in_node(int nid, · line 2549
Quoted 5 lines
mm/page_alloc.c +1 -1
-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?
Quoted 4 lines
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.

Quoted 5 lines

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

Satyam Sharma

2007-06-18 07:28 UTC #5 lore raw

Quoted 23 lines · Satyam Sharma

Hi,
Quoted 20 lines · Yasunori Goto
Quoted 11 lines · Yasunori Goto
Quoted 10 lines
mm/page_alloc.c unsigned long __meminit zone_absent_pages_in_node(int nid, · line 2549
Quoted 5 lines
mm/page_alloc.c +1 -1
-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?
Quoted 4 lines
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.

Quoted 5 lines

Quoted 1 line
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.

Quoted 5 lines

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.]

Quoted 7 lines

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)?

Sam Ravnborg

2007-06-18 07:48 UTC #6 lore raw

Quoted 4 lines · Satyam Sharma

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

Sam Ravnborg

2007-06-18 07:44 UTC #7 lore raw

In reply to Yasunori Goto

Quoted 11 lines · Yasunori Goto

mm/page_alloc.c unsigned long __meminit zone_absent_pages_in_node(int nid, · line 2549
Quoted 7 lines
}
mm/page_alloc.c +1 -1
-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

Satyam Sharma

2007-06-18 10:29 UTC #8 lore raw

Hi,

Quoted 13 lines · Sam Ravnborg

Quoted 11 lines · Yasunori Goto
mm/page_alloc.c unsigned long __meminit zone_absent_pages_in_node(int nid, · line 2549
Quoted 7 lines
}
mm/page_alloc.c +1 -1
-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?

Quoted 5 lines

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.]

Quoted 5 lines

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 :-)

Quoted 5 lines · Sam Ravnborg

Quoted 4 lines · Satyam Sharma
Actually, modpost will _not_ complain precisely _because_ kernel uses always_inline so a separate body for the function will never be emitted at all.

Quoted 8 lines

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

Sam Ravnborg

2007-06-18 10:53 UTC #9 lore raw

Quoted 6 lines

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