FYI: mmap_sem OOM patch

As I mentioned this in my MM discussion proposal, I am sending the following patch for discussion.

This helps some workloads we use, that are normally run with little memory headroom and get occasionally pushed over the edge to OOM. In some cases, the OOMing thread can't exit because it needs to acquire the mmap_sem, while the thread holding mmap_sem can't progress because it needs to allocate memory.

IIRC we first encountered this against our 2.6.18 kernel; this has been reproduced with 2.6.33 (sorry, we'll have to clean up that test case a bit before I can send it); and following patch is against 2.6.34 plus down_read_unfair changes (which have been discussed here before, not resending them at this point unless someone asks).

Note that while this patch modifies some performance critical MM functions, it does make sure to stay out of the fast paths.

Use down_read_unfair() in OOM-killed tasks

When tasks are being targeted by OOM killer, we want to use down_read_unfair to ensure a fast exit.

- The down_read_unfair() call in fast_get_user_pages() is to cover the futex calls in mm_release() (including exit_robust_list(), compat_exit_robust_list(), exit_pi_state_list() and sys_futex())

- The down_read_unfair() call in do_page_fault() is to cover the put_user() call in mm_release() and exiting the robust futex lists.

This is a rework of a previous change in 2.6.18:

Change the down_read()s in the exit path to be unfair so that we do not result in a potentially 3-to-4-way deadlock during the ooms.

What happens is we end up with a single thread in the oom loop (T1) that ends up killing a sibling thread (T2). That sibling thread will need to acquire the read side of the mmap_sem in the exit path. It's possible however that yet a different thread (T3) is in the middle of a virtual address space operation (mmap, munmap) and is enqueue to grab the write side of the mmap_sem behind yet another thread (T4) that is stuck in the OOM loop (behind T1) with mmap_sem held for read (like allocating a page for pagecache as part of a fault.

      T1              T2              T3              T4
      .               .               .               .
   oom:               .               .               .
   oomkill            .               .               .
      ^    \          .               .               .
     /|\    ---->  do_exit:           .               .
      |            sleep in           .               .
      |            read(mmap_sem)     .               .
      |                     \         .               .
      |                      ----> mmap               .
      |                            sleep in           .
      |                            write(mmap_sem)    .
      |                                     \         .
      |                                      ----> fault
      |                                            holding read(mmap_sem)
      |                                            oom
      |                                               |
      |                                               /
      \----------------------------------------------/

Signed-off-by Michel Lespinasse <[email protected]>

arch/x86/mm/fault.c +4 -1

index f627779..4b3a1c7 100644
@@ -1062,7 +1062,10 @@ do_page_fault(struct pt_regs *regs, unsigned long error_code)
 			bad_area_nosemaphore(regs, error_code, address);
 			return;
 		}
-		down_read(&mm->mmap_sem);
+		if (test_thread_flag(TIF_MEMDIE))
+			down_read_unfair(&mm->mmap_sem);
+		else
+			down_read(&mm->mmap_sem);
 	} else {
 		/*
 		 * The above down_read_trylock() might have succeeded in

arch/x86/mm/gup.c +4 -1

index 738e659..578d1a7 100644
@@ -357,7 +357,10 @@ slow_irqon:
 		start += nr << PAGE_SHIFT;
 		pages += nr;
 
-		down_read(&mm->mmap_sem);
+		if (unlikely(test_thread_flag(TIF_MEMDIE)))
+			down_read_unfair(&mm->mmap_sem);
+		else
+			down_read(&mm->mmap_sem);
 		ret = get_user_pages(current, mm, start,
 			(end - start) >> PAGE_SHIFT, write, 0, pages, NULL);
 		up_read(&mm->mmap_sem);

kernel/exit.c +0 -0

index 7f2683a..2318d3a 100644

Yup.
If admins want to kill memory hogging process manually when the system is under heavy swap thrashing, we will face the same problem, need unfairness and fast exit. So, unfair exiting design looks very good.

If you will updated the description, I'm glad :)

arch/x86/mm/fault.c +0 -0

index f627779..4b3a1c7 100644
@@ -1062,7 +1062,10 @@ do_page_fault(struct pt_regs *regs, unsigned long error_code)
 			bad_area_nosemaphore(regs, error_code, address);

Quoted 11 lines

arch/x86/mm/fault.c +1 -1

index f627779..4b3a1c7 100644
@@ -1062,7 +1062,10 @@ do_page_fault(struct pt_regs *regs, unsigned long error_code)
 			bad_area_nosemaphore(regs, error_code, address);
 			return;
 		}
-		down_read(&mm->mmap_sem);
+		if (test_thread_flag(TIF_MEMDIE))

arch/x86/mm/fault.c do_page_fault(struct pt_regs *regs, unsigned long error_code) · line 1071

Quoted 6 lines

arch/x86/mm/fault.c +3 -0

+			down_read_unfair(&mm->mmap_sem);
+		else
+			down_read(&mm->mmap_sem);
 	} else {
 		/*
 		 * The above down_read_trylock() might have succeeded in

I still think adding that _unfair interface is asking for trouble.

Quoted 12 lines · Michel Lespinasse

Quoted 11 lines

arch/x86/mm/fault.c +1 -1

index f627779..4b3a1c7 100644
@@ -1062,7 +1062,10 @@ do_page_fault(struct pt_regs *regs, unsigned long error_code)
 			bad_area_nosemaphore(regs, error_code, address);
 			return;
 		}
-		down_read(&mm->mmap_sem);
+		if (test_thread_flag(TIF_MEMDIE))

Quoted 8 lines

arch/x86/mm/fault.c do_page_fault(struct pt_regs *regs, unsigned long error_code) · line 1071

Quoted 6 lines

arch/x86/mm/fault.c +3 -0

+			down_read_unfair(&mm->mmap_sem);
+		else
+			down_read(&mm->mmap_sem);
 	} else {
 		/*
 		 * The above down_read_trylock() might have succeeded in

I still think adding that _unfair interface is asking for trouble.

Can you please explain trouble that you worry? Why do we need to keep thread fairness when OOM case?

Quoted 21 lines

Quoted 20 lines · Michel Lespinasse

Quoted 7 lines

arch/x86/mm/fault.c +0 -0

index f627779..4b3a1c7 100644
@@ -1062,7 +1062,10 @@ do_page_fault(struct pt_regs *regs, unsigned long error_code)
 			bad_area_nosemaphore(regs, error_code, address);

arch/x86/mm/fault.c do_page_fault(struct pt_regs *regs, unsigned long error_code) · line 1071

Quoted 10 lines

arch/x86/mm/fault.c +4 -1

 			return;
 		}
-		down_read(&mm->mmap_sem);
+		if (test_thread_flag(TIF_MEMDIE))
+			down_read_unfair(&mm->mmap_sem);
+		else
+			down_read(&mm->mmap_sem);
 	} else {
 		/*
 		 * The above down_read_trylock() might have succeeded in

I still think adding that _unfair interface is asking for trouble.

Quoted 2 lines

Can you please explain trouble that you worry? Why do we need to keep thread fairness when OOM case?

Just the whole concept of the unfair thing offends me ;-) I didn't really look at the particular application in this case.

What happens is we end up with a single thread in the oom loop (T1) that ends up killing a sibling thread (T2). That sibling thread will need to acquire the read side of the mmap_sem in the exit path. It's possible however that yet a different thread (T3) is in the middle of a virtual address space operation (mmap, munmap) and is enqueue to grab the write side of the mmap_sem behind yet another thread (T4) that is stuck in the OOM loop (behind T1) with mmap_sem held for read (like allocating a page for pagecache as part of a fault.

      T1              T2              T3              T4
      .               .               .               .
   oom:               .               .               .
   oomkill            .               .               .
      ^    \          .               .               .
     /|\    ---->  do_exit:           .               .
      |            sleep in           .               .
      |            read(mmap_sem)     .               .
      |                     \         .               .
      |                      ----> mmap               .
      |                            sleep in           .

      |                            write(mmap_sem)    .
      |                                     \         .
      |                                      ----> fault
      |                                            holding read(mmap_sem)
      |                                            oom
      |                                               |
      |                                               /
      \----------------------------------------------/ 

One way to fix this is to have T4 wake from the oom queue and return an allocation failure instead of insisting on going oom itself when T1 decides to take down the task.

How would you have T4 figure out the deadlock situation ? T1 is taking down T2, not T4...

Quoted 7 lines

One way to fix this is to have T4 wake from the oom queue and return an allocation failure instead of insisting on going oom itself when T1 decides to take down the task.

How would you have T4 figure out the deadlock situation ? T1 is taking down T2, not T4...

If T2 and T4 share a mmap_sem they belong to the same process. OOM takes down the whole process by sending around signals of sorts (SIGKILL?), so if T4 gets a fatal signal while it is waiting to enter the oom thingy, have it abort and return an allocation failure.

That alloc failure (along with a pending fatal signal) will very likely lead to the release of its mmap_sem (if not, there's more things to cure).

At which point the cycle is broken an stuff continues as it was intended.

Quoted 14 lines · Michel Lespinasse

Quoted 7 lines

One way to fix this is to have T4 wake from the oom queue and return an allocation failure instead of insisting on going oom itself when T1 decides to take down the task.

How would you have T4 figure out the deadlock situation ? T1 is taking down T2, not T4...

If T2 and T4 share a mmap_sem they belong to the same process. OOM takes down the whole process by sending around signals of sorts (SIGKILL?), so if T4 gets a fatal signal while it is waiting to enter the oom thingy, have it abort and return an allocation failure.

Quoted 6 lines

That alloc failure (along with a pending fatal signal) will very likely lead to the release of its mmap_sem (if not, there's more things to cure).

At which point the cycle is broken an stuff continues as it was intended.

Now, I've reread current code. I think mmotm already have this.

Thought?

Quoted 21 lines

Quoted 20 lines · Michel Lespinasse

Quoted 7 lines

One way to fix this is to have T4 wake from the oom queue and return an allocation failure instead of insisting on going oom itself when T1 decides to take down the task.

How would you have T4 figure out the deadlock situation ? T1 is taking down T2, not T4...

If T2 and T4 share a mmap_sem they belong to the same process. OOM takes down the whole process by sending around signals of sorts (SIGKILL?), so if T4 gets a fatal signal while it is waiting to enter the oom thingy, have it abort and return an allocation failure.

That alloc failure (along with a pending fatal signal) will very likely lead to the release of its mmap_sem (if not, there's more things to cure).

At which point the cycle is broken an stuff continues as it was intended.

Quoted 1 line

Now, I've reread current code. I think mmotm already have this.

<snip code>

[ small note on that we really should kill __GFP_NOFAIL, its utter deadlock potential ]

Quoted 1 line

Thought?

So either its not working or google never tried that code?

Quoted 2 lines

[ small note on that we really should kill __GFP_NOFAIL, its utter deadlock potential ]

I disagree. __GFP_NOFAIL mean this allocation failure can makes really dangerous result. Instead, OOM-Killer should try to kill next process. I think.

Quoted 6 lines

Quoted 2 lines

[ small note on that we really should kill __GFP_NOFAIL, its utter deadlock potential ]

I disagree. __GFP_NOFAIL mean this allocation failure can makes really dangerous result. Instead, OOM-Killer should try to kill next process. I think.

Say _what_?! you think NOFAIL is a sane thing?

Pretty much everybody has
been agreeing for years that the thing should die.

I disagree. __GFP_NOFAIL mean this allocation failure can makes really dangerous result. Instead, OOM-Killer should try to kill next process. I think.

Quoted 5 lines

I disagree. __GFP_NOFAIL mean this allocation failure can makes really dangerous result. Instead, OOM-Killer should try to kill next process. I think.

That's not what happens, __alloc_pages_high_priority() will loop forever for __GFP_NOFAIL, the oom killer is never recalled.

Quoted 7 lines

Quoted 4 lines

I disagree. __GFP_NOFAIL mean this allocation failure can makes really dangerous result. Instead, OOM-Killer should try to kill next process. I think.

That's not what happens, __alloc_pages_high_priority() will loop forever for __GFP_NOFAIL, the oom killer is never recalled.

Yup, please reread the discusstion.