[PATCH] fix improper .init-type section references

[PATCH] fix improper .init-type section references

[Jan Beulich]

.. which modpost started warning about.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

 kernel/kthread.c |    2 +-
 mm/page_alloc.c  |   18 +++++++++---------
 2 files changed, 10 insertions(+), 10 deletions(-)

--- linux-2.6.22-rc4/kernel/kthread.c	2007-06-11 18:09:47.000000000 +0200
+++ 2.6.22-rc4-init-attribs/kernel/kthread.c	2007-06-11 09:10:42.000000000 +0200
@@ -215,7 +215,7 @@ int kthread_stop(struct task_struct *k)
 EXPORT_SYMBOL(kthread_stop);
 
 
-static __init void kthreadd_setup(void)
+static noinline __init_refok void kthreadd_setup(void)
 {
 	struct task_struct *tsk = current;
 
--- linux-2.6.22-rc4/mm/page_alloc.c	2007-06-11 18:09:49.000000000 +0200
+++ 2.6.22-rc4-init-attribs/mm/page_alloc.c	2007-06-11 09:10:42.000000000 +0200
@@ -126,13 +126,13 @@ static unsigned long __meminitdata dma_r
     #endif
   #endif
 
-  struct node_active_region __meminitdata early_node_map[MAX_ACTIVE_REGIONS];
-  int __meminitdata nr_nodemap_entries;
-  unsigned long __meminitdata arch_zone_lowest_possible_pfn[MAX_NR_ZONES];
-  unsigned long __meminitdata arch_zone_highest_possible_pfn[MAX_NR_ZONES];
+  static struct node_active_region __meminitdata early_node_map[MAX_ACTIVE_REGIONS];
+  static int __meminitdata nr_nodemap_entries;
+  static unsigned long __meminitdata arch_zone_lowest_possible_pfn[MAX_NR_ZONES];
+  static unsigned long __meminitdata arch_zone_highest_possible_pfn[MAX_NR_ZONES];
 #ifdef CONFIG_MEMORY_HOTPLUG_RESERVE
-  unsigned long __initdata node_boundary_start_pfn[MAX_NUMNODES];
-  unsigned long __initdata node_boundary_end_pfn[MAX_NUMNODES];
+  static unsigned long __meminitdata node_boundary_start_pfn[MAX_NUMNODES];
+  static unsigned long __meminitdata node_boundary_end_pfn[MAX_NUMNODES];
 #endif /* CONFIG_MEMORY_HOTPLUG_RESERVE */
 #endif /* CONFIG_ARCH_POPULATES_NODE_MAP */
 
@@ -1968,7 +1968,7 @@ void zone_init_free_lists(struct pglist_
 	memmap_init_zone((size), (nid), (zone), (start_pfn), MEMMAP_EARLY)
 #endif
 
-static int __cpuinit zone_batchsize(struct zone *zone)
+static int __devinit zone_batchsize(struct zone *zone)
 {
 	int batch;
 
@@ -2370,7 +2370,7 @@ void __init push_node_boundaries(unsigne
 }
 
 /* If necessary, push the node boundary out for reserve hotadd */
-static void __init account_node_boundary(unsigned int nid,
+static void __meminit account_node_boundary(unsigned int nid,
 		unsigned long *start_pfn, unsigned long *end_pfn)
 {
 	printk(KERN_DEBUG "Entering account_node_boundary(%u, %lu, %lu)\n",
@@ -2390,7 +2390,7 @@ static void __init account_node_boundary
 void __init push_node_boundaries(unsigned int nid,
 		unsigned long start_pfn, unsigned long end_pfn) {}
 
-static void __init account_node_boundary(unsigned int nid,
+static void __meminit account_node_boundary(unsigned int nid,
 		unsigned long *start_pfn, unsigned long *end_pfn) {}
 #endif
 

Re: [PATCH] fix improper .init-type section references

[Satyam Sharma]

Hi Jan,

On 6/12/07, Jan Beulich <jbeulich@novell.com> wrote:
> .. which modpost started warning about.
>
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
>
>  kernel/kthread.c |    2 +-
>  mm/page_alloc.c  |   18 +++++++++---------
>  2 files changed, 10 insertions(+), 10 deletions(-)
>
> --- linux-2.6.22-rc4/kernel/kthread.c   2007-06-11 18:09:47.000000000 +0200
> +++ 2.6.22-rc4-init-attribs/kernel/kthread.c    2007-06-11 09:10:42.000000000 +0200
> @@ -215,7 +215,7 @@ int kthread_stop(struct task_struct *k)
>  EXPORT_SYMBOL(kthread_stop);
>
>
> -static __init void kthreadd_setup(void)
> +static noinline __init_refok void kthreadd_setup(void)
>  {
>         struct task_struct *tsk = current;

This isn't ok. There isn't any __init function that is (safely) referenced
by kthreadd_setup(), so we shouldn't really be marking it as such.
Also, kthreadd_setup() is really only ever called at init time, so we'd
want it to remain __init.

I believe the correct fix to silence modpost here would be to mark its
caller kthreadd() also as __init, because it too is used only at init time?

[ Cc:'ing Sam. ]

> --- linux-2.6.22-rc4/mm/page_alloc.c    2007-06-11 18:09:49.000000000 +0200
> +++ 2.6.22-rc4-init-attribs/mm/page_alloc.c     2007-06-11 09:10:42.000000000 +0200
> @@ -126,13 +126,13 @@ static unsigned long __meminitdata dma_r
>      #endif
>    #endif
>
> -  struct node_active_region __meminitdata early_node_map[MAX_ACTIVE_REGIONS];
> -  int __meminitdata nr_nodemap_entries;
> -  unsigned long __meminitdata arch_zone_lowest_possible_pfn[MAX_NR_ZONES];
> -  unsigned long __meminitdata arch_zone_highest_possible_pfn[MAX_NR_ZONES];
> +  static struct node_active_region __meminitdata early_node_map[MAX_ACTIVE_REGIONS];
> +  static int __meminitdata nr_nodemap_entries;
> +  static unsigned long __meminitdata arch_zone_lowest_possible_pfn[MAX_NR_ZONES];
> +  static unsigned long __meminitdata arch_zone_highest_possible_pfn[MAX_NR_ZONES];
>  #ifdef CONFIG_MEMORY_HOTPLUG_RESERVE
> -  unsigned long __initdata node_boundary_start_pfn[MAX_NUMNODES];
> -  unsigned long __initdata node_boundary_end_pfn[MAX_NUMNODES];
> +  static unsigned long __meminitdata node_boundary_start_pfn[MAX_NUMNODES];
> +  static unsigned long __meminitdata node_boundary_end_pfn[MAX_NUMNODES];

Umm, can't really comment on mm/ code, but this could've been a
separate patch ... (the simple non-static to static conversions, at least)

Satyam

Re: [PATCH] fix improper .init-type section references

[Jan Beulich]

>> -static __init void kthreadd_setup(void)
>> +static noinline __init_refok void kthreadd_setup(void)
>>  {
>>         struct task_struct *tsk = current;
>
>This isn't ok. There isn't any __init function that is (safely) referenced
>by kthreadd_setup(), so we shouldn't really be marking it as such.
>Also, kthreadd_setup() is really only ever called at init time, so we'd
>want it to remain __init.

Oh, I see, I misunderstood the purpose of the tag - I assumed it would
mark an __init function that is known to only be referenced from init-only
code paths inside non-init functions (i.e. I didn't pay attention that the
resulting section's name is .text.init.refok, not .init.text.refok).

I have to admit I have some difficulty understanding when the tags
are going to be useful the way they are implemented right now.

>I believe the correct fix to silence modpost here would be to mark its
>caller kthreadd() also as __init, because it too is used only at init time?

I don't think so - it is my understanding that this is the body of a thread
that never dies.

Jan

Re: [PATCH] fix improper .init-type section references

[Satyam Sharma]

On 6/12/07, Jan Beulich <jbeulich@novell.com> wrote:
> >> -static __init void kthreadd_setup(void)
> >> +static noinline __init_refok void kthreadd_setup(void)
> >>  {
> >>         struct task_struct *tsk = current;
> >
> >This isn't ok. There isn't any __init function that is (safely) referenced
> >by kthreadd_setup(), so we shouldn't really be marking it as such.
> >Also, kthreadd_setup() is really only ever called at init time, so we'd
> >want it to remain __init.
>
> Oh, I see, I misunderstood the purpose of the tag - I assumed it would
> mark an __init function that is known to only be referenced from init-only
> code paths inside non-init functions (i.e. I didn't pay attention that the
> resulting section's name is .text.init.refok, not .init.text.refok).
>
> I have to admit I have some difficulty understanding when the tags
> are going to be useful the way they are implemented right now.

Yup, I had discussed precisely the same issue (whether to associate
__init_refok with callers or callees) with Sam earlier, but he thought
it'd be more useful to have normal-caller-can-ref-init-callees semantics
for the same.

> >I believe the correct fix to silence modpost here would be to mark its
> >caller kthreadd() also as __init, because it too is used only at init time?
>
> I don't think so - it is my understanding that this is the body of a thread
> that never dies.

Ugh, yes, I'm smoking God-knows-what, and you're absolutely correct!

So we should be marking kthreadd() as __init_refok instead, it seems.

Satyam

Re: [PATCH] fix improper .init-type section references

[Satyam Sharma]

On 6/12/07, Satyam Sharma <satyam.sharma@gmail.com> wrote:
> On 6/12/07, Jan Beulich <jbeulich@novell.com> wrote:
> [...]
> > >I believe the correct fix to silence modpost here would be to mark its
> > >caller kthreadd() also as __init, because it too is used only at init time?
> >
> > I don't think so - it is my understanding that this is the body of a thread
> > that never dies.
>
> Ugh, yes, I'm smoking God-knows-what, and you're absolutely correct!
>
> So we should be marking kthreadd() as __init_refok instead, it seems.

Actually, kthreadd_setup() is a 4-liner with only one callsite. Not much
point marking it as __init. In fact, does it even need to be a separate
function at all? We could just get rid of kthreadd_setup() and open-code
those 4 lines in kthreadd() itself ...