LinuxLists.cc - [PATCH] android: binder: Drop lru lock in isolate callback

2017-09-08 22:39:43

Subject: [PATCH] android: binder: Drop lru lock in isolate callback

2017-09-13 21:59:47

Subject: [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 <[email protected]>
---
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)

2017-09-13 22:09:07

by Andrew Morton

[permalink] [raw]

Subject: Re: [PATCH] mm: Restore mmput_async

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?

2017-09-13 22:44:15

by Sherry Yang

[permalink] [raw]

Subject: 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.

On Wed, Sep 13, 2017 at 6:09 PM, Andrew Morton
<[email protected]> wrote:
> 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?

2017-09-13 22:57:59

by Andrew Morton

[permalink] [raw]

Subject: Re: [PATCH] mm: Restore mmput_async

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

> On Wed, Sep 13, 2017 at 6:09 PM, Andrew Morton
> <[email protected]> wrote:
> > 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?
>
> 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?

2017-09-14 00:08:50

by Arve Hjønnevåg

[permalink] [raw]

Subject: Re: [PATCH] mm: Restore mmput_async

On Wed, Sep 13, 2017 at 3:57 PM, Andrew Morton
<[email protected]> wrote:
> On Wed, 13 Sep 2017 18:44:11 -0400 Sherry Yang <[email protected]> wrote:
>
>> On Wed, Sep 13, 2017 at 6:09 PM, Andrew Morton
>> <[email protected]> wrote:
>> > 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?
>>
>> 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

2017-09-14 08:19:33

by Michal Hocko

[permalink] [raw]

Subject: Re: [PATCH] mm: Restore mmput_async

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 <[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]>

> ---
> 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

2017-09-14 18:23:08

by Sherry Yang

[permalink] [raw]

Subject: [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 <[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(-)

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)

2017-09-14 20:12:49

by Andrew Morton

[permalink] [raw]

Subject: Re: [PATCH v2] android: binder: Drop lru lock in isolate callback

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.
>
> 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(-)

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));
_