running get_user_pages() from kernel thread

So the question is: is this thing is by desgin? (that kernel thread cant call get_user_pages???), should i use something like switch_mm()??

Quoted 2 lines · Izik Eidus

So the question is: is this thing is by desgin? (that kernel thread cant call get_user_pages???), should i use something like switch_mm()??

I think switch_mm trick should be used for page faults, but gup shouldn't require it because it gets the 'mm' as parameter and the current->mm has to be irrelevant. current->mm is only relevant for gup-fast (obviously :). So I think the only bit that needs fixing is grab_swap_token to not run if current->mm is null.

Quoted 7 lines · Andrea Arcangeli

Quoted 1 line · Izik Eidus

So the question is: is this thing is by desgin? (that kernel thread cant call get_user_pages???), should i use something like switch_mm()??

I think switch_mm trick should be used for page faults, but gup shouldn't require it because it gets the 'mm' as parameter and the current->mm has to be irrelevant. current->mm is only relevant for gup-fast (obviously :). So I think the only bit that needs fixing is grab_swap_token to not run if current->mm is null.

Looks like Izik and I hit the same problem (otherwise running well):
I too decided we needn't do more than avoid the issue in grab_swap_token. (I've a feeling someone has hit this issue before with some other thread, though I've no idea which - does RHEL include a patch like this perhaps?).

Signed-off-by Hugh Dickins <[email protected]>

Quoted 10 lines · Andrea Arcangeli

Quoted 2 lines · Izik Eidus

So the question is: is this thing is by desgin? (that kernel thread cant call get_user_pages???), should i use something like switch_mm()??

I think switch_mm trick should be used for page faults, but gup shouldn't require it because it gets the 'mm' as parameter and the current->mm has to be irrelevant. current->mm is only relevant for gup-fast (obviously :). So I think the only bit that needs fixing is grab_swap_token to not run if current->mm is null.

Looks like Izik and I hit the same problem (otherwise running well):
I too decided we needn't do more than avoid the issue in grab_swap_token. (I've a feeling someone has hit this issue before with some other thread, though I've no idea which - does RHEL include a patch like this perhaps?).

[PATCH] mm: grab_swap_token back out if no mm

If a kthread happens to use get_user_pages() on an mm (as KSM does), there's a chance that it will end up trying to read in a swap page, and oops in grab_swap_token() because the kthread has no mm:
nothing clever, just avoid that case.

Signed-off-by Hugh Dickins <[email protected]>

---

 mm/thrash.c |    3 +++
 1 file changed, 3 insertions(+)

Quoted 5 lines · Hugh Dickins

Looks like Izik and I hit the same problem (otherwise running well):
I too decided we needn't do more than avoid the issue in grab_swap_token. (I've a feeling someone has hit this issue before with some other thread, though I've no idea which - does RHEL include a patch like this perhaps?).

It looks very familiar, indeed.

RHEL 4 and 5 both have code like this in the swap token code. I seem to remember submitting such code upstream, too.

I have no idea why it never got upstream, maybe I was drowned in Xen work at the time, or maybe the bug simply didn't happen upstream for whatever reason.

Quoted 2 lines

Signed-off-by Hugh Dickins <[email protected]>

Acked-by Rik van Riel <[email protected]>

Quoted 9 lines · Andrea Arcangeli

Quoted 2 lines · Izik Eidus

So the question is: is this thing is by desgin? (that kernel thread cant call get_user_pages???), should i use something like switch_mm()??

I think switch_mm trick should be used for page faults, but gup shouldn't require it because it gets the 'mm' as parameter and the current->mm has to be irrelevant. current->mm is only relevant for gup-fast (obviously :). So I think the only bit that needs fixing is grab_swap_token to not run if current->mm is null.

Looks like Izik and I hit the same problem (otherwise running well):
I too decided we needn't do more than avoid the issue in grab_swap_token. (I've a feeling someone has hit this issue before with some other thread, though I've no idea which - does RHEL include a patch like this perhaps?).

[PATCH] mm: grab_swap_token back out if no mm

If a kthread happens to use get_user_pages() on an mm (as KSM does), there's a chance that it will end up trying to read in a swap page, and oops in grab_swap_token() because the kthread has no mm:
nothing clever, just avoid that case.

Signed-off-by Hugh Dickins <[email protected]>

---

 mm/thrash.c |    3 +++
 1 file changed, 3 insertions(+)

liking the below better - what do you think? If you approve, I'll send it in to Andrew for mmotm: I can't quite justify it without KSM.

Quoted 2 lines · Hugh Dickins

Looks like Izik and I hit the same problem (otherwise running well):

Good, This solve another issue that you probably dont hit beacuse you work with the madvise version:
the .release call back of the file_operations strcture will call to:
ksm_sma_release() that will call to remove_slot_from_hash_and_tree() that will do the silly break_cow() call (even that the prcoesses just die!!!)

Now beacuse that exit_mm() will set tsk->mm = NULL before the .release will get called, we will trigger this path even without the kernel thread context. (I prepred patch that just avoid the break_cow() from the .release context, but it isnt needed with this patch)

(You shouldnt have that specific problem in the madvise() version beacuse we dont have this .release callback anymore, but we still do there useless break_cow() calls, considering the fact that the process just die, this break_cow() calls should be made only when the user ask specifically to stop merging pages i guess...,

I will send patch that will make it work more logical on top of your patches as soon as you send something)

Did you have a particular reason not to pass in the faulting mm instead?

As the swap token should only care about the faulting address space leading to swap io and not about the running process anyway, we could do it like below and remove all those pesky current->derefs in the same go. What do you think?

Quoted 2 lines · Hugh Dickins

liking the below better - what do you think? If you approve, I'll send it in to Andrew for mmotm: I can't quite justify it without KSM.

I think it's definitely better to fix gup to be safe from kernel thread even without KSM so this can go in mainline well before KSM. Unless gup requires a swapin fault it won't ever trigger.

Quoted 6 lines · Hugh Dickins

mm/thrash.c +0 -0

@@ -30,6 +30,9 @@ void grab_swap_token(void)
 {
 	int current_interval;
 

mm/thrash.c void grab_swap_token(void) · line 35

Quoted 3 lines

mm/thrash.c +3 -0

+	if (!current->mm)	/* kthread doing get_user_pages on an mm */
+		return;
+

Did you have a particular reason not to pass in the faulting mm instead?

[PATCH] mm: pass mm to grab_swap_token

If a kthread happens to use get_user_pages() on an mm (as KSM does), there's a chance that it will end up trying to read in a swap page, then oops in grab_swap_token() because the kthread has no mm: GUP passes down the right mm, so grab_swap_token() ought to be using it.

Signed-off-by Hugh Dickins <[email protected]>

Quoted 2 lines · Izik Eidus

Hugh Dickins wrote:

Quoted 2 lines

Quoted 2 lines

Looks like Izik and I hit the same problem (otherwise running well):

Ran well enough overnight (with mm/mmap.c vm_flags hack to be merging everything it could) that, to judge by your remarks below, I ought to switch away from investigating and fixing and reviewing, to sending you patches to bring us back in synch.

I don't think I have any problems with the madvise route, which weren't already problems with the /dev/ksm route: there are oddities I'm eager to look into (fork still raising questions for me), but nothing serious enough to get in the way of resynching.

Andrea will be relieved to learn that I like mm_slot->touched very much:
I don't know (and now don't need to know) what was serving that purpose in the /dev/ksm version.

Quoted 6 lines

Good, This solve another issue that you probably dont hit beacuse you work with the madvise version:
the .release call back of the file_operations strcture will call to:
ksm_sma_release() that will call to remove_slot_from_hash_and_tree() that will do the silly break_cow() call (even that the prcoesses just die!!!)

Yes, I'd noticed before that break_cow() can be silly more work than necessary, may need care in ordering things. But, if it's still any issue, it's something that can be optimized later: you have a technique that goes about it safely, that's good.

Quoted 11 lines

Now beacuse that exit_mm() will set tsk->mm = NULL before the .release will get called, we will trigger this path even without the kernel thread context. (I prepred patch that just avoid the break_cow() from the .release context, but it isnt needed with this patch)

(You shouldnt have that specific problem in the madvise() version beacuse we dont have this .release callback anymore, but we still do there useless break_cow() calls, considering the fact that the process just die, this break_cow() calls should be made only when the user ask specifically to stop merging pages i guess...,

Yes, and I'm thinking that although it's fine for madvise(,,MADV_MERGEABLE) to be slow to get around to merging, probably madvise(,,MADV_UNMERGEABLE) needs to have broken COW on any KSM pages before the call returns. I've a suspicion that nobody will ever use MADV_UNMERGEABLE, outside of our testing; and yet it's against my principles not to provide it, and if it is used, then I think it needs to give that guarantee before returning. But again, something we can fill in once we're in synch.

Quoted 3 lines

I will send patch that will make it work more logical on top of your patches as soon as you send something)

Right, what would you like me to base against? What if you were to send me a rollup patch against 2.6.30 of what your tree has now? Or would you prefer to base against an mmotm? With or without your RFC patches, or something close to them?

Once I have your base, whichever way you prefer it, then I can put together a series of patches to adjust that to what I'm now working with (mainly: the ksm.c end of it would be much as in your RFC though with tidyups, and minus 4/4; whereas the madvise.c end of it I reworked).

The patches we send to Andrew for mmotm, later on, would be something different, I believe (and something different from your git history):
there I think we'd be asking him to remove the KSM patches he already has, and providing fresh equivalents based around the madvise interface (so, for example, I think there would be no patch at all to mm/rmap.c, all the changes made there earlier being later reverted).

Hugh

Make use of the compiler's typechecking on !CONFIG_SWAP as well.

Signed-off-by Johannes Weiner <[email protected]>

From: Hugh Dickins <[email protected]>

grab_swap_token() should not make any assumptions about the running process as the swap token is an attribute of the address space and the faulting mm is not necessarily current->mm.

This fixes get_user_pages() from kernel threads which would blow up when encountering a swapped out page and grab_swap_token() dereferencing the unset for kernel threads current->mm.

Signed-off-by Hugh Dickins <[email protected]>

Signed-off-by Johannes Weiner <[email protected]>

Quoted 3 lines · Johannes Weiner

Did you have a particular reason not to pass in the faulting mm instead?

No good reason.

Quoted 4 lines

As the swap token should only care about the faulting address space leading to swap io and not about the running process anyway, we could do it like below and remove all those pesky current->derefs in the same go. What do you think?

I think we'll have to claim joint authorship.

But I win by one checkpatch point, and a few minutes ;)

From: Hugh Dickins <[email protected]>

grab_swap_token() should not make any assumptions about the running process as the swap token is an attribute of the address space and the faulting mm is not necessarily current->mm.

This fixes get_user_pages() from kernel threads which would blow up when encountering a swapped out page and grab_swap_token() dereferencing the unset for kernel threads current->mm.

Signed-off-by Hugh Dickins <[email protected]>

Signed-off-by Johannes Weiner <[email protected]>

Hi, Hannes.

How about adding Hugh's comment ?

If a kthread happens to use get_user_pages() on an mm (as KSM does), there's a chance that it will end up trying to read in a swap page, then oops in grab_swap_token() because the kthread has no mm: GUP passes down the right mm, so grab_swap_token() ought to be using it.

We have not identified a stronger case than KSM's daemon (not yet in mainline), but the issue must have come up before, since RHEL has included a fix for this for years (though a different fix, they just back out of grab_swap_token if current->mm is unset: which is what we first proposed, but using the right mm here seems more correct).

Reported-by Izik Eidus <[email protected]>

Signed-off-by Johannes Weiner <[email protected]>

Signed-off-by Hugh Dickins <[email protected]>

Acked-by Rik van Riel <[email protected]>