[PATCH] mm: Fix boot crash in mm_alloc()

[PATCH] mm: Fix boot crash in mm_alloc()

[Ingo Molnar]

Would be nice to get the fix below into -rc1 as well, it triggers 
rather easily on bootup when CONFIG_CPUMASK_OFFSTACK is turned on.

	Ingo

---------------------->

Re: [PATCH] mm: Fix boot crash in mm_alloc()

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1698 bytes --]

On Sun, May 29, 2011 at 12:22 AM, Ingo Molnar <mingo@elte.hu> wrote:
>
> Would be nice to get the fix below into -rc1 as well, it triggers
> rather easily on bootup when CONFIG_CPUMASK_OFFSTACK is turned on.

Looking at that commit de03c72cfce5, it looks odd in other ways too.

For example, it looks like mm_cpumask is always initialized to zero.
That's a bit odd, isn't it, since it *used* to be initialized
statically with this:

-       .cpu_vm_mask    = CPU_MASK_ALL,

which is rather different from zero.

Now, I'm sure the init mm_cpumask doesn't really matter, but I'd have
expected a commentary about it.

I also wonder if that whole conversion to cpumask_var_t was worth it,
since clearly it wasn't very well tested. It results in an extra
allocation at fork() time for the many-cpu case, and I do get the
feeling that we would have been better off keeping the cpumask inside
the mm_struct. Moving it to the end of mm_struct makes sense for the
many-cpu case, but at the same time I end up wondering what it does to
the switch_mm() cache behavior. (And perhaps the TLB flush IPI cache
activity).

Ho humm. I have this suspicion that that whole patch wasn't fully
thought out, and that I should revert it rather than fix the oops.

Or, in fact, we could just do something like the attached (UNTESTED!)
which does the whole "move big allocation to end, but keep the
cpumask_var_t at the beginning, and don't do any extra allocations"
thing.

NOTE NOTE NOTE! Not only is the attached patch untested, but please
see the added FIXME comment about the whole mm_struct
kmem_cache_create(). Right now we always allocate the whole
maximum-sized bitmap.

Comments?

                          Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 4695 bytes --]

 include/linux/mm_types.h |   14 +++++++++++---
 include/linux/sched.h    |    1 -
 init/main.c              |    2 +-
 kernel/fork.c            |   39 ++++++++++-----------------------------
 4 files changed, 22 insertions(+), 34 deletions(-)

diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 2a78aae78c69..f4e9bb17bdf2 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -243,7 +243,7 @@ struct mm_struct {
 						 * together off init_mm.mmlist, and are protected
 						 * by mmlist_lock
 						 */
-
+	cpumask_var_t cpu_vm_mask_var;
 
 	unsigned long hiwater_rss;	/* High-watermark of RSS usage */
 	unsigned long hiwater_vm;	/* High-water virtual memory usage */
@@ -311,10 +311,18 @@ struct mm_struct {
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 	pgtable_t pmd_huge_pte; /* protected by page_table_lock */
 #endif
-
-	cpumask_var_t cpu_vm_mask_var;
+#ifdef CONFIG_CPUMASK_OFFSTACK
+	struct cpumask cpumask_allocation;
+#endif
 };
 
+static inline void mm_init_cpumask(struct mm_struct *mm)
+{
+#ifdef CONFIG_CPUMASK_OFFSTACK
+	mm->cpu_vm_mask_var = &mm->cpumask_allocation;
+#endif
+}
+
 /* Future-safe accessor for struct mm_struct's cpu_vm_mask. */
 static inline cpumask_t *mm_cpumask(struct mm_struct *mm)
 {
diff --git a/include/linux/sched.h b/include/linux/sched.h
index bcddd0138105..2a8621c4be1e 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2194,7 +2194,6 @@ static inline void mmdrop(struct mm_struct * mm)
 	if (unlikely(atomic_dec_and_test(&mm->mm_count)))
 		__mmdrop(mm);
 }
-extern int mm_init_cpumask(struct mm_struct *mm, struct mm_struct *oldmm);
 
 /* mmput gets rid of the mappings and all user-space */
 extern void mmput(struct mm_struct *);
diff --git a/init/main.c b/init/main.c
index d2f1e086bf33..cafba67c13bf 100644
--- a/init/main.c
+++ b/init/main.c
@@ -487,6 +487,7 @@ asmlinkage void __init start_kernel(void)
 	printk(KERN_NOTICE "%s", linux_banner);
 	setup_arch(&command_line);
 	mm_init_owner(&init_mm, &init_task);
+	mm_init_cpumask(&init_mm);
 	setup_command_line(command_line);
 	setup_nr_cpu_ids();
 	setup_per_cpu_areas();
@@ -510,7 +511,6 @@ asmlinkage void __init start_kernel(void)
 	sort_main_extable();
 	trap_init();
 	mm_init();
-	BUG_ON(mm_init_cpumask(&init_mm, 0));
 
 	/*
 	 * Set up the scheduler prior starting any interrupts (such as the
diff --git a/kernel/fork.c b/kernel/fork.c
index ca406d916713..d30c792a83a2 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -484,20 +484,6 @@ static void mm_init_aio(struct mm_struct *mm)
 #endif
 }
 
-int mm_init_cpumask(struct mm_struct *mm, struct mm_struct *oldmm)
-{
-#ifdef CONFIG_CPUMASK_OFFSTACK
-	if (!alloc_cpumask_var(&mm->cpu_vm_mask_var, GFP_KERNEL))
-		return -ENOMEM;
-
-	if (oldmm)
-		cpumask_copy(mm_cpumask(mm), mm_cpumask(oldmm));
-	else
-		memset(mm_cpumask(mm), 0, cpumask_size());
-#endif
-	return 0;
-}
-
 static struct mm_struct * mm_init(struct mm_struct * mm, struct task_struct *p)
 {
 	atomic_set(&mm->mm_users, 1);
@@ -538,17 +524,8 @@ struct mm_struct * mm_alloc(void)
 		return NULL;
 
 	memset(mm, 0, sizeof(*mm));
-	mm = mm_init(mm, current);
-	if (!mm)
-		return NULL;
-
-	if (mm_init_cpumask(mm, NULL)) {
-		mm_free_pgd(mm);
-		free_mm(mm);
-		return NULL;
-	}
-
-	return mm;
+	mm_init_cpumask(mm);
+	return mm_init(mm, current);
 }
 
 /*
@@ -753,6 +730,7 @@ struct mm_struct *dup_mm(struct task_struct *tsk)
 		goto fail_nomem;
 
 	memcpy(mm, oldmm, sizeof(*mm));
+	mm_init_cpumask(mm);
 
 	/* Initializing for Swap token stuff */
 	mm->token_priority = 0;
@@ -765,9 +743,6 @@ struct mm_struct *dup_mm(struct task_struct *tsk)
 	if (!mm_init(mm, tsk))
 		goto fail_nomem;
 
-	if (mm_init_cpumask(mm, oldmm))
-		goto fail_nocpumask;
-
 	if (init_new_context(tsk, mm))
 		goto fail_nocontext;
 
@@ -796,7 +771,6 @@ fail_nomem:
 fail_nocontext:
 	free_cpumask_var(mm->cpu_vm_mask_var);
 
-fail_nocpumask:
 	/*
 	 * If init_new_context() failed, we cannot use mmput() to free the mm
 	 * because it calls destroy_context()
@@ -1591,6 +1565,13 @@ void __init proc_caches_init(void)
 	fs_cachep = kmem_cache_create("fs_cache",
 			sizeof(struct fs_struct), 0,
 			SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_NOTRACK, NULL);
+	/*
+	 * FIXME! The "sizeof(struct mm_struct)" currently includes the
+	 * whole struct cpumask for the OFFSTACK case. We could change
+	 * this to *only* allocate as much of it as required by the
+	 * maximum number of CPU's we can ever have.  The cpumask_allocation
+	 * is at the end of the structure, exactly for that reason.
+	 */
 	mm_cachep = kmem_cache_create("mm_struct",
 			sizeof(struct mm_struct), ARCH_MIN_MMSTRUCT_ALIGN,
 			SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_NOTRACK, NULL);

Re: [PATCH] mm: Fix boot crash in mm_alloc()

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 850 bytes --]

On Sun, May 29, 2011 at 9:22 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Or, in fact, we could just do something like the attached (UNTESTED!)

So I did warn you that it was untested.

It still is, but I walked through it a bit more, and I realized that
while I had gotten rid of the extra allocations of the
cpu_vm_mask_var, I hadn't gotten rid of the freeing.

So that patch would definitely not have worked very well with
CONFIG_CPUMASK_OFFSTACK.

And I noticed that I moved the cpu_vm_mask back in the wrong space, it
should likely be as close as possible to the mm_context_t, since the
main user is likely the task switching code that touches that anyway.

So here's a slightly updated patch.

STILL TOTALLY UNTESTED! The fixes were just from eyeballing it a bit
more, not from any actual testing.

                      Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 4827 bytes --]

 include/linux/mm_types.h |   14 ++++++++++++--
 include/linux/sched.h    |    1 -
 init/main.c              |    2 +-
 kernel/fork.c            |   42 ++++++++++--------------------------------
 4 files changed, 23 insertions(+), 36 deletions(-)

diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 2a78aae78c69..027935c86c68 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -264,6 +264,8 @@ struct mm_struct {
 
 	struct linux_binfmt *binfmt;
 
+	cpumask_var_t cpu_vm_mask_var;
+
 	/* Architecture-specific MM context */
 	mm_context_t context;
 
@@ -311,10 +313,18 @@ struct mm_struct {
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 	pgtable_t pmd_huge_pte; /* protected by page_table_lock */
 #endif
-
-	cpumask_var_t cpu_vm_mask_var;
+#ifdef CONFIG_CPUMASK_OFFSTACK
+	struct cpumask cpumask_allocation;
+#endif
 };
 
+static inline void mm_init_cpumask(struct mm_struct *mm)
+{
+#ifdef CONFIG_CPUMASK_OFFSTACK
+	mm->cpu_vm_mask_var = &mm->cpumask_allocation;
+#endif
+}
+
 /* Future-safe accessor for struct mm_struct's cpu_vm_mask. */
 static inline cpumask_t *mm_cpumask(struct mm_struct *mm)
 {
diff --git a/include/linux/sched.h b/include/linux/sched.h
index bcddd0138105..2a8621c4be1e 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2194,7 +2194,6 @@ static inline void mmdrop(struct mm_struct * mm)
 	if (unlikely(atomic_dec_and_test(&mm->mm_count)))
 		__mmdrop(mm);
 }
-extern int mm_init_cpumask(struct mm_struct *mm, struct mm_struct *oldmm);
 
 /* mmput gets rid of the mappings and all user-space */
 extern void mmput(struct mm_struct *);
diff --git a/init/main.c b/init/main.c
index d2f1e086bf33..cafba67c13bf 100644
--- a/init/main.c
+++ b/init/main.c
@@ -487,6 +487,7 @@ asmlinkage void __init start_kernel(void)
 	printk(KERN_NOTICE "%s", linux_banner);
 	setup_arch(&command_line);
 	mm_init_owner(&init_mm, &init_task);
+	mm_init_cpumask(&init_mm);
 	setup_command_line(command_line);
 	setup_nr_cpu_ids();
 	setup_per_cpu_areas();
@@ -510,7 +511,6 @@ asmlinkage void __init start_kernel(void)
 	sort_main_extable();
 	trap_init();
 	mm_init();
-	BUG_ON(mm_init_cpumask(&init_mm, 0));
 
 	/*
 	 * Set up the scheduler prior starting any interrupts (such as the
diff --git a/kernel/fork.c b/kernel/fork.c
index ca406d916713..0276c30401a0 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -484,20 +484,6 @@ static void mm_init_aio(struct mm_struct *mm)
 #endif
 }
 
-int mm_init_cpumask(struct mm_struct *mm, struct mm_struct *oldmm)
-{
-#ifdef CONFIG_CPUMASK_OFFSTACK
-	if (!alloc_cpumask_var(&mm->cpu_vm_mask_var, GFP_KERNEL))
-		return -ENOMEM;
-
-	if (oldmm)
-		cpumask_copy(mm_cpumask(mm), mm_cpumask(oldmm));
-	else
-		memset(mm_cpumask(mm), 0, cpumask_size());
-#endif
-	return 0;
-}
-
 static struct mm_struct * mm_init(struct mm_struct * mm, struct task_struct *p)
 {
 	atomic_set(&mm->mm_users, 1);
@@ -538,17 +524,8 @@ struct mm_struct * mm_alloc(void)
 		return NULL;
 
 	memset(mm, 0, sizeof(*mm));
-	mm = mm_init(mm, current);
-	if (!mm)
-		return NULL;
-
-	if (mm_init_cpumask(mm, NULL)) {
-		mm_free_pgd(mm);
-		free_mm(mm);
-		return NULL;
-	}
-
-	return mm;
+	mm_init_cpumask(mm);
+	return mm_init(mm, current);
 }
 
 /*
@@ -559,7 +536,6 @@ struct mm_struct * mm_alloc(void)
 void __mmdrop(struct mm_struct *mm)
 {
 	BUG_ON(mm == &init_mm);
-	free_cpumask_var(mm->cpu_vm_mask_var);
 	mm_free_pgd(mm);
 	destroy_context(mm);
 	mmu_notifier_mm_destroy(mm);
@@ -753,6 +729,7 @@ struct mm_struct *dup_mm(struct task_struct *tsk)
 		goto fail_nomem;
 
 	memcpy(mm, oldmm, sizeof(*mm));
+	mm_init_cpumask(mm);
 
 	/* Initializing for Swap token stuff */
 	mm->token_priority = 0;
@@ -765,9 +742,6 @@ struct mm_struct *dup_mm(struct task_struct *tsk)
 	if (!mm_init(mm, tsk))
 		goto fail_nomem;
 
-	if (mm_init_cpumask(mm, oldmm))
-		goto fail_nocpumask;
-
 	if (init_new_context(tsk, mm))
 		goto fail_nocontext;
 
@@ -794,9 +768,6 @@ fail_nomem:
 	return NULL;
 
 fail_nocontext:
-	free_cpumask_var(mm->cpu_vm_mask_var);
-
-fail_nocpumask:
 	/*
 	 * If init_new_context() failed, we cannot use mmput() to free the mm
 	 * because it calls destroy_context()
@@ -1591,6 +1562,13 @@ void __init proc_caches_init(void)
 	fs_cachep = kmem_cache_create("fs_cache",
 			sizeof(struct fs_struct), 0,
 			SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_NOTRACK, NULL);
+	/*
+	 * FIXME! The "sizeof(struct mm_struct)" currently includes the
+	 * whole struct cpumask for the OFFSTACK case. We could change
+	 * this to *only* allocate as much of it as required by the
+	 * maximum number of CPU's we can ever have.  The cpumask_allocation
+	 * is at the end of the structure, exactly for that reason.
+	 */
 	mm_cachep = kmem_cache_create("mm_struct",
 			sizeof(struct mm_struct), ARCH_MIN_MMSTRUCT_ALIGN,
 			SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_NOTRACK, NULL);

Re: [PATCH] mm: Fix boot crash in mm_alloc()

[Linus Torvalds]

On Sun, May 29, 2011 at 10:19 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> STILL TOTALLY UNTESTED! The fixes were just from eyeballing it a bit
> more, not from any actual testing.

Ok, I eyeballed it some more, and tested both the OFFSTACK and ONSTACK
case, and decided that I had better commit it now rather than wait any
later since I'll do the -rc1 later today, and will be on an airplane
most of tomorrow.

The exact placement of the cpu_vm_mask_var is up for grabs. For
example, I started thinking that it might be better to put it *after*
the mm_context_t, since for the non-OFFSTACK case it's generally
touched at the beginning rather than the end.

And the actual change to make the mm_cachep kmem_cache_create() use a
variable-sized allocation for the OFFSTACK case is similarly left as
an exercise for the the reader. So effectively, this reverts a lot of
de03c72cfce5, but does so in a way that should make very it easy to
get back to where KOSAKI was aiming for.

Whatever. I was hoping to get comments on it, but I think I need to
rather push it out to get tested and public than wait any longer. The
patch *looks* fine, tests ok on my machine, and removes more lines
than it adds despite the new big comment.

                    Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: Fix boot crash in mm_alloc()

[KOSAKI Motohiro]

(2011/05/30 3:43), Linus Torvalds wrote:
> On Sun, May 29, 2011 at 10:19 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> STILL TOTALLY UNTESTED! The fixes were just from eyeballing it a bit
>> more, not from any actual testing.
> 
> Ok, I eyeballed it some more, and tested both the OFFSTACK and ONSTACK
> case, and decided that I had better commit it now rather than wait any
> later since I'll do the -rc1 later today, and will be on an airplane
> most of tomorrow.
> 
> The exact placement of the cpu_vm_mask_var is up for grabs. For
> example, I started thinking that it might be better to put it *after*
> the mm_context_t, since for the non-OFFSTACK case it's generally
> touched at the beginning rather than the end.
> 
> And the actual change to make the mm_cachep kmem_cache_create() use a
> variable-sized allocation for the OFFSTACK case is similarly left as
> an exercise for the the reader. So effectively, this reverts a lot of
> de03c72cfce5, but does so in a way that should make very it easy to
> get back to where KOSAKI was aiming for.
> 
> Whatever. I was hoping to get comments on it, but I think I need to
> rather push it out to get tested and public than wait any longer. The
> patch *looks* fine, tests ok on my machine, and removes more lines
> than it adds despite the new big comment.

Hi

Thank you Linus and I'm sorry for bother you and guys. So, if I understand
this thread correctly, rest my homework is 1) make cpumask_allocation variable
size 2) remove NR_CPUS bit fill/copy from fork/exec path. Right?

I think (2) is big matter than (1). NR_CPUS(=4096) bits copy easily screw up
cache behavior. Anyway, will do. Thank you!



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: Fix boot crash in mm_alloc()

[Ingo Molnar]

* KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> (2011/05/30 3:43), Linus Torvalds wrote:
> > On Sun, May 29, 2011 at 10:19 AM, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> >>
> >> STILL TOTALLY UNTESTED! The fixes were just from eyeballing it a bit
> >> more, not from any actual testing.
> > 
> > Ok, I eyeballed it some more, and tested both the OFFSTACK and ONSTACK
> > case, and decided that I had better commit it now rather than wait any
> > later since I'll do the -rc1 later today, and will be on an airplane
> > most of tomorrow.
> > 
> > The exact placement of the cpu_vm_mask_var is up for grabs. For
> > example, I started thinking that it might be better to put it *after*
> > the mm_context_t, since for the non-OFFSTACK case it's generally
> > touched at the beginning rather than the end.
> > 
> > And the actual change to make the mm_cachep kmem_cache_create() use a
> > variable-sized allocation for the OFFSTACK case is similarly left as
> > an exercise for the the reader. So effectively, this reverts a lot of
> > de03c72cfce5, but does so in a way that should make very it easy to
> > get back to where KOSAKI was aiming for.
> > 
> > Whatever. I was hoping to get comments on it, but I think I need to
> > rather push it out to get tested and public than wait any longer. The
> > patch *looks* fine, tests ok on my machine, and removes more lines
> > than it adds despite the new big comment.
> 
> Hi
> 
> Thank you Linus and I'm sorry for bother you and guys. So, if I 
> understand this thread correctly, rest my homework is 1) make 
> cpumask_allocation variable size 2) remove NR_CPUS bit fill/copy 
> from fork/exec path. Right?
> 
> I think (2) is big matter than (1). NR_CPUS(=4096) bits copy easily 
> screw up cache behavior. Anyway, will do. Thank you!

I think the first task would be to double check that the code in 
3.0-rc1 is indeed correct! :-)

Thanks,

	Ingo

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>