From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755720AbcIJQRF (ORCPT ); Sat, 10 Sep 2016 12:17:05 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:48337 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755145AbcIJQRD (ORCPT ); Sat, 10 Sep 2016 12:17:03 -0400 Date: Sat, 10 Sep 2016 09:16:59 -0700 From: Christoph Hellwig To: Todd Kjos Cc: Greg Kroah-Hartman , Arve Hj??nnev??g , Riley Andrews , devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org, Thomas Gleixner , Peter Zijlstra Subject: Re: [PATCH] android: binder: Disable preemption while holding the global binder lock Message-ID: <20160910161659.GA7987@infradead.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.1 (2016-04-27) X-SRS-Rewrite: SMTP reverse-path rewritten from by bombadil.infradead.org. See http://www.infradead.org/rpr.html Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Signed-off-by: Todd Kjos > --- > 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---