[PATCH] BUG() in exec_mmap()

[PATCH] BUG() in exec_mmap()

[Dave Kleikamp]

Marcelo,
A recent change to exec_mmap() removed the initialization of old_mm,
leaving an uninitialized use of it.  This patch would completely remove
old_mm from the function.  Is this what was intended?

===== fs/exec.c 1.30 vs edited =====
--- 1.30/fs/exec.c	Thu Oct  9 08:48:43 2003
+++ edited/fs/exec.c	Thu Oct  9 14:58:42 2003
@@ -423,7 +423,7 @@
 
 static int exec_mmap(void)
 {
-	struct mm_struct * mm, * old_mm;
+	struct mm_struct * mm;
 
 	mm = mm_alloc();
 	if (mm) {
@@ -447,11 +447,6 @@
 		task_unlock(current);
 		activate_mm(active_mm, mm);
 		mm_release();
-		if (old_mm) {
-			if (active_mm != old_mm) BUG();
-			mmput(old_mm);
-			return 0;
-		}
 		mmdrop(active_mm);
 		return 0;
 	}

-- 
David Kleikamp
IBM Linux Technology Center

Re: [PATCH] BUG() in exec_mmap()

[Marcelo Tosatti]

On Thu, 9 Oct 2003, Dave Kleikamp wrote:

> Marcelo,
> A recent change to exec_mmap() removed the initialization of old_mm,
> leaving an uninitialized use of it.  This patch would completely remove
> old_mm from the function.  Is this what was intended?

Yes. 

Blame me... patch applied, thank you!

2.4.x performance tests Re: [PATCH] BUG() in exec_mmap()

[Marcelo Tosatti]

On Thu, 9 Oct 2003, Marcelo Tosatti wrote:

> 
> 
> On Thu, 9 Oct 2003, Dave Kleikamp wrote:
> 
> > Marcelo,
> > A recent change to exec_mmap() removed the initialization of old_mm,
> > leaving an uninitialized use of it.  This patch would completely remove
> > old_mm from the function.  Is this what was intended?
> 
> Yes. 
> 
> Blame me... patch applied, thank you!

BTW, further performance testing of the removal of this optimization is 
VERY welcome.

I've done some tests and no big performance harm has showed up, but thats 
just me.

Re: [PATCH] BUG() in exec_mmap()

[Mika Penttilä]

Marcelo Tosatti wrote:

>On Thu, 9 Oct 2003, Dave Kleikamp wrote:
>
>  
>
>>Marcelo,
>>A recent change to exec_mmap() removed the initialization of old_mm,
>>leaving an uninitialized use of it.  This patch would completely remove
>>old_mm from the function.  Is this what was intended?
>>    
>>
>
>Yes. 
>
>Blame me... patch applied, thank you!
>
>-
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/
>  
>

Hmm.. you still need to mmput(old_mm) etc, just remove the mm_users == 1 
optimization from the beginning of exec_mmap, so this patch is wrong!

--Mika

Re: [PATCH] BUG() in exec_mmap()

[Marcelo Tosatti]

On Thu, 9 Oct 2003, Mika Penttilä wrote:

> Hmm.. you still need to mmput(old_mm) etc, just remove the mm_users == 1 
> optimization from the beginning of exec_mmap, so this patch is wrong!

Right. Ill fix it up by hand.

Re: [PATCH] BUG() in exec_mmap()

[Ernie Petrides]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1763 bytes --]

On Friday, 10-Oct-2003 at 7:57 -0300, Marcelo Tosatti wrote:

> On Thu, 9 Oct 2003, Ernie Petrides wrote:
>
> > On Thursday, 9-Oct-2003 at 17:47 -0300, Marcelo Tosatti wrote:
> >
> > > On Thu, 9 Oct 2003, Mika Penttilä wrote:
> > >
> > > > Hmm.. you still need to mmput(old_mm) etc, just remove the mm_users == 1
> > > > optimization from the beginning of exec_mmap, so this patch is wrong!
> > >
> > > Right. Ill fix it up by hand.
> >
> > Mika is correct that the exit_mmap(old_mm) still needs to happen on the
> > last use of the "mm_struct".  But whether it's called directly from
> > exec_mmap() or indirectly from mmput() still needs to depend on the
> > value of "mm_users".
> >
> > The original logic avoided the mmdrop(active_mm) call if there was an
> > old_mm, so I'd infer that the mm_struct reference count is not bumped
> > twice for both references from the task_struct (mm and active_mm).  So
> > the patch would need to be reworked to avoid the double decrement, too.
>
> I dont get you, sorry (I'm not a real expert on that piece of code,
> so...).
>
> From what I understand the "if (old_mm && mm_users == 1)" if case is just
> an optimization to avoid the allocation of a new mm structure.
>
> The functionality will be the same without that piece of code, it will
> just be slower.

I've checked your -pre7 patch now, and I see that the 2nd block of code
was added back in.  So what you've got now should work fine.


> > Is it possible to just use down_read(&old_mm->mmap_sem) and
> > up_read(&old_mm->mmap_sem) inside exec_mmap() around the optimized call
> > to exit_mmap() instead?
>
> Doing that locking inside exit_mmap() not feasible IMO... it might be too
> expensive.

Okay, sounds good.  Thanks for the follow-up.


Cheers.  -ernie

Re: [PATCH] BUG() in exec_mmap()

[Marcelo Tosatti]

On Thu, 9 Oct 2003, Ernie Petrides wrote:

> On Thursday, 9-Oct-2003 at 17:47 -0300, Marcelo Tosatti wrote:
> 
> > On Thu, 9 Oct 2003, Mika Penttilä wrote:
> >
> > > Hmm.. you still need to mmput(old_mm) etc, just remove the mm_users == 1
> > > optimization from the beginning of exec_mmap, so this patch is wrong!
> >
> > Right. Ill fix it up by hand.
> 
> Mika is correct that the exit_mmap(old_mm) still needs to happen on the
> last use of the "mm_struct".  But whether it's called directly from
> exec_mmap() or indirectly from mmput() still needs to depend on the
> value of "mm_users".
> 
> The original logic avoided the mmdrop(active_mm) call if there was an
> old_mm, so I'd infer that the mm_struct reference count is not bumped
> twice for both references from the task_struct (mm and active_mm).  So
> the patch would need to be reworked to avoid the double decrement, too.

I dont get you, sorry (I'm not a real expert on that piece of code, 
so...).

>From what I understand the "if (old_mm && mm_users == 1)" if case is just 
an optimization to avoid the allocation of a new mm structure.

The functionality will be the same without that piece of code, it will 
just be slower. 

> Sorry I missed the discussion on the original changes.  Was there a
> race condition with another cpu gaining a reference in proc_pid_status()
> or access_process_vm() or something like that?  

Exactly.

> Is it possible to just use down_read(&old_mm->mmap_sem) and
> up_read(&old_mm->mmap_sem) inside exec_mmap() around the optimized call
> to exit_mmap() instead?

Doing that locking inside exit_mmap() not feasible IMO... it might be too 
expensive. 

Re: [PATCH] BUG() in exec_mmap()

[Ernie Petrides]

On Thursday, 9-Oct-2003 at 21:1 EDT, Ernie Petrides wrote:

> Sorry I missed the discussion on the original changes.  Was there a
> race condition with another cpu gaining a reference in proc_pid_status()
> or access_process_vm() or something like that?  Is it possible to just
> use down_read(&old_mm->mmap_sem) and up_read(&old_mm->mmap_sem) inside
> exec_mmap() around the optimized call to exit_mmap() instead?

I meant down_write() and up_write().  (But I haven't evaluated whether
this would be feasible.)

Cheers.  -ernie

Re: [PATCH] BUG() in exec_mmap()

[Ernie Petrides]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1171 bytes --]

On Thursday, 9-Oct-2003 at 17:47 -0300, Marcelo Tosatti wrote:

> On Thu, 9 Oct 2003, Mika Penttilä wrote:
>
> > Hmm.. you still need to mmput(old_mm) etc, just remove the mm_users == 1
> > optimization from the beginning of exec_mmap, so this patch is wrong!
>
> Right. Ill fix it up by hand.

Mika is correct that the exit_mmap(old_mm) still needs to happen on the
last use of the "mm_struct".  But whether it's called directly from
exec_mmap() or indirectly from mmput() still needs to depend on the
value of "mm_users".

The original logic avoided the mmdrop(active_mm) call if there was an
old_mm, so I'd infer that the mm_struct reference count is not bumped
twice for both references from the task_struct (mm and active_mm).  So
the patch would need to be reworked to avoid the double decrement, too.

Sorry I missed the discussion on the original changes.  Was there a
race condition with another cpu gaining a reference in proc_pid_status()
or access_process_vm() or something like that?  Is it possible to just
use down_read(&old_mm->mmap_sem) and up_read(&old_mm->mmap_sem) inside
exec_mmap() around the optimized call to exit_mmap() instead?

Cheers.  -ernie