Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1731958 > unrolled thread

[PATCH] mm: Restore mmput_async

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH] mm: Restore mmput_async Sherry Yang <sherryy@android.com> - 2017-09-14 00:00 +0200
    Re: [PATCH] mm: Restore mmput_async Andrew Morton <akpm@linux-foundation.org> - 2017-09-14 00:10 +0200
      Re: [PATCH] mm: Restore mmput_async Sherry Yang <sherryy@android.com> - 2017-09-14 00:50 +0200
        Re: [PATCH] mm: Restore mmput_async Andrew Morton <akpm@linux-foundation.org> - 2017-09-14 01:00 +0200
          Re: [PATCH] mm: Restore mmput_async Arve Hjønnevåg <arve@android.com> - 2017-09-14 02:10 +0200
    Re: [PATCH] mm: Restore mmput_async Michal Hocko <mhocko@kernel.org> - 2017-09-14 10:20 +0200
      [PATCH v2] android: binder: Drop lru lock in isolate callback Sherry Yang <sherryy@android.com> - 2017-09-14 20:30 +0200
        Re: [PATCH v2] android: binder: Drop lru lock in isolate callback Andrew Morton <akpm@linux-foundation.org> - 2017-09-14 22:20 +0200

#1731958 — [PATCH] 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 <sherryy@android.com>
---
 include/linux/sched/mm.h |  6 ++++++
 kernel/fork.c            | 18 ++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
index 3a19c253bdb1..ae53e413fb13 100644
--- a/include/linux/sched/mm.h
+++ b/include/linux/sched/mm.h
@@ -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);
diff --git a/kernel/fork.c b/kernel/fork.c
index 10646182440f..e702cb9ffbd8 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -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
  *
-- 
2.11.0 (Apple Git-81)

[toc] | [next] | [standalone]

#1731960

On Wed, 13 Sep 2017 17:59:27 -0400 Sherry Yang <sherryy@android.com> 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?

[toc] | [prev] | [next] | [standalone]

#1731968

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

On Wed, Sep 13, 2017 at 6:09 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Wed, 13 Sep 2017 17:59:27 -0400 Sherry Yang <sherryy@android.com> 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?

[toc] | [prev] | [next] | [standalone]

#1731970

On Wed, 13 Sep 2017 18:44:11 -0400 Sherry Yang <sherryy@android.com> wrote:

> On Wed, Sep 13, 2017 at 6:09 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Wed, 13 Sep 2017 17:59:27 -0400 Sherry Yang <sherryy@android.com> 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?
>
> 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?

[toc] | [prev] | [next] | [standalone]

#1731989

On Wed, Sep 13, 2017 at 3:57 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Wed, 13 Sep 2017 18:44:11 -0400 Sherry Yang <sherryy@android.com> wrote:
>
>> On Wed, Sep 13, 2017 at 6:09 PM, Andrew Morton
>> <akpm@linux-foundation.org> wrote:
>> > On Wed, 13 Sep 2017 17:59:27 -0400 Sherry Yang <sherryy@android.com> 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?
>>
>> 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?
>

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.

-- 
Arve Hjønnevåg

[toc] | [prev] | [next] | [standalone]

#1732110

On Wed 13-09-17 17:59:27, Sherry Yang 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")
> 
> Signed-off-by: Sherry Yang <sherryy@android.com>

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 <mhocko@suse.com>

> ---
>  include/linux/sched/mm.h |  6 ++++++
>  kernel/fork.c            | 18 ++++++++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
> index 3a19c253bdb1..ae53e413fb13 100644
> --- a/include/linux/sched/mm.h
> +++ b/include/linux/sched/mm.h
> @@ -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);
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 10646182440f..e702cb9ffbd8 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -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
>   *
> -- 
> 2.11.0 (Apple Git-81)
> 

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1732494 — [PATCH v2] 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.

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 <kyan@codeaurora.org>
Acked-by: Arve Hjønnevåg <arve@android.com>
Signed-off-by: Sherry Yang <sherryy@android.com>
---
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(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 8fe165844e47..064f5e31ec55 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -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);
diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
index 3a19c253bdb1..ae53e413fb13 100644
--- a/include/linux/sched/mm.h
+++ b/include/linux/sched/mm.h
@@ -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);
diff --git a/kernel/fork.c b/kernel/fork.c
index 10646182440f..e702cb9ffbd8 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -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
  *
-- 
2.11.0 (Apple Git-81)

[toc] | [prev] | [next] | [standalone]

#1732554 — Re: [PATCH v2] android: binder: Drop lru lock in isolate callback

On Thu, 14 Sep 2017 14:22:25 -0400 Sherry Yang <sherryy@android.com> 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.
> 
> 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 <akpm@linux-foundation.org>
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 <akpm@linux-foundation.org>
---

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

diff -puN include/linux/sched/mm.h~a include/linux/sched/mm.h
--- a/include/linux/sched/mm.h~a
+++ 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.
diff -puN kernel/fork.c~a kernel/fork.c
--- a/kernel/fork.c~a
+++ a/kernel/fork.c
@@ -386,6 +386,46 @@ void free_task(struct task_struct *tsk)
 }
 EXPORT_SYMBOL(free_task);
 
+/*
+ * Called when the last reference to the mm
+ * is dropped: either by a lazy thread or by
+ * mmput. Free the page directory and the mm.
+ */
+static void __mmdrop(struct mm_struct *mm)
+{
+	BUG_ON(mm == &init_mm);
+	mm_free_pgd(mm);
+	destroy_context(mm);
+	hmm_mm_destroy(mm);
+	mmu_notifier_mm_destroy(mm);
+	check_mm(mm);
+	put_user_ns(mm->user_ns);
+	free_mm(mm);
+}
+
+void mmdrop(struct mm_struct *mm)
+{
+	if (unlikely(atomic_dec_and_test(&mm->mm_count)))
+		__mmdrop(mm);
+}
+EXPORT_SYMBOL_GPL(mmdrop)
+
+static void mmdrop_async_fn(struct work_struct *work)
+{
+	struct mm_struct *mm;
+
+	mm = container_of(work, struct mm_struct, async_put_work);
+	__mmdrop(mm);
+}
+
+static 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);
+	}
+}
+
 static inline void free_signal_struct(struct signal_struct *sig)
 {
 	taskstats_tgid_free(sig);
@@ -895,24 +935,6 @@ struct mm_struct *mm_alloc(void)
 	return mm_init(mm, current, current_user_ns());
 }
 
-/*
- * Called when the last reference to the mm
- * is dropped: either by a lazy thread or by
- * mmput. Free the page directory and the mm.
- */
-void __mmdrop(struct mm_struct *mm)
-{
-	BUG_ON(mm == &init_mm);
-	mm_free_pgd(mm);
-	destroy_context(mm);
-	hmm_mm_destroy(mm);
-	mmu_notifier_mm_destroy(mm);
-	check_mm(mm);
-	put_user_ns(mm->user_ns);
-	free_mm(mm);
-}
-EXPORT_SYMBOL_GPL(__mmdrop);
-
 static inline void __mmput(struct mm_struct *mm)
 {
 	VM_BUG_ON(atomic_read(&mm->mm_users));
_

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web