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

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

[Ernie Petrides]

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

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


In a variation of a (loosely) 2.4.21-based kernel, I get a 0% degradation
in the speed of a program exec'ing itself 1,000,000 times with the addition
of the missing locking in exec_mmap():

--- linux-2.4.21/fs/exec.c.orig
+++ linux-2.4.21/fs/exec.c
@@ -452,9 +452,11 @@ static int exec_mmap(void)
 
 	old_mm = current->mm;
 	if (old_mm && atomic_read(&old_mm->mm_users) == 1) {
+		down_write(&old_mm->mmap_sem);
 		mm_release();
 		exit_aio(old_mm);
 		exit_mmap(old_mm);
+		up_write(&old_mm->mmap_sem);
 		return 0;
 	}
 

Applying your change to the same kernel, I get a 2.5% degradation in the
same test case:

--- linux-2.4.21/fs/exec.c.orig
+++ linux-2.4.21/fs/exec.c
@@ -451,12 +451,6 @@ static int exec_mmap(void)
 	struct mm_struct * mm, * old_mm;
 
 	old_mm = current->mm;
-	if (old_mm && atomic_read(&old_mm->mm_users) == 1) {
-		mm_release();
-		exit_aio(old_mm);
-		exit_mmap(old_mm);
-		return 0;
-	}
 
 	mm = mm_alloc();
 	if (mm) {


Average times over 3 runs (of 1,000,000 execs each) were:

	base kernel:	529 elapsed seconds
	w/1st change:	529 elapsed seconds
	w/2nd change:	542 elapsed seconds

I wouldn't bother optimizing for an exec() syscall, but the 1st change
also eliminates the possibility of two ENOMEM error paths in the typical
case of not sharing an mm_struct.


Also, a reproducer that could expose the race condition (in typically 5-20
seconds) on a dual-Xeon Dell box ran for 10's of minutes without problems
with either fix.

Cheers.  -ernie

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

[Andrea Arcangeli]

On Mon, Oct 13, 2003 at 02:52:44AM -0400, Ernie Petrides wrote:
> --- linux-2.4.21/fs/exec.c.orig
> +++ linux-2.4.21/fs/exec.c
> @@ -452,9 +452,11 @@ static int exec_mmap(void)
>  
>  	old_mm = current->mm;
>  	if (old_mm && atomic_read(&old_mm->mm_users) == 1) {
> +		down_write(&old_mm->mmap_sem);
>  		mm_release();
>  		exit_aio(old_mm);
>  		exit_mmap(old_mm);
> +		up_write(&old_mm->mmap_sem);
>  		return 0;
>  	}

Is there any special reason you take it around mm_release and exit_aio
too? I don't feel this is needed. exit_aio btw still assumes nobody can
race, so it doesn't take any spinlock (brlocks actually) to guard
against other aio threads, I believe that's ok since as worse the other
tasks can mangle the vm with ptrace, they'll never get to mess with aio,
only the current task can and the mm_user == 1 check guarantees we've no
sibiling threads. the mmap_sem shouldn't help exit_aio anyways, if
something it'll make it deadlock if there's any access to the VM that
generates a page fault in the cancel() callback.

So I suggest this sequence should be safe:

	mm_release();
	exit_aio(old_mm);

	down_write(&old_mm->mmap_sem);
	exit_mmap(old_mm);
	up_write(&old_mm->mmap_sem);

Please double check ;)

Andrea - If you prefer relying on open source software, check these links:
	    rsync.kernel.org::pub/scm/linux/kernel/bkcvs/linux-2.[45]/
	    http://www.cobite.com/cvsps/

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.