android: binder: Drop lru lock in isolate callback

PATCH Superseded Sherry Yang 4 participants 9 messages Sep 8, 2017 – Sep 14, 2017

Subsystems (4)

MM core Scheduler exec and binfmt Android drivers

lore

Share & tools

lore

Sherry Yang

author

2017-09-08 22:39 UTC #0 lore raw

PATCH 1/1 android: binder: Drop lru lock in isolate callback

Drop the global lru lock in isolate callback
before calling zap_page_range which calls
cond_resched, and re-acquire the global lru
lock before returning. Also change return
code to LRU_REMOVED_RETRY.

Use mmput_async when fail to acquire mmap sem
in an atomic context.

Fix "BUG: sleeping function called from invalid context" errors when CONFIG_DEBUG_ATOMIC_SLEEP is enabled.

Fixes f2517eb76f1f2 ("android: binder: Add global lru shrinker to binder")

Reported-by Kyle Yan <[email protected]>

Acked-by Arve Hjønnevåg <[email protected]>

Signed-off-by Sherry Yang <[email protected]>

---
 drivers/android/binder_alloc.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

drivers/android/binder_alloc.c +12 -6

index 8fe165844e47..064f5e31ec55 100644
@@ -913,6 +913,7 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 	struct binder_alloc *alloc;
 	uintptr_t page_addr;
 	size_t index;
+	struct vm_area_struct *vma;
 
 	alloc = page->alloc;
 	if (!mutex_trylock(&alloc->mutex))

@@ -923,16 +924,22 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 
 	index = page - alloc->pages;
 	page_addr = (uintptr_t)alloc->buffer + index * PAGE_SIZE;
-	if (alloc->vma) {
+	vma = alloc->vma;
+	if (vma) {
 		mm = get_task_mm(alloc->tsk);
 		if (!mm)
 			goto err_get_task_mm_failed;
 		if (!down_write_trylock(&mm->mmap_sem))
 			goto err_down_write_mmap_sem_failed;
+	}
+
+	list_lru_isolate(lru, item);
+	spin_unlock(lock);
 
+	if (vma) {
 		trace_binder_unmap_user_start(alloc, index);
 
-		zap_page_range(alloc->vma,
+		zap_page_range(vma,
 			       page_addr + alloc->user_buffer_offset,
 			       PAGE_SIZE);

@@ -950,13 +957,12 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 
 	trace_binder_unmap_kernel_end(alloc, index);
 
-	list_lru_isolate(lru, item);
-
+	spin_lock(lock);
 	mutex_unlock(&alloc->mutex);
-	return LRU_REMOVED;
+	return LRU_REMOVED_RETRY;
 
 err_down_write_mmap_sem_failed:
-	mmput(mm);
+	mmput_async(mm);
 err_get_task_mm_failed:
 err_page_already_freed:
 	mutex_unlock(&alloc->mutex);

Signature · 1 line

2.11.0 (Apple Git-81)

Sherry Yang

author

2017-09-13 21:59 UTC #1 lore raw

PATCH 1/1 mm: Restore mmput_async

Restore asynchronous mmput, allowing mmput_async to be called from an atomic context in Android binder shrinker callback.

mmput_async was initially introduced in ec8d7c14e ("mm, oom_reaper: do not mmput synchronously from the oom reaper context"), and was removed in 212925802 ("mm: oom: let oom_reap_task and exit_mmap run concurrently")

Signed-off-by Sherry Yang <[email protected]>

---
 include/linux/sched/mm.h |  6 ++++++
 kernel/fork.c            | 18 ++++++++++++++++++
 2 files changed, 24 insertions(+)

include/linux/sched/mm.h +6 -0

index 3a19c253bdb1..ae53e413fb13 100644
@@ -84,6 +84,12 @@ static inline bool mmget_not_zero(struct mm_struct *mm)
 
 /* mmput gets rid of the mappings and all user-space */
 extern void mmput(struct mm_struct *);
+#ifdef CONFIG_MMU
+/* same as above but performs the slow path from the async context. Can
+ * be called from the atomic context as well
+ */
+void mmput_async(struct mm_struct *);
+#endif
 
 /* Grab a reference to a task's mm, if it is not already going away */
 extern struct mm_struct *get_task_mm(struct task_struct *task);

kernel/fork.c +18 -0

index 10646182440f..e702cb9ffbd8 100644
@@ -946,6 +946,24 @@ void mmput(struct mm_struct *mm)
 }
 EXPORT_SYMBOL_GPL(mmput);
 
+#ifdef CONFIG_MMU
+static void mmput_async_fn(struct work_struct *work)
+{
+	struct mm_struct *mm = container_of(work, struct mm_struct,
+					    async_put_work);
+
+	__mmput(mm);
+}
+
+void mmput_async(struct mm_struct *mm)
+{
+	if (atomic_dec_and_test(&mm->mm_users)) {
+		INIT_WORK(&mm->async_put_work, mmput_async_fn);
+		schedule_work(&mm->async_put_work);
+	}
+}
+#endif
+
 /**
  * set_mm_exe_file - change a reference to the mm's executable file
  *

Signature · 1 line

2.11.0 (Apple Git-81)

Andrew Morton

maintainer

2017-09-13 22:09 UTC #2 lore raw

Re: [PATCH] mm: Restore mmput_async

Quoted 7 lines · On Wed, 13 Sep 2017 17:59:27 -0400 Sherry Yang <[email protected]> wrote:

Restore asynchronous mmput, allowing mmput_async to be called from an atomic context in Android binder shrinker callback.
mmput_async was initially introduced in ec8d7c14e ("mm, oom_reaper: do not mmput synchronously from the oom reaper context"), and was removed in 212925802 ("mm: oom: let oom_reap_task and exit_mmap run concurrently")

Presumably there's a patch somewhere which adds a call to mmput_async() into drivers/android/binder.c? Where is that patch?

Sherry Yang

author

2017-09-13 22:44 UTC #3 lore raw

Re: [PATCH] mm: Restore mmput_async

The patch that uses mmput_async() is
https://lkml.org/lkml/2017/9/8/785. Gmail doesn't seem to respect in-reply-to.

Quoted 12 lines · Andrew Morton

Quoted 7 lines · On Wed, 13 Sep 2017 17:59:27 -0400 Sherry Yang <[email protected]> wrote:
Restore asynchronous mmput, allowing mmput_async to be called from an atomic context in Android binder shrinker callback.
mmput_async was initially introduced in ec8d7c14e ("mm, oom_reaper: do not mmput synchronously from the oom reaper context"), and was removed in 212925802 ("mm: oom: let oom_reap_task and exit_mmap run concurrently")
Presumably there's a patch somewhere which adds a call to mmput_async() into drivers/android/binder.c? Where is that patch?

Andrew Morton

maintainer

2017-09-13 22:57 UTC #4 lore raw

Re: [PATCH] mm: Restore mmput_async

Quoted 12 lines · On Wed, 13 Sep 2017 18:44:11 -0400 Sherry Yang <[email protected]> wrote:

Quoted 10 lines · Andrew Morton
Quoted 7 lines · On Wed, 13 Sep 2017 17:59:27 -0400 Sherry Yang <[email protected]> wrote:
Restore asynchronous mmput, allowing mmput_async to be called from an atomic context in Android binder shrinker callback.
mmput_async was initially introduced in ec8d7c14e ("mm, oom_reaper: do not mmput synchronously from the oom reaper context"), and was removed in 212925802 ("mm: oom: let oom_reap_task and exit_mmap run concurrently")

Quoted 6 lines

Quoted 2 lines
Presumably there's a patch somewhere which adds a call to mmput_async() into drivers/android/binder.c? Where is that patch?
The patch that uses mmput_async() is
https://lkml.org/lkml/2017/9/8/785. Gmail doesn't seem to respect in-reply-to.

(Top-posting repaired. Please don't!)

Is it necessary for binder_alloc_free_page() to take a ref on the mm? As long as alloc->tsk doesn't exit during binder_alloc_free_page()'s execution, that task's reference on the mm should be sufficient to keep the mm alive?

Arve Hjønnevåg

maintainer

2017-09-14 00:08 UTC #5 lore raw

Re: [PATCH] mm: Restore mmput_async

Quoted 21 lines · Andrew Morton

Quoted 12 lines · On Wed, 13 Sep 2017 18:44:11 -0400 Sherry Yang <[email protected]> wrote:
Quoted 10 lines · Andrew Morton
Quoted 7 lines · On Wed, 13 Sep 2017 17:59:27 -0400 Sherry Yang <[email protected]> wrote:
Restore asynchronous mmput, allowing mmput_async to be called from an atomic context in Android binder shrinker callback.
mmput_async was initially introduced in ec8d7c14e ("mm, oom_reaper: do not mmput synchronously from the oom reaper context"), and was removed in 212925802 ("mm: oom: let oom_reap_task and exit_mmap run concurrently")
Quoted 6 lines
Quoted 2 lines
Presumably there's a patch somewhere which adds a call to mmput_async() into drivers/android/binder.c? Where is that patch?
The patch that uses mmput_async() is
https://lkml.org/lkml/2017/9/8/785. Gmail doesn't seem to respect in-reply-to.

Quoted 7 lines

(Top-posting repaired. Please don't!)
Is it necessary for binder_alloc_free_page() to take a ref on the mm? As long as alloc->tsk doesn't exit during binder_alloc_free_page()'s execution, that task's reference on the mm should be sufficient to keep the mm alive?

alloc->tsk can exit during binder_alloc_free_page. We don't hold a reference to the task's mm struct while we don't actively use it, as this would prevent the driver from getting closed when a process dies.

Signature · 1 line

Arve Hjønnevåg

Michal Hocko

Acked-by on v1 1/1

2017-09-14 08:19 UTC #6 lore raw

Re: [PATCH] mm: Restore mmput_async

In reply to Sherry Yang

Quoted 9 lines · Sherry Yang

Restore asynchronous mmput, allowing mmput_async to be called from an atomic context in Android binder shrinker callback.
mmput_async was initially introduced in ec8d7c14e ("mm, oom_reaper: do not mmput synchronously from the oom reaper context"), and was removed in 212925802 ("mm: oom: let oom_reap_task and exit_mmap run concurrently")
Signed-off-by Sherry Yang <[email protected]>

I would just fold this into https://lkml.org/lkml/2017/9/8/785. The patch is simple enough to take this in one go and it will be clear who and why needs to restore the api.

No objections otherwise.

Acked-by Michal Hocko <[email protected]>

Quoted 54 lines

---
 include/linux/sched/mm.h |  6 ++++++
 kernel/fork.c            | 18 ++++++++++++++++++
 2 files changed, 24 insertions(+)

include/linux/sched/mm.h +6 -0

index 3a19c253bdb1..ae53e413fb13 100644
@@ -84,6 +84,12 @@ static inline bool mmget_not_zero(struct mm_struct *mm)
 
 /* mmput gets rid of the mappings and all user-space */
 extern void mmput(struct mm_struct *);
+#ifdef CONFIG_MMU
+/* same as above but performs the slow path from the async context. Can
+ * be called from the atomic context as well
+ */
+void mmput_async(struct mm_struct *);
+#endif
 
 /* Grab a reference to a task's mm, if it is not already going away */
 extern struct mm_struct *get_task_mm(struct task_struct *task);

kernel/fork.c +18 -0

index 10646182440f..e702cb9ffbd8 100644
@@ -946,6 +946,24 @@ void mmput(struct mm_struct *mm)
 }
 EXPORT_SYMBOL_GPL(mmput);
 
+#ifdef CONFIG_MMU
+static void mmput_async_fn(struct work_struct *work)
+{
+	struct mm_struct *mm = container_of(work, struct mm_struct,
+					    async_put_work);
+
+	__mmput(mm);
+}
+
+void mmput_async(struct mm_struct *mm)
+{
+	if (atomic_dec_and_test(&mm->mm_users)) {
+		INIT_WORK(&mm->async_put_work, mmput_async_fn);
+		schedule_work(&mm->async_put_work);
+	}
+}
+#endif
+
 /**
  * set_mm_exe_file - change a reference to the mm's executable file
  *

Signature · 1 line

2.11.0 (Apple Git-81)

Signature · 2 lines

Michal Hocko
SUSE Labs

Sherry Yang

author

2017-09-14 18:23 UTC #7 lore raw

PATCH v2 1/1 android: binder: Drop lru lock in isolate callback

Use mmput_async when fail to acquire mmap sem
in an atomic context.

Fix "BUG: sleeping function called from invalid context" errors when CONFIG_DEBUG_ATOMIC_SLEEP is enabled.

Also restore mmput_async, which was initially introduced in ec8d7c14e ("mm, oom_reaper: do not mmput synchronously from the oom reaper context"), and was removed in 212925802 ("mm: oom: let oom_reap_task and exit_mmap run concurrently").

Fixes f2517eb76f1f2 ("android: binder: Add global lru shrinker to binder")

Reported-by Kyle Yan <[email protected]>

Acked-by Arve Hjønnevåg <[email protected]>

Signed-off-by Sherry Yang <[email protected]>

---
Changes in v2:
Combined '[PATCH] android: binder: Drop lru lock in isolate callback' with '[PATCH] mm: Restore mmput_async'.

 drivers/android/binder_alloc.c | 18 ++++++++++++------
 include/linux/sched/mm.h       |  6 ++++++
 kernel/fork.c                  | 18 ++++++++++++++++++
 3 files changed, 36 insertions(+), 6 deletions(-)

drivers/android/binder_alloc.c +12 -6

index 8fe165844e47..064f5e31ec55 100644
@@ -913,6 +913,7 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 	struct binder_alloc *alloc;
 	uintptr_t page_addr;
 	size_t index;
+	struct vm_area_struct *vma;
 
 	alloc = page->alloc;
 	if (!mutex_trylock(&alloc->mutex))

@@ -923,16 +924,22 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 
 	index = page - alloc->pages;
 	page_addr = (uintptr_t)alloc->buffer + index * PAGE_SIZE;
-	if (alloc->vma) {
+	vma = alloc->vma;
+	if (vma) {
 		mm = get_task_mm(alloc->tsk);
 		if (!mm)
 			goto err_get_task_mm_failed;
 		if (!down_write_trylock(&mm->mmap_sem))
 			goto err_down_write_mmap_sem_failed;
+	}
+
+	list_lru_isolate(lru, item);
+	spin_unlock(lock);
 
+	if (vma) {
 		trace_binder_unmap_user_start(alloc, index);
 
-		zap_page_range(alloc->vma,
+		zap_page_range(vma,
 			       page_addr + alloc->user_buffer_offset,
 			       PAGE_SIZE);

@@ -950,13 +957,12 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 
 	trace_binder_unmap_kernel_end(alloc, index);
 
-	list_lru_isolate(lru, item);
-
+	spin_lock(lock);
 	mutex_unlock(&alloc->mutex);
-	return LRU_REMOVED;
+	return LRU_REMOVED_RETRY;
 
 err_down_write_mmap_sem_failed:
-	mmput(mm);
+	mmput_async(mm);
 err_get_task_mm_failed:
 err_page_already_freed:
 	mutex_unlock(&alloc->mutex);

include/linux/sched/mm.h +6 -0

index 3a19c253bdb1..ae53e413fb13 100644
@@ -84,6 +84,12 @@ static inline bool mmget_not_zero(struct mm_struct *mm)
 
 /* mmput gets rid of the mappings and all user-space */
 extern void mmput(struct mm_struct *);
+#ifdef CONFIG_MMU
+/* same as above but performs the slow path from the async context. Can
+ * be called from the atomic context as well
+ */
+void mmput_async(struct mm_struct *);
+#endif
 
 /* Grab a reference to a task's mm, if it is not already going away */
 extern struct mm_struct *get_task_mm(struct task_struct *task);

kernel/fork.c +18 -0

index 10646182440f..e702cb9ffbd8 100644
@@ -946,6 +946,24 @@ void mmput(struct mm_struct *mm)
 }
 EXPORT_SYMBOL_GPL(mmput);
 
+#ifdef CONFIG_MMU
+static void mmput_async_fn(struct work_struct *work)
+{
+	struct mm_struct *mm = container_of(work, struct mm_struct,
+					    async_put_work);
+
+	__mmput(mm);
+}
+
+void mmput_async(struct mm_struct *mm)
+{
+	if (atomic_dec_and_test(&mm->mm_users)) {
+		INIT_WORK(&mm->async_put_work, mmput_async_fn);
+		schedule_work(&mm->async_put_work);
+	}
+}
+#endif
+
 /**
  * set_mm_exe_file - change a reference to the mm's executable file
  *

Signature · 1 line

2.11.0 (Apple Git-81)

Andrew Morton

maintainer

2017-09-14 20:12 UTC #8 lore raw

Quoted 9 lines · On Thu, 14 Sep 2017 14:22:25 -0400 Sherry Yang <[email protected]> wrote:

Drop the global lru lock in isolate callback
before calling zap_page_range which calls
cond_resched, and re-acquire the global lru
lock before returning. Also change return
code to LRU_REMOVED_RETRY.
Use mmput_async when fail to acquire mmap sem
in an atomic context.

Quoted 7 lines

Fix "BUG: sleeping function called from invalid context" errors when CONFIG_DEBUG_ATOMIC_SLEEP is enabled.
Also restore mmput_async, which was initially introduced in ec8d7c14e ("mm, oom_reaper: do not mmput synchronously from the oom reaper context"), and was removed in 212925802 ("mm: oom: let oom_reap_task and exit_mmap run concurrently").

Ho hum, OK. It's a bit sad to bring this back for one caller but mm.async_put_work is there and won't be going away.

The mmdrop_async stuff should be moved into fork.c (and maybe made static) as well. It's pointless having this stuff global and inlined, especially mmdrop_async_fn().

Something like the below, not yet tested...

From: Andrew Morton <[email protected]>
Subject: include/linux/sched/mm.h: uninline mmdrop_async(), etc

mmdrop_async() is only used in fork.c. Move that and its support functions into fork.c, uninline it all.

Signed-off-by Andrew Morton <[email protected]>

---

 include/linux/sched/mm.h |   24 +--------------
 kernel/fork.c            |   58 +++++++++++++++++++++++++------------
 2 files changed, 42 insertions(+), 40 deletions(-)

include/linux/sched/mm.h renamed from include/linux/sched/mm.h~a +2 ~~-22~~

diff -puN include/linux/sched/mm.h~a include/linux/sched/mm.h
@@ -10,7 +10,7 @@
 /*
  * Routines for handling mm_structs
  */
-extern struct mm_struct * mm_alloc(void);
+extern struct mm_struct *mm_alloc(void);
 
 /**
  * mmgrab() - Pin a &struct mm_struct.

@@ -34,27 +34,7 @@ static inline void mmgrab(struct mm_stru
 	atomic_inc(&mm->mm_count);
 }
 
-/* mmdrop drops the mm and the page tables */
-extern void __mmdrop(struct mm_struct *);
-static inline void mmdrop(struct mm_struct *mm)
-{
-	if (unlikely(atomic_dec_and_test(&mm->mm_count)))
-		__mmdrop(mm);
-}
-
-static inline void mmdrop_async_fn(struct work_struct *work)
-{
-	struct mm_struct *mm = container_of(work, struct mm_struct, async_put_work);
-	__mmdrop(mm);
-}
-
-static inline void mmdrop_async(struct mm_struct *mm)
-{
-	if (unlikely(atomic_dec_and_test(&mm->mm_count))) {
-		INIT_WORK(&mm->async_put_work, mmdrop_async_fn);
-		schedule_work(&mm->async_put_work);
-	}
-}
+extern void mmdrop(struct mm_struct *mm);
 
 /**
  * mmget() - Pin the address space associated with a &struct mm_struct.

kernel/fork.c renamed from kernel/fork.c~a +40 ~~-18~~

70 lines not on this page · raw message