[PATCH] kanoj-mm2.0-2.2.9 unneccesary page force in by munlock

[PATCH] kanoj-mm2.0-2.2.9 unneccesary page force in by munlock

[Kanoj Sarcar]

While looking at the code for munlock() in mm/mlock.c, I found
that munlock() unneccesarily executes a code path that forces
page fault in over the entire range. The following patch fixes 
this problem:

--- /usr/tmp/p_rdiff_a007Qs/mlock.c     Sun May 16 20:48:12 1999
+++ mm/mlock.c  Sun May 16 20:47:01 1999
@@ -117,8 +117,9 @@
                pages = (end - start) >> PAGE_SHIFT;
                if (!(newflags & VM_LOCKED))
                        pages = -pages;
+               else
+                       make_pages_present(start, end);
                vma->vm_mm->locked_vm += pages;
-               make_pages_present(start, end);
        }
        return retval;
 }

Please review and include in the source.

Thanks.

Kanoj
kanoj@engr.sgi.com
--
To unsubscribe, send a message with 'unsubscribe linux-mm my@address'
in the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/

Re: [PATCH] kanoj-mm2.0-2.2.9 unneccesary page force in by munlock

[Linus Torvalds]

On Sun, 16 May 1999, Kanoj Sarcar wrote:
>
> While looking at the code for munlock() in mm/mlock.c, I found
> that munlock() unneccesarily executes a code path that forces
> page fault in over the entire range. The following patch fixes 
> this problem:

Well, it shouldn't force a page-fault, as the code is only executed if the
lockedness changes - and if it is a unlock then it will have been locked
before, so all the pages will have been present, and as such we wouldn't
actually need to fault them in.

I agree that it is certainly unnecessary, though, and pollutes TLB's etc
for no good reason.

How about this diff instead, avoiding the if-then-else setup?

		Linus

-----
--- v2.3.2/linux/mm/mlock.c	Fri Nov 20 11:43:19 1998
+++ linux/mm/mlock.c	Sun May 16 21:45:23 1999
@@ -115,10 +115,11 @@
 	if (!retval) {
 		/* keep track of amount of locked VM */
 		pages = (end - start) >> PAGE_SHIFT;
-		if (!(newflags & VM_LOCKED))
+		if (newflags & VM_LOCKED) {
 			pages = -pages;
-		vma->vm_mm->locked_vm += pages;
-		make_pages_present(start, end);
+			make_pages_present(start, end);
+		}
+		vma->vm_mm->locked_vm -= pages;
 	}
 	return retval;
 }


--
To unsubscribe, send a message with 'unsubscribe linux-mm my@address'
in the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/

Re: [PATCH] kanoj-mm2.0-2.2.9 unneccesary page force in by munlock

[Kanoj Sarcar]

> 
> 
> On Sun, 16 May 1999, Kanoj Sarcar wrote:
> >
> > While looking at the code for munlock() in mm/mlock.c, I found
> > that munlock() unneccesarily executes a code path that forces
> > page fault in over the entire range. The following patch fixes 
> > this problem:
> 
> Well, it shouldn't force a page-fault, as the code is only executed if the
> lockedness changes - and if it is a unlock then it will have been locked
> before, so all the pages will have been present, and as such we wouldn't
> actually need to fault them in.

Hmm, my logic was a little bit different. Note that you can call munlock()
on a range even when a previous mlock() has not been done on the range (I
think that's not an munlock error in POSIX). In 2.2.9, this would end up
faulting in the pages, which doesn't need to happen ... (haven't really
thought whether "root" can erroneously force memory deadlocks this way)

> 
> I agree that it is certainly unnecessary, though, and pollutes TLB's etc
> for no good reason.
> 
> How about this diff instead, avoiding the if-then-else setup?
> 
> 		Linus
> 
> -----
> --- v2.3.2/linux/mm/mlock.c	Fri Nov 20 11:43:19 1998
> +++ linux/mm/mlock.c	Sun May 16 21:45:23 1999
> @@ -115,10 +115,11 @@
>  	if (!retval) {
>  		/* keep track of amount of locked VM */
>  		pages = (end - start) >> PAGE_SHIFT;
> -		if (!(newflags & VM_LOCKED))
> +		if (newflags & VM_LOCKED) {
>  			pages = -pages;
> -		vma->vm_mm->locked_vm += pages;
> -		make_pages_present(start, end);
> +			make_pages_present(start, end);
> +		}
> +		vma->vm_mm->locked_vm -= pages;
>  	}
>  	return retval;
>  }
> 

I can't see any difference with my proposed fix, so if you are happy
with this one, I am too :-)  Note that in both our solutions, an "if"
check is happening, I am not sure if you are trying to remove the 
"else" code to reduce icache pollution. Also note that mlocks far 
outnumber munlocks (since apps depend on the final exit to release
all the locked memory), which in your solution translates to more
negate operations (pages = -pages).

Thanks.

Kanoj
kanoj@engr.sgi.com
--
To unsubscribe, send a message with 'unsubscribe linux-mm my@address'
in the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/

Re: [PATCH] kanoj-mm2.0-2.2.9 unneccesary page force in by munlock

[Linus Torvalds]

On Sun, 16 May 1999, Kanoj Sarcar wrote:
> 
> Hmm, my logic was a little bit different. Note that you can call munlock()
> on a range even when a previous mlock() has not been done on the range (I
> think that's not an munlock error in POSIX). In 2.2.9, this would end up
> faulting in the pages, which doesn't need to happen ... (haven't really
> thought whether "root" can erroneously force memory deadlocks this way)

Well, if you look closely, the mlock_fixup() routine tests whether
lockedness has changed and returns early if it hasn't.. So in your case
nothing at all would have been done..

		Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm my@address'
in the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/

Re: [PATCH] kanoj-mm2.0-2.2.9 unneccesary page force in by munlock

[Kanoj Sarcar]

> 
> 
> On Sun, 16 May 1999, Kanoj Sarcar wrote:
> > 
> > Hmm, my logic was a little bit different. Note that you can call munlock()
> > on a range even when a previous mlock() has not been done on the range (I
> > think that's not an munlock error in POSIX). In 2.2.9, this would end up
> > faulting in the pages, which doesn't need to happen ... (haven't really
> > thought whether "root" can erroneously force memory deadlocks this way)
> 
> Well, if you look closely, the mlock_fixup() routine tests whether
> lockedness has changed and returns early if it hasn't.. So in your case
> nothing at all would have been done..
> 
> 		Linus

Indeed, it does, my mistake ... it still makes sense to clean up the
code, as you mentioned originally ...

Thanks.

Kanoj
kanoj@engr.sgi.com
--
To unsubscribe, send a message with 'unsubscribe linux-mm my@address'
in the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/