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

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

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

[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, 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!

[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