Re: [PATCH] android: binder: Disable preemption while holding the global binder lock

From: Christoph Hellwig
Date: Sat Sep 10 2016 - 12:17:08 EST

Next message: Peter Zijlstra: "Re: [PATCH] android: binder: Disable preemption while holding the global binder lock"
Previous message: Jonathan Cameron: "Re: [PATCH 2/2] iio: 104-quad-8: Add IIO support for the ACCES 104-QUAD-8"
In reply to: Greg Kroah-Hartman: "Re: [PATCH] android: binder: Disable preemption while holding the global binder lock"
Next in thread: Peter Zijlstra: "Re: [PATCH] android: binder: Disable preemption while holding the global binder lock"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Sep 08, 2016 at 09:12:50AM -0700, Todd Kjos wrote:
> In Android systems, the display pipeline relies on low
> latency binder transactions and is therefore sensitive to
> delays caused by contention for the global binder lock.
> Jank is siginificantly reduced by disabling preemption
> while the global binder lock is held.

That's now how preempt_disable is supposed to use. It is for critical
sections that use per-cpu or similar resources.

>
> Originally-from: Riley Andrews <riandrews@xxxxxxxxxx>
> Signed-off-by: Todd Kjos <tkjos@xxxxxxxxxx>
> ---
> drivers/android/binder.c | 194 +++++++++++++++++++++++++++++++++++------------
> 1 file changed, 146 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 16288e7..c36e420 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -379,6 +379,7 @@ static int task_get_unused_fd_flags(struct
> binder_proc *proc, int flags)
> struct files_struct *files = proc->files;
> unsigned long rlim_cur;
> unsigned long irqs;
> + int ret;
>
> if (files == NULL)
> return -ESRCH;
> @@ -389,7 +390,11 @@ static int task_get_unused_fd_flags(struct
> binder_proc *proc, int flags)
> rlim_cur = task_rlimit(proc->tsk, RLIMIT_NOFILE);
> unlock_task_sighand(proc->tsk, &irqs);
>
> - return __alloc_fd(files, 0, rlim_cur, flags);
> + preempt_enable_no_resched();
> + ret = __alloc_fd(files, 0, rlim_cur, flags);
> + preempt_disable();
> +
> + return ret;
> }
>
> /*
> @@ -398,8 +403,11 @@ static int task_get_unused_fd_flags(struct
> binder_proc *proc, int flags)
> static void task_fd_install(
> struct binder_proc *proc, unsigned int fd, struct file *file)
> {
> - if (proc->files)
> + if (proc->files) {
> + preempt_enable_no_resched();
> __fd_install(proc->files, fd, file);
> + preempt_disable();
> + }
> }
>
> /*
> @@ -427,6 +435,7 @@ static inline void binder_lock(const char *tag)
> {
> trace_binder_lock(tag);
> mutex_lock(&binder_main_lock);
> + preempt_disable();
> trace_binder_locked(tag);
> }
>
> @@ -434,8 +443,65 @@ static inline void binder_unlock(const char *tag)
> {
> trace_binder_unlock(tag);
> mutex_unlock(&binder_main_lock);
> + preempt_enable();
> +}
> +
> +static inline void *kzalloc_nopreempt(size_t size)
> +{
> + void *ptr;
> +
> + ptr = kzalloc(size, GFP_NOWAIT);
> + if (ptr)
> + return ptr;
> +
> + preempt_enable_no_resched();
> + ptr = kzalloc(size, GFP_KERNEL);
> + preempt_disable();
> +
> + return ptr;
> +}
> +
> +static inline long copy_to_user_nopreempt(void __user *to,
> + const void *from, long n)
> +{
> + long ret;
> +
> + preempt_enable_no_resched();
> + ret = copy_to_user(to, from, n);
> + preempt_disable();
> + return ret;
> +}
> +
> +static inline long copy_from_user_nopreempt(void *to,
> + const void __user *from,
> + long n)
> +{
> + long ret;
> +
> + preempt_enable_no_resched();
> + ret = copy_from_user(to, from, n);
> + preempt_disable();
> + return ret;
> }
>
> +#define get_user_nopreempt(x, ptr) \
> +({ \
> + int __ret; \
> + preempt_enable_no_resched(); \
> + __ret = get_user(x, ptr); \
> + preempt_disable(); \
> + __ret; \
> +})
> +
> +#define put_user_nopreempt(x, ptr) \
> +({ \
> + int __ret; \
> + preempt_enable_no_resched(); \
> + __ret = put_user(x, ptr); \
> + preempt_disable(); \
> + __ret; \
> +})
> +
> static void binder_set_nice(long nice)
> {
> long min_nice;
> @@ -568,6 +634,8 @@ static int binder_update_page_range(struct
> binder_proc *proc, int allocate,
> else
> mm = get_task_mm(proc->tsk);
>
> + preempt_enable_no_resched();
> +
> if (mm) {
> down_write(&mm->mmap_sem);
> vma = proc->vma;
> @@ -622,6 +690,9 @@ static int binder_update_page_range(struct
> binder_proc *proc, int allocate,
> up_write(&mm->mmap_sem);
> mmput(mm);
> }
> +
> + preempt_disable();
> +
> return 0;
>
> free_range:
> @@ -644,6 +715,9 @@ err_no_vma:
> up_write(&mm->mmap_sem);
> mmput(mm);
> }
> +
> + preempt_disable();
> +
> return -ENOMEM;
> }
>
> @@ -903,7 +977,7 @@ static struct binder_node *binder_new_node(struct
> binder_proc *proc,
> return NULL;
> }
>
> - node = kzalloc(sizeof(*node), GFP_KERNEL);
> + node = kzalloc_nopreempt(sizeof(*node));
> if (node == NULL)
> return NULL;
> binder_stats_created(BINDER_STAT_NODE);
> @@ -1040,7 +1114,7 @@ static struct binder_ref
> *binder_get_ref_for_node(struct binder_proc *proc,
> else
> return ref;
> }
> - new_ref = kzalloc(sizeof(*ref), GFP_KERNEL);
> + new_ref = kzalloc_nopreempt(sizeof(*ref));
> if (new_ref == NULL)
> return NULL;
> binder_stats_created(BINDER_STAT_REF);
> @@ -1438,14 +1512,14 @@ static void binder_transaction(struct binder_proc *proc,
> e->to_proc = target_proc->pid;
>
> /* TODO: reuse incoming transaction for reply */
> - t = kzalloc(sizeof(*t), GFP_KERNEL);
> + t = kzalloc_nopreempt(sizeof(*t));
> if (t == NULL) {
> return_error = BR_FAILED_REPLY;
> goto err_alloc_t_failed;
> }
> binder_stats_created(BINDER_STAT_TRANSACTION);
>
> - tcomplete = kzalloc(sizeof(*tcomplete), GFP_KERNEL);
> + tcomplete = kzalloc_nopreempt(sizeof(*tcomplete));
> if (tcomplete == NULL) {
> return_error = BR_FAILED_REPLY;
> goto err_alloc_tcomplete_failed;
> @@ -1502,15 +1576,16 @@ static void binder_transaction(struct binder_proc *proc,
> offp = (binder_size_t *)(t->buffer->data +
> ALIGN(tr->data_size, sizeof(void *)));
>
> - if (copy_from_user(t->buffer->data, (const void __user *)(uintptr_t)
> - tr->data.ptr.buffer, tr->data_size)) {
> + if (copy_from_user_nopreempt(t->buffer->data,
> + (const void __user *)(uintptr_t) tr->data.ptr.buffer,
> + tr->data_size)) {
> binder_user_error("%d:%d got transaction with invalid data ptr\n",
> proc->pid, thread->pid);
> return_error = BR_FAILED_REPLY;
> goto err_copy_data_failed;
> }
> - if (copy_from_user(offp, (const void __user *)(uintptr_t)
> - tr->data.ptr.offsets, tr->offsets_size)) {
> + if (copy_from_user_nopreempt(offp, (const void __user *)(uintptr_t)
> + tr->data.ptr.offsets, tr->offsets_size)) {
> binder_user_error("%d:%d got transaction with invalid offsets ptr\n",
> proc->pid, thread->pid);
> return_error = BR_FAILED_REPLY;
> @@ -1770,7 +1845,7 @@ static int binder_thread_write(struct binder_proc *proc,
> void __user *end = buffer + size;
>
> while (ptr < end && thread->return_error == BR_OK) {
> - if (get_user(cmd, (uint32_t __user *)ptr))
> + if (get_user_nopreempt(cmd, (uint32_t __user *)ptr))
> return -EFAULT;
> ptr += sizeof(uint32_t);
> trace_binder_command(cmd);
> @@ -1788,7 +1863,7 @@ static int binder_thread_write(struct binder_proc *proc,
> struct binder_ref *ref;
> const char *debug_string;
>
> - if (get_user(target, (uint32_t __user *)ptr))
> + if (get_user_nopreempt(target, (uint32_t __user *)ptr))
> return -EFAULT;
> ptr += sizeof(uint32_t);
> if (target == 0 && binder_context_mgr_node &&
> @@ -1838,10 +1913,12 @@ static int binder_thread_write(struct binder_proc *proc,
> binder_uintptr_t cookie;
> struct binder_node *node;
>
> - if (get_user(node_ptr, (binder_uintptr_t __user *)ptr))
> + if (get_user_nopreempt(node_ptr,
> + (binder_uintptr_t __user *)ptr))
> return -EFAULT;
> ptr += sizeof(binder_uintptr_t);
> - if (get_user(cookie, (binder_uintptr_t __user *)ptr))
> + if (get_user_nopreempt(cookie,
> + (binder_uintptr_t __user *)ptr))
> return -EFAULT;
> ptr += sizeof(binder_uintptr_t);
> node = binder_get_node(proc, node_ptr);
> @@ -1899,7 +1976,8 @@ static int binder_thread_write(struct binder_proc *proc,
> binder_uintptr_t data_ptr;
> struct binder_buffer *buffer;
>
> - if (get_user(data_ptr, (binder_uintptr_t __user *)ptr))
> + if (get_user_nopreempt(data_ptr,
> + (binder_uintptr_t __user *)ptr))
> return -EFAULT;
> ptr += sizeof(binder_uintptr_t);
>
> @@ -1941,7 +2019,7 @@ static int binder_thread_write(struct binder_proc *proc,
> case BC_REPLY: {
> struct binder_transaction_data tr;
>
> - if (copy_from_user(&tr, ptr, sizeof(tr)))
> + if (copy_from_user_nopreempt(&tr, ptr, sizeof(tr)))
> return -EFAULT;
> ptr += sizeof(tr);
> binder_transaction(proc, thread, &tr, cmd == BC_REPLY);
> @@ -1991,10 +2069,12 @@ static int binder_thread_write(struct binder_proc *proc,
> struct binder_ref *ref;
> struct binder_ref_death *death;
>
> - if (get_user(target, (uint32_t __user *)ptr))
> + if (get_user_nopreempt(target,
> + (uint32_t __user *)ptr))
> return -EFAULT;
> ptr += sizeof(uint32_t);
> - if (get_user(cookie, (binder_uintptr_t __user *)ptr))
> + if (get_user_nopreempt(cookie,
> + (binder_uintptr_t __user *)ptr))
> return -EFAULT;
> ptr += sizeof(binder_uintptr_t);
> ref = binder_get_ref(proc, target);
> @@ -2023,7 +2103,7 @@ static int binder_thread_write(struct binder_proc *proc,
> proc->pid, thread->pid);
> break;
> }
> - death = kzalloc(sizeof(*death), GFP_KERNEL);
> + death = kzalloc_nopreempt(sizeof(*death));
> if (death == NULL) {
> thread->return_error = BR_ERROR;
> binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
> @@ -2077,8 +2157,8 @@ static int binder_thread_write(struct binder_proc *proc,
> struct binder_work *w;
> binder_uintptr_t cookie;
> struct binder_ref_death *death = NULL;
> -
> - if (get_user(cookie, (binder_uintptr_t __user *)ptr))
> + if (get_user_nopreempt(cookie,
> + (binder_uintptr_t __user *)ptr))
> return -EFAULT;
>
> ptr += sizeof(cookie);
> @@ -2159,7 +2239,7 @@ static int binder_thread_read(struct binder_proc *proc,
> int wait_for_proc_work;
>
> if (*consumed == 0) {
> - if (put_user(BR_NOOP, (uint32_t __user *)ptr))
> + if (put_user_nopreempt(BR_NOOP, (uint32_t __user *)ptr))
> return -EFAULT;
> ptr += sizeof(uint32_t);
> }
> @@ -2170,7 +2250,8 @@ retry:
>
> if (thread->return_error != BR_OK && ptr < end) {
> if (thread->return_error2 != BR_OK) {
> - if (put_user(thread->return_error2, (uint32_t __user *)ptr))
> + if (put_user_nopreempt(thread->return_error2,
> + (uint32_t __user *)ptr))
> return -EFAULT;
> ptr += sizeof(uint32_t);
> binder_stat_br(proc, thread, thread->return_error2);
> @@ -2178,7 +2259,8 @@ retry:
> goto done;
> thread->return_error2 = BR_OK;
> }
> - if (put_user(thread->return_error, (uint32_t __user *)ptr))
> + if (put_user_nopreempt(thread->return_error,
> + (uint32_t __user *)ptr))
> return -EFAULT;
> ptr += sizeof(uint32_t);
> binder_stat_br(proc, thread, thread->return_error);
> @@ -2256,7 +2338,7 @@ retry:
> } break;
> case BINDER_WORK_TRANSACTION_COMPLETE: {
> cmd = BR_TRANSACTION_COMPLETE;
> - if (put_user(cmd, (uint32_t __user *)ptr))
> + if (put_user_nopreempt(cmd, (uint32_t __user *) ptr))
> return -EFAULT;
> ptr += sizeof(uint32_t);
>
> @@ -2298,15 +2380,16 @@ retry:
> node->has_weak_ref = 0;
> }
> if (cmd != BR_NOOP) {
> - if (put_user(cmd, (uint32_t __user *)ptr))
> + if (put_user_nopreempt(cmd,
> + (uint32_t __user *) ptr))
> return -EFAULT;
> ptr += sizeof(uint32_t);
> - if (put_user(node->ptr,
> - (binder_uintptr_t __user *)ptr))
> + if (put_user_nopreempt(node->ptr,
> + (binder_uintptr_t __user *)ptr))
> return -EFAULT;
> ptr += sizeof(binder_uintptr_t);
> - if (put_user(node->cookie,
> - (binder_uintptr_t __user *)ptr))
> + if (put_user_nopreempt(node->cookie,
> + (binder_uintptr_t __user *)ptr))
> return -EFAULT;
> ptr += sizeof(binder_uintptr_t);
>
> @@ -2349,11 +2432,12 @@ retry:
> cmd = BR_CLEAR_DEATH_NOTIFICATION_DONE;
> else
> cmd = BR_DEAD_BINDER;
> - if (put_user(cmd, (uint32_t __user *)ptr))
> + if (put_user_nopreempt(cmd,
> + (uint32_t __user *) ptr))
> return -EFAULT;
> ptr += sizeof(uint32_t);
> - if (put_user(death->cookie,
> - (binder_uintptr_t __user *)ptr))
> + if (put_user_nopreempt(death->cookie,
> + (binder_uintptr_t __user *) ptr))
> return -EFAULT;
> ptr += sizeof(binder_uintptr_t);
> binder_stat_br(proc, thread, cmd);
> @@ -2420,10 +2504,10 @@ retry:
> ALIGN(t->buffer->data_size,
> sizeof(void *));
>
> - if (put_user(cmd, (uint32_t __user *)ptr))
> + if (put_user_nopreempt(cmd, (uint32_t __user *) ptr))
> return -EFAULT;
> ptr += sizeof(uint32_t);
> - if (copy_to_user(ptr, &tr, sizeof(tr)))
> + if (copy_to_user_nopreempt(ptr, &tr, sizeof(tr)))
> return -EFAULT;
> ptr += sizeof(tr);
>
> @@ -2465,7 +2549,8 @@ done:
> binder_debug(BINDER_DEBUG_THREADS,
> "%d:%d BR_SPAWN_LOOPER\n",
> proc->pid, thread->pid);
> - if (put_user(BR_SPAWN_LOOPER, (uint32_t __user *)buffer))
> + if (put_user_nopreempt(BR_SPAWN_LOOPER,
> + (uint32_t __user *) buffer))
> return -EFAULT;
> binder_stat_br(proc, thread, BR_SPAWN_LOOPER);
> }
> @@ -2540,7 +2625,7 @@ static struct binder_thread
> *binder_get_thread(struct binder_proc *proc)
> break;
> }
> if (*p == NULL) {
> - thread = kzalloc(sizeof(*thread), GFP_KERNEL);
> + thread = kzalloc_nopreempt(sizeof(*thread));
> if (thread == NULL)
> return NULL;
> binder_stats_created(BINDER_STAT_THREAD);
> @@ -2644,7 +2729,7 @@ static int binder_ioctl_write_read(struct file *filp,
> ret = -EINVAL;
> goto out;
> }
> - if (copy_from_user(&bwr, ubuf, sizeof(bwr))) {
> + if (copy_from_user_nopreempt(&bwr, ubuf, sizeof(bwr))) {
> ret = -EFAULT;
> goto out;
> }
> @@ -2662,7 +2747,7 @@ static int binder_ioctl_write_read(struct file *filp,
> trace_binder_write_done(ret);
> if (ret < 0) {
> bwr.read_consumed = 0;
> - if (copy_to_user(ubuf, &bwr, sizeof(bwr)))
> + if (copy_to_user_nopreempt(ubuf, &bwr, sizeof(bwr)))
> ret = -EFAULT;
> goto out;
> }
> @@ -2676,7 +2761,7 @@ static int binder_ioctl_write_read(struct file *filp,
> if (!list_empty(&proc->todo))
> wake_up_interruptible(&proc->wait);
> if (ret < 0) {
> - if (copy_to_user(ubuf, &bwr, sizeof(bwr)))
> + if (copy_to_user_nopreempt(ubuf, &bwr, sizeof(bwr)))
> ret = -EFAULT;
> goto out;
> }
> @@ -2686,7 +2771,7 @@ static int binder_ioctl_write_read(struct file *filp,
> proc->pid, thread->pid,
> (u64)bwr.write_consumed, (u64)bwr.write_size,
> (u64)bwr.read_consumed, (u64)bwr.read_size);
> - if (copy_to_user(ubuf, &bwr, sizeof(bwr))) {
> + if (copy_to_user_nopreempt(ubuf, &bwr, sizeof(bwr))) {
> ret = -EFAULT;
> goto out;
> }
> @@ -2768,7 +2853,8 @@ static long binder_ioctl(struct file *filp,
> unsigned int cmd, unsigned long arg)
> goto err;
> break;
> case BINDER_SET_MAX_THREADS:
> - if (copy_from_user(&proc->max_threads, ubuf, sizeof(proc->max_threads))) {
> + if (copy_from_user_nopreempt(&proc->max_threads, ubuf,
> + sizeof(proc->max_threads))) {
> ret = -EINVAL;
> goto err;
> }
> @@ -2791,9 +2877,9 @@ static long binder_ioctl(struct file *filp,
> unsigned int cmd, unsigned long arg)
> ret = -EINVAL;
> goto err;
> }
> - if (put_user(BINDER_CURRENT_PROTOCOL_VERSION,
> - &ver->protocol_version)) {
> - ret = -EINVAL;
> + if (put_user_nopreempt(BINDER_CURRENT_PROTOCOL_VERSION,
> + &ver->protocol_version)) {
> + ret = -EINVAL;
> goto err;
> }
> break;
> @@ -2854,6 +2940,7 @@ static const struct vm_operations_struct binder_vm_ops = {
> static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
> {
> int ret;
> +
> struct vm_struct *area;
> struct binder_proc *proc = filp->private_data;
> const char *failure_string;
> @@ -2914,7 +3001,12 @@ static int binder_mmap(struct file *filp,
> struct vm_area_struct *vma)
> vma->vm_ops = &binder_vm_ops;
> vma->vm_private_data = proc;
>
> - if (binder_update_page_range(proc, 1, proc->buffer, proc->buffer +
> PAGE_SIZE, vma)) {
> + /* binder_update_page_range assumes preemption is disabled */
> + preempt_disable();
> + ret = binder_update_page_range(proc, 1, proc->buffer,
> + proc->buffer + PAGE_SIZE, vma);
> + preempt_enable_no_resched();
> + if (ret) {
> ret = -ENOMEM;
> failure_string = "alloc small buf";
> goto err_alloc_small_buf_failed;
> @@ -3185,8 +3277,12 @@ static void binder_deferred_func(struct
> work_struct *work)
> int defer;
>
> do {
> - binder_lock(__func__);
> + trace_binder_lock(__func__);
> + mutex_lock(&binder_main_lock);
> + trace_binder_locked(__func__);
> +
> mutex_lock(&binder_deferred_lock);
> + preempt_disable();
> if (!hlist_empty(&binder_deferred_list)) {
> proc = hlist_entry(binder_deferred_list.first,
> struct binder_proc, deferred_work_node);
> @@ -3212,7 +3308,9 @@ static void binder_deferred_func(struct work_struct *work)
> if (defer & BINDER_DEFERRED_RELEASE)
> binder_deferred_release(proc); /* frees proc */
>
> - binder_unlock(__func__);
> + trace_binder_unlock(__func__);
> + mutex_unlock(&binder_main_lock);
> + preempt_enable_no_resched();
> if (files)
> put_files_struct(files);
> } while (proc);
> --
> 2.8.0.rc3.226.g39d4020
---end quoted text---

Next message: Peter Zijlstra: "Re: [PATCH] android: binder: Disable preemption while holding the global binder lock"
Previous message: Jonathan Cameron: "Re: [PATCH 2/2] iio: 104-quad-8: Add IIO support for the ACCES 104-QUAD-8"
In reply to: Greg Kroah-Hartman: "Re: [PATCH] android: binder: Disable preemption while holding the global binder lock"
Next in thread: Peter Zijlstra: "Re: [PATCH] android: binder: Disable preemption while holding the global binder lock"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]