Skip to contentSkip to conversation

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

PATCH In review Kanoj Sarcar 2 participants 5 messages May 17, 1999

Subsystems (3)
Memory management Kbuild Memory mapping
lore
Share & tools
lore

Kanoj Sarcar

author
#0 lore raw

PATCH 1/1 kanoj-mm2.0-2.2.9 unneccesary page force in by munlock

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:

mlock.c Sun May 16 20:47:01 1999 renamed from usr/tmp/p_rdiff_a007Qs/mlock.c Sun May 16 20:48:12 1999 +2 -1
@@ -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
[email protected]
--
To unsubscribe, send a message with 'unsubscribe linux-mm my@address' in the body to [email protected]. For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/

#1 lore raw
Quoted 5 lines · 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:

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

-----

mm/mlock.c renamed from linux/mm/mlock.c +4 -3
@@ -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 [email protected]. For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/

author
#2 lore raw
Quoted 9 lines · Kanoj Sarcar
Quoted 5 lines

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:

Quoted 4 lines

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)

Quoted 17 lines

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

-----

mm/mlock.c renamed from linux/mm/mlock.c +0 -1
@@ -115,10 +115,11 @@
 	if (!retval) {
 		/* keep track of amount of locked VM */
 		pages = (end - start) >> PAGE_SHIFT;
-		if (!(newflags & VM_LOCKED))

mm/mlock.c line 125

Quoted 10 lines
mm/mlock.c renamed from linux/mm/mlock.c +4 -2
+		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
[email protected]
--
To unsubscribe, send a message with 'unsubscribe linux-mm my@address' in the body to [email protected]. For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/

#3 lore raw
Quoted 6 lines · Kanoj Sarcar

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 [email protected]. For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/

author
#4 lore raw
Quoted 10 lines · Kanoj Sarcar
Quoted 6 lines

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)

Quoted 5 lines

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
[email protected]
--
To unsubscribe, send a message with 'unsubscribe linux-mm my@address' in the body to [email protected]. For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/