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

Groups > linux.kernel > #1480665 > unrolled thread

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

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

  Re: [PATCH] android: binder: Disable preemption while holding the  global binder lock Christoph Hellwig <hch@infradead.org> - 2016-09-10 18:20 +0200
    Re: [PATCH] android: binder: Disable preemption while holding the  global binder lock Peter Zijlstra <peterz@infradead.org> - 2016-09-10 18:30 +0200
      Re: [PATCH] android: binder: Disable preemption while holding the  global binder lock Thomas Gleixner <tglx@linutronix.de> - 2016-09-10 18:50 +0200
        Re: [PATCH] android: binder: Disable preemption while holding the  global binder lock Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-09-10 19:30 +0200
          Re: [PATCH] android: binder: Disable preemption while holding the  global binder lock Todd Kjos <tkjos@android.com> - 2016-09-12 18:00 +0200
          Re: [PATCH] android: binder: Disable preemption while holding the  global binder lock Arve Hjønnevåg <arve@android.com> - 2016-09-13 05:50 +0200
            Re: [PATCH] android: binder: Disable preemption while holding the  global binder lock Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-09-13 08:50 +0200
              Re: [PATCH] android: binder: Disable preemption while holding the  global binder lock Arve Hjønnevåg <arve@android.com> - 2016-09-13 22:00 +0200
            Re: [PATCH] android: binder: Disable preemption while holding the  global binder lock Peter Zijlstra <peterz@infradead.org> - 2016-09-13 09:40 +0200
              Re: [PATCH] android: binder: Disable preemption while holding the  global binder lock Arve Hjønnevåg <arve@android.com> - 2016-09-13 22:00 +0200
                Re: [PATCH] android: binder: Disable preemption while holding the  global binder lock Peter Zijlstra <peterz@infradead.org> - 2016-09-14 09:20 +0200
                  Re: [PATCH] android: binder: Disable preemption while holding the  global binder lock Peter Zijlstra <peterz@infradead.org> - 2016-09-14 09:50 +0200
                  Re: [PATCH] android: binder: Disable preemption while holding the  global binder lock Peter Zijlstra <peterz@infradead.org> - 2016-09-14 15:40 +0200
                Re: [PATCH] android: binder: Disable preemption while holding the  global binder lock Peter Zijlstra <peterz@infradead.org> - 2016-09-14 18:20 +0200
                  Re: [PATCH] android: binder: Disable preemption while holding the  global binder lock Peter Zijlstra <peterz@infradead.org> - 2016-09-14 18:20 +0200
                    Re: [PATCH] android: binder: Disable preemption while holding the  global binder lock Peter Zijlstra <peterz@infradead.org> - 2016-09-14 19:00 +0200
                      Re: [PATCH] android: binder: Disable preemption while holding the  global binder lock Todd Kjos <tkjos@android.com> - 2016-09-17 02:50 +0200

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

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@google.com>
> Signed-off-by: Todd Kjos <tkjos@google.com>
> ---
>  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---

[toc] | [next] | [standalone]

#1480668

On Sat, Sep 10, 2016 at 09:16:59AM -0700, Christoph Hellwig wrote:
> 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

not, that's supposed to be _not_. Just to be absolutely clear, this is
NOT how you're supposed to use preempt_disable().

> sections that use per-cpu or similar resources.
> 
> > 
> > Originally-from: Riley Andrews <riandrews@google.com>
> > Signed-off-by: Todd Kjos <tkjos@google.com>

> > @@ -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();

And the fact that people want to use preempt_enable_no_resched() shows
that they're absolutely clueless.

This is so broken its not funny.

NAK NAK NAK

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

#1480673

On Sat, 10 Sep 2016, Peter Zijlstra wrote:

> On Sat, Sep 10, 2016 at 09:16:59AM -0700, Christoph Hellwig wrote:
> > 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
> 
> not, that's supposed to be _not_. Just to be absolutely clear, this is
> NOT how you're supposed to use preempt_disable().
> 
> > sections that use per-cpu or similar resources.
> > 
> > > 
> > > Originally-from: Riley Andrews <riandrews@google.com>
> > > Signed-off-by: Todd Kjos <tkjos@google.com>
> 
> > > @@ -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();
> 
> And the fact that people want to use preempt_enable_no_resched() shows
> that they're absolutely clueless.
> 
> This is so broken its not funny.
> 
> NAK NAK NAK

Indeed. Sprinkling random preempt_enabe/disable() pairs all over the place
documents clearly that this is tinkering and not proper software
engineering.

NAK from my side as well.

Thanks,

	Thomas

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

#1480679

On Sat, Sep 10, 2016 at 06:37:29PM +0200, Thomas Gleixner wrote:
> On Sat, 10 Sep 2016, Peter Zijlstra wrote:
> 
> > On Sat, Sep 10, 2016 at 09:16:59AM -0700, Christoph Hellwig wrote:
> > > 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
> > 
> > not, that's supposed to be _not_. Just to be absolutely clear, this is
> > NOT how you're supposed to use preempt_disable().
> > 
> > > sections that use per-cpu or similar resources.
> > > 
> > > > 
> > > > Originally-from: Riley Andrews <riandrews@google.com>
> > > > Signed-off-by: Todd Kjos <tkjos@google.com>
> > 
> > > > @@ -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();
> > 
> > And the fact that people want to use preempt_enable_no_resched() shows
> > that they're absolutely clueless.
> > 
> > This is so broken its not funny.
> > 
> > NAK NAK NAK
> 
> Indeed. Sprinkling random preempt_enabe/disable() pairs all over the place
> documents clearly that this is tinkering and not proper software
> engineering.

I have pointed out in the other thread for this patch (the one that had
a patch that could be applied) that the single lock in the binder code
is the main problem here, it should be solved instead of this messing
around with priorities.

So don't worry, I'm not taking this change :)

thanks,

greg k-h

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

#1481512

Thanks for the reviews. We'll come up with a different solution.

On Sat, Sep 10, 2016 at 10:28 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, Sep 10, 2016 at 06:37:29PM +0200, Thomas Gleixner wrote:
>> On Sat, 10 Sep 2016, Peter Zijlstra wrote:
>>
>> > On Sat, Sep 10, 2016 at 09:16:59AM -0700, Christoph Hellwig wrote:
>> > > 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
>> >
>> > not, that's supposed to be _not_. Just to be absolutely clear, this is
>> > NOT how you're supposed to use preempt_disable().
>> >
>> > > sections that use per-cpu or similar resources.
>> > >
>> > > >
>> > > > Originally-from: Riley Andrews <riandrews@google.com>
>> > > > Signed-off-by: Todd Kjos <tkjos@google.com>
>> >
>> > > > @@ -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();
>> >
>> > And the fact that people want to use preempt_enable_no_resched() shows
>> > that they're absolutely clueless.
>> >
>> > This is so broken its not funny.
>> >
>> > NAK NAK NAK
>>
>> Indeed. Sprinkling random preempt_enabe/disable() pairs all over the place
>> documents clearly that this is tinkering and not proper software
>> engineering.
>
> I have pointed out in the other thread for this patch (the one that had
> a patch that could be applied) that the single lock in the binder code
> is the main problem here, it should be solved instead of this messing
> around with priorities.
>
> So don't worry, I'm not taking this change :)
>
> thanks,
>
> greg k-h

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

#1482128

On Sat, Sep 10, 2016 at 10:28 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, Sep 10, 2016 at 06:37:29PM +0200, Thomas Gleixner wrote:
>> On Sat, 10 Sep 2016, Peter Zijlstra wrote:
>>
>> > On Sat, Sep 10, 2016 at 09:16:59AM -0700, Christoph Hellwig wrote:
>> > > 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
>> >
>> > not, that's supposed to be _not_. Just to be absolutely clear, this is
>> > NOT how you're supposed to use preempt_disable().
>> >
>> > > sections that use per-cpu or similar resources.
>> > >
>> > > >
>> > > > Originally-from: Riley Andrews <riandrews@google.com>
>> > > > Signed-off-by: Todd Kjos <tkjos@google.com>
>> >
>> > > > @@ -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();
>> >
>> > And the fact that people want to use preempt_enable_no_resched() shows
>> > that they're absolutely clueless.
>> >
>> > This is so broken its not funny.
>> >
>> > NAK NAK NAK
>>
>> Indeed. Sprinkling random preempt_enabe/disable() pairs all over the place
>> documents clearly that this is tinkering and not proper software
>> engineering.
>
> I have pointed out in the other thread for this patch (the one that had
> a patch that could be applied) that the single lock in the binder code
> is the main problem here, it should be solved instead of this messing
> around with priorities.
>

While removing the single lock in the binder driver would help reduce
the problem that this patch tries to work around, it would not fix it.
The largest problems occur when a very low priority thread gets
preempted while holding the lock. When a high priority thread then
needs the same lock it can't get it. Changing the driver to use more
fine-grained locking would reduce the set of threads that can trigger
this problem, but there are processes that receive work from both high
and low priority threads and could still end up in the same situation.

A previous attempt to fix this problem, changed the lock to use
rt_mutex instead of mutex, but this apparently did not work as well as
this patch. I believe the added overhead was noticeable, and it did
not work when the preempted thread was in a different cgroup (I don't
know if this is still the case).

It would be useful to generic solution to this problem.

> So don't worry, I'm not taking this change :)
>
> thanks,
>
> greg k-h



-- 
Arve Hjønnevåg

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

#1482171

On Mon, Sep 12, 2016 at 08:44:09PM -0700, Arve Hjønnevåg wrote:
> On Sat, Sep 10, 2016 at 10:28 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Sat, Sep 10, 2016 at 06:37:29PM +0200, Thomas Gleixner wrote:
> >> On Sat, 10 Sep 2016, Peter Zijlstra wrote:
> >>
> >> > On Sat, Sep 10, 2016 at 09:16:59AM -0700, Christoph Hellwig wrote:
> >> > > 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
> >> >
> >> > not, that's supposed to be _not_. Just to be absolutely clear, this is
> >> > NOT how you're supposed to use preempt_disable().
> >> >
> >> > > sections that use per-cpu or similar resources.
> >> > >
> >> > > >
> >> > > > Originally-from: Riley Andrews <riandrews@google.com>
> >> > > > Signed-off-by: Todd Kjos <tkjos@google.com>
> >> >
> >> > > > @@ -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();
> >> >
> >> > And the fact that people want to use preempt_enable_no_resched() shows
> >> > that they're absolutely clueless.
> >> >
> >> > This is so broken its not funny.
> >> >
> >> > NAK NAK NAK
> >>
> >> Indeed. Sprinkling random preempt_enabe/disable() pairs all over the place
> >> documents clearly that this is tinkering and not proper software
> >> engineering.
> >
> > I have pointed out in the other thread for this patch (the one that had
> > a patch that could be applied) that the single lock in the binder code
> > is the main problem here, it should be solved instead of this messing
> > around with priorities.
> >
> 
> While removing the single lock in the binder driver would help reduce
> the problem that this patch tries to work around, it would not fix it.
> The largest problems occur when a very low priority thread gets
> preempted while holding the lock. When a high priority thread then
> needs the same lock it can't get it. Changing the driver to use more
> fine-grained locking would reduce the set of threads that can trigger
> this problem, but there are processes that receive work from both high
> and low priority threads and could still end up in the same situation.

Ok, but how would this patch solve the problem?  It only reduces the
window of when the preemption could occur, it doesn't stop it from
happening entirely.

> A previous attempt to fix this problem, changed the lock to use
> rt_mutex instead of mutex, but this apparently did not work as well as
> this patch. I believe the added overhead was noticeable, and it did
> not work when the preempted thread was in a different cgroup (I don't
> know if this is still the case).

Why would rt_mutex solve this?

I worry that this is only going to get worse as more portions of the
Android userspace/HAL get rewritten to use binder more and more.  What
about trying to get rid of the lock entirely?  That way the task
priority levels would work "properly" here.

thanks,

greg k-h

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

#1482738

On Mon, Sep 12, 2016 at 11:42 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Sep 12, 2016 at 08:44:09PM -0700, Arve Hjønnevåg wrote:
>> On Sat, Sep 10, 2016 at 10:28 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Sat, Sep 10, 2016 at 06:37:29PM +0200, Thomas Gleixner wrote:
>> >> On Sat, 10 Sep 2016, Peter Zijlstra wrote:
>> >>
>> >> > On Sat, Sep 10, 2016 at 09:16:59AM -0700, Christoph Hellwig wrote:
>> >> > > 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
>> >> >
>> >> > not, that's supposed to be _not_. Just to be absolutely clear, this is
>> >> > NOT how you're supposed to use preempt_disable().
>> >> >
>> >> > > sections that use per-cpu or similar resources.
>> >> > >
>> >> > > >
>> >> > > > Originally-from: Riley Andrews <riandrews@google.com>
>> >> > > > Signed-off-by: Todd Kjos <tkjos@google.com>
>> >> >
>> >> > > > @@ -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();
>> >> >
>> >> > And the fact that people want to use preempt_enable_no_resched() shows
>> >> > that they're absolutely clueless.
>> >> >
>> >> > This is so broken its not funny.
>> >> >
>> >> > NAK NAK NAK
>> >>
>> >> Indeed. Sprinkling random preempt_enabe/disable() pairs all over the place
>> >> documents clearly that this is tinkering and not proper software
>> >> engineering.
>> >
>> > I have pointed out in the other thread for this patch (the one that had
>> > a patch that could be applied) that the single lock in the binder code
>> > is the main problem here, it should be solved instead of this messing
>> > around with priorities.
>> >
>>
>> While removing the single lock in the binder driver would help reduce
>> the problem that this patch tries to work around, it would not fix it.
>> The largest problems occur when a very low priority thread gets
>> preempted while holding the lock. When a high priority thread then
>> needs the same lock it can't get it. Changing the driver to use more
>> fine-grained locking would reduce the set of threads that can trigger
>> this problem, but there are processes that receive work from both high
>> and low priority threads and could still end up in the same situation.
>
> Ok, but how would this patch solve the problem?  It only reduces the
> window of when the preemption could occur, it doesn't stop it from
> happening entirely.
>

I agree, this patch does not _solve_ the problem either. It only
reduces it, as there are many places where it re-enables preemption
for significant work.

>> A previous attempt to fix this problem, changed the lock to use
>> rt_mutex instead of mutex, but this apparently did not work as well as
>> this patch. I believe the added overhead was noticeable, and it did
>> not work when the preempted thread was in a different cgroup (I don't
>> know if this is still the case).
>
> Why would rt_mutex solve this?

If the task that holds the lock would run when you try to get the
lock, there would be no long delay to get the lock. rt_mutex seems to
be designed to solve this problem.

>
> I worry that this is only going to get worse as more portions of the
> Android userspace/HAL get rewritten to use binder more and more.  What
> about trying to get rid of the lock entirely?  That way the task
> priority levels would work "properly" here.
>

I think a good first step would be to switch to locks with a smaller
scope. I'm not how useful it would be to eliminate locks in this
driver since it calls into other code that uses locks.

> thanks,
>
> greg k-h

-- 
Arve Hjønnevåg

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

#1482205

On Mon, Sep 12, 2016 at 08:44:09PM -0700, Arve Hjønnevåg wrote:

> A previous attempt to fix this problem, changed the lock to use
> rt_mutex instead of mutex, but this apparently did not work as well as
> this patch. I believe the added overhead was noticeable, and it did
> not work when the preempted thread was in a different cgroup (I don't
> know if this is still the case).

Do you actually have RR/FIFO/DL tasks? Currently PI isn't
defined/implemented for OTHER.

cgroups should be irrelevant, PI is unaware of them.

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

#1482737

On Tue, Sep 13, 2016 at 12:32 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Sep 12, 2016 at 08:44:09PM -0700, Arve Hjønnevåg wrote:
>
>> A previous attempt to fix this problem, changed the lock to use
>> rt_mutex instead of mutex, but this apparently did not work as well as
>> this patch. I believe the added overhead was noticeable, and it did
>> not work when the preempted thread was in a different cgroup (I don't
>> know if this is still the case).
>
> Do you actually have RR/FIFO/DL tasks? Currently PI isn't
> defined/implemented for OTHER.
>

Most of the tasks here are not RR/FIFO/DL tasks. I don't see anything
in the rtmutex code or documentation that indicates that they don't
work for normal tasks. From what I can tell the priority gets boosted
in every case. This may not work as well for CFS tasks as for realtime
tasks, but it should at least help when there is a large priority
difference.

> cgroups should be irrelevant, PI is unaware of them.

I don't think cgroups are irrelevant. PI being unaware of them
explains the problem I described. If the task that holds the lock is
in a cgroup that has a low cpu.shares value, then boosting the task's
priority within that group does necessarily make it any more likely to
run.

-- 
Arve Hjønnevåg

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

#1482969

On Tue, Sep 13, 2016 at 12:53:27PM -0700, Arve Hjønnevåg wrote:
> On Tue, Sep 13, 2016 at 12:32 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Mon, Sep 12, 2016 at 08:44:09PM -0700, Arve Hjønnevåg wrote:
> >
> >> A previous attempt to fix this problem, changed the lock to use
> >> rt_mutex instead of mutex, but this apparently did not work as well as
> >> this patch. I believe the added overhead was noticeable, and it did
> >> not work when the preempted thread was in a different cgroup (I don't
> >> know if this is still the case).
> >
> > Do you actually have RR/FIFO/DL tasks? Currently PI isn't
> > defined/implemented for OTHER.
> >
> 
> Most of the tasks here are not RR/FIFO/DL tasks. I don't see anything
> in the rtmutex code or documentation that indicates that they don't
> work for normal tasks. From what I can tell the priority gets boosted
> in every case. This may not work as well for CFS tasks as for realtime
> tasks, but it should at least help when there is a large priority
> difference.

It does something (it used to explicitly ignore OTHER) but its not
something well defined or usable.

> > cgroups should be irrelevant, PI is unaware of them.
> 
> I don't think cgroups are irrelevant. PI being unaware of them
> explains the problem I described. If the task that holds the lock is
> in a cgroup that has a low cpu.shares value, then boosting the task's
> priority within that group does necessarily make it any more likely to
> run.

See, the problem is that 'priority' is a meaningless concept for
OTHER/CFS.

In any case, typically only RT tasks care about PI, and the traditional
Priority Inheritance algorithm only really works correctly for FIFO. As
is RR has issues and DL is a crude hack, CFS is really just an accident
by not explicitly exempting it.

We could define a meaningful something for CFS and implement that, but
it isn't currently done.

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

#1483037

On Wed, Sep 14, 2016 at 09:10:01AM +0200, Peter Zijlstra wrote:
> We could define a meaningful something for CFS and implement that, but
> it isn't currently done.

So the generalization of the Priority Inheritance Protocol is Proxy
Execution Protocol, which basically lets the boosted task run _as_ the
task on the block chain as picked by the schedule function (treating all
of them as runnable). Where 'as' means it really consumes scheduling
resources of the (blocked) donor task.

Since the scheduling function for FIFO is: pick the highest prio one and
go for it, this trivially reduces to PI for FIFO.

Now, Proxy Execution Protocol is 'trivial' to implement on UP, but has a
few wobbles on SMP.

But we can use it to define a sensible definition for a WFQ class
scheduler (like CFS). For these the scheduling function basically runs
the boosted task as every donor task on the block chain gets their slice.
Alternatively, since it treats all 'blocked' tasks as runnable, the
total weight of the boosted task is its own weight plus the sum of
weight on the block chain.

Which is something that shouldn't be too hard to implement, but very
much isn't what happens today.

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

#1483307

On Wed, Sep 14, 2016 at 09:10:01AM +0200, Peter Zijlstra wrote:
> On Tue, Sep 13, 2016 at 12:53:27PM -0700, Arve Hjønnevåg wrote:

> > Most of the tasks here are not RR/FIFO/DL tasks. I don't see anything
> > in the rtmutex code or documentation that indicates that they don't
> > work for normal tasks. From what I can tell the priority gets boosted
> > in every case. This may not work as well for CFS tasks as for realtime
> > tasks, but it should at least help when there is a large priority
> > difference.
> 
> It does something (it used to explicitly ignore OTHER) but its not
> something well defined or usable.

I looked again, and while it updates the ->prio field for OTHER tasks,
that does not seem to cause a change to the actual weight field (which
is derived from ->static_prio).

So it really should not do anything.. as I remebered it.

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

#1483459

On Tue, Sep 13, 2016 at 12:53:27PM -0700, Arve Hjønnevåg wrote:
> On Tue, Sep 13, 2016 at 12:32 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > cgroups should be irrelevant, PI is unaware of them.
> 
> I don't think cgroups are irrelevant. PI being unaware of them
> explains the problem I described. If the task that holds the lock is
> in a cgroup that has a low cpu.shares value, then boosting the task's
> priority within that group does necessarily make it any more likely to
> run.

Thing is, for FIFO/DL the important parameters (prio and deadline resp.)
are not cgroup dependent.

For CFS you're right, and as per usual, cgroups will be a royal pain.
While we can compute the total weight in the block chain, getting that
back to a task which is stuck in a cgroup is just not going to work
well.

The only 'solution' I can come up with in a hurry is, when the task is
boosted, move it to the root cgroup. That of course has a whole heap of
problems all on its own.

/me curses @ cgroups.. bloody stupid things.

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

#1483461

On Wed, Sep 14, 2016 at 06:11:03PM +0200, Peter Zijlstra wrote:
> On Tue, Sep 13, 2016 at 12:53:27PM -0700, Arve Hjønnevåg wrote:
> > On Tue, Sep 13, 2016 at 12:32 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > > cgroups should be irrelevant, PI is unaware of them.
> > 
> > I don't think cgroups are irrelevant. PI being unaware of them
> > explains the problem I described. If the task that holds the lock is
> > in a cgroup that has a low cpu.shares value, then boosting the task's
> > priority within that group does necessarily make it any more likely to
> > run.
> 
> Thing is, for FIFO/DL the important parameters (prio and deadline resp.)
> are not cgroup dependent.
> 
> For CFS you're right, and as per usual, cgroups will be a royal pain.
> While we can compute the total weight in the block chain, getting that
> back to a task which is stuck in a cgroup is just not going to work
> well.

Not to mention the fact that the weight depends on the current running
state. Having those tasks block insta changes the actual weight.

> /me curses @ cgroups.. bloody stupid things.

More of that.

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

#1483480

On Wed, Sep 14, 2016 at 06:13:40PM +0200, Peter Zijlstra wrote:
> On Wed, Sep 14, 2016 at 06:11:03PM +0200, Peter Zijlstra wrote:
> > On Tue, Sep 13, 2016 at 12:53:27PM -0700, Arve Hjønnevåg wrote:
> > > On Tue, Sep 13, 2016 at 12:32 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > > > cgroups should be irrelevant, PI is unaware of them.
> > > 
> > > I don't think cgroups are irrelevant. PI being unaware of them
> > > explains the problem I described. If the task that holds the lock is
> > > in a cgroup that has a low cpu.shares value, then boosting the task's
> > > priority within that group does necessarily make it any more likely to
> > > run.
> > 
> > Thing is, for FIFO/DL the important parameters (prio and deadline resp.)
> > are not cgroup dependent.
> > 
> > For CFS you're right, and as per usual, cgroups will be a royal pain.
> > While we can compute the total weight in the block chain, getting that
> > back to a task which is stuck in a cgroup is just not going to work
> > well.
> 
> Not to mention the fact that the weight depends on the current running
> state. Having those tasks block insta changes the actual weight.
> 
> > /me curses @ cgroups.. bloody stupid things.
> 
> More of that.


Something a little like so... completely untested, and has a fair number
of corner cases still left open I think..


--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1691,8 +1691,8 @@ struct task_struct {
 	struct seccomp seccomp;
 
 /* Thread group tracking */
-   	u32 parent_exec_id;
-   	u32 self_exec_id;
+	u32 parent_exec_id;
+	u32 self_exec_id;
 /* Protection of (de-)allocation: mm, files, fs, tty, keyrings, mems_allowed,
  * mempolicy */
 	spinlock_t alloc_lock;
@@ -1710,6 +1710,11 @@ struct task_struct {
 	struct task_struct *pi_top_task;
 	/* Deadlock detection and priority inheritance handling */
 	struct rt_mutex_waiter *pi_blocked_on;
+	unsigned long pi_weight;
+#ifdef CONFIG_CGROUP_SCHED
+	struct task_group *orig_task_group;
+	unsigned long normal_weight;
+#endif
 #endif
 
 #ifdef CONFIG_DEBUG_MUTEXES
@@ -1977,6 +1982,8 @@ static inline int tsk_nr_cpus_allowed(st
 	return p->nr_cpus_allowed;
 }
 
+extern unsigned long task_pi_weight(struct task_struct *p);
+
 #define TNF_MIGRATED	0x01
 #define TNF_NO_GROUP	0x02
 #define TNF_SHARED	0x04
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -20,6 +20,19 @@
 #include "rtmutex_common.h"
 
 /*
+ * !(rt_prio || dl_prio)
+ */
+static inline bool other_prio(int prio)
+{
+	return prio >= MAX_RT_PRIO;
+}
+
+static inline bool other_task(struct task_struct *task)
+{
+	return other_prio(task->prio);
+}
+
+/*
  * lock->owner state tracking:
  *
  * lock->owner holds the task_struct pointer of the owner. Bit 0
@@ -226,6 +239,11 @@ rt_mutex_enqueue(struct rt_mutex *lock,
 
 	rb_link_node(&waiter->tree_entry, parent, link);
 	rb_insert_color(&waiter->tree_entry, &lock->waiters);
+	/*
+	 * We want the lock->waiter_weight to reflect the sum of all queued
+	 * waiters.
+	 */
+	lock->waiters_weight += waiter->weight;
 }
 
 static void
@@ -239,6 +257,7 @@ rt_mutex_dequeue(struct rt_mutex *lock,
 
 	rb_erase(&waiter->tree_entry, &lock->waiters);
 	RB_CLEAR_NODE(&waiter->tree_entry);
+	lock->waiters_weight += waiter->weight;
 }
 
 static void
@@ -265,6 +284,18 @@ rt_mutex_enqueue_pi(struct task_struct *
 
 	rb_link_node(&waiter->pi_tree_entry, parent, link);
 	rb_insert_color(&waiter->pi_tree_entry, &task->pi_waiters);
+
+	/*
+	 * Since a task can own multiple locks, and we have one pi_waiter
+	 * for every lock, propagate the lock->waiter_weight, which is the sum
+	 * of all weights queued on that lock, into the top waiter, and add
+	 * that to the owning task's pi_weight.
+	 *
+	 * This results in pi_weight being the sum of all blocked waiters
+	 * on this task.
+	 */
+	waiter->top_weight = waiter->lock->waiters_weight;
+	task->pi_weight += waiter->top_weight;
 }
 
 static void
@@ -278,6 +309,7 @@ rt_mutex_dequeue_pi(struct task_struct *
 
 	rb_erase(&waiter->pi_tree_entry, &task->pi_waiters);
 	RB_CLEAR_NODE(&waiter->pi_tree_entry);
+	task->pi_weight -= waiter->top_weight;
 }
 
 static void rt_mutex_adjust_prio(struct task_struct *p)
@@ -497,7 +529,7 @@ static int rt_mutex_adjust_prio_chain(st
 		 * detection is enabled we continue, but stop the
 		 * requeueing in the chain walk.
 		 */
-		if (top_waiter != task_top_pi_waiter(task)) {
+		if (top_waiter != task_top_pi_waiter(task) && !other_task(task)) {
 			if (!detect_deadlock)
 				goto out_unlock_pi;
 			else
@@ -512,7 +544,7 @@ static int rt_mutex_adjust_prio_chain(st
 	 * enabled we continue, but stop the requeueing in the chain
 	 * walk.
 	 */
-	if (rt_mutex_waiter_equal(waiter, cmp_task(task))) {
+	if (rt_mutex_waiter_equal(waiter, cmp_task(task)) && !other_task(task)) {
 		if (!detect_deadlock)
 			goto out_unlock_pi;
 		else
@@ -627,6 +659,11 @@ static int rt_mutex_adjust_prio_chain(st
 	 */
 	waiter->prio = task->prio;
 	waiter->deadline = task->dl.deadline;
+	/*
+	 * The weight of the task depends on the block chain, since
+	 * we're iterating that, its likely to have changed.
+	 */
+	waiter->weight = task_pi_weight(task);
 
 	rt_mutex_enqueue(lock, waiter);
 
@@ -685,6 +722,15 @@ static int rt_mutex_adjust_prio_chain(st
 		waiter = rt_mutex_top_waiter(lock);
 		rt_mutex_enqueue_pi(task, waiter);
 		rt_mutex_adjust_prio(task);
+
+	} else if (other_task(task)) {
+		/*
+		 * Propagate the lock->waiters_weight into task->pi_weight
+		 */
+		rt_mutex_dequeue_pi(task, prerequeue_top_waiter);
+		rt_mutex_enqueue_pi(task, prerequeue_top_waiter);
+		rt_mutex_adjust_prio(task);
+
 	} else {
 		/*
 		 * Nothing changed. No need to do any priority
@@ -728,7 +774,7 @@ static int rt_mutex_adjust_prio_chain(st
 	 * then we can stop the chain walk here if we are not in full
 	 * deadlock detection mode.
 	 */
-	if (!detect_deadlock && waiter != top_waiter)
+	if (!detect_deadlock && waiter != top_waiter && !other_task(task))
 		goto out_put_task;
 
 	goto again;
@@ -904,6 +950,7 @@ static int task_blocks_on_rt_mutex(struc
 	waiter->lock = lock;
 	waiter->prio = task->prio;
 	waiter->deadline = task->dl.deadline;
+	waiter->weight = task_pi_weight(task);
 
 	/* Get the top priority waiter on the lock */
 	if (rt_mutex_has_waiters(lock))
@@ -918,7 +965,7 @@ static int task_blocks_on_rt_mutex(struc
 		return 0;
 
 	raw_spin_lock(&owner->pi_lock);
-	if (waiter == rt_mutex_top_waiter(lock)) {
+	if (other_task(owner) || waiter == rt_mutex_top_waiter(lock)) {
 		rt_mutex_dequeue_pi(owner, top_waiter);
 		rt_mutex_enqueue_pi(owner, waiter);
 
@@ -1017,7 +1064,8 @@ static void mark_wakeup_next_waiter(stru
 static void remove_waiter(struct rt_mutex *lock,
 			  struct rt_mutex_waiter *waiter)
 {
-	bool is_top_waiter = (waiter == rt_mutex_top_waiter(lock));
+	struct rt_mutex_waiter *top_waiter = rt_mutex_top_waiter(lock);
+	bool is_top_waiter = (waiter == top_waiter);
 	struct task_struct *owner = rt_mutex_owner(lock);
 	struct rt_mutex *next_lock;
 
@@ -1032,12 +1080,12 @@ static void remove_waiter(struct rt_mute
 	 * Only update priority if the waiter was the highest priority
 	 * waiter of the lock and there is an owner to update.
 	 */
-	if (!owner || !is_top_waiter)
+	if (!owner || (!other_task(owner) && !is_top_waiter))
 		return;
 
 	raw_spin_lock(&owner->pi_lock);
 
-	rt_mutex_dequeue_pi(owner, waiter);
+	rt_mutex_dequeue_pi(owner, top_waiter);
 
 	if (rt_mutex_has_waiters(lock))
 		rt_mutex_enqueue_pi(owner, rt_mutex_top_waiter(lock));
--- a/kernel/locking/rtmutex_common.h
+++ b/kernel/locking/rtmutex_common.h
@@ -34,6 +34,7 @@ struct rt_mutex_waiter {
 #endif
 	int prio;
 	u64 deadline;
+	unsigned weight, top_weight;
 };
 
 /*
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3725,6 +3725,8 @@ void rt_mutex_setprio(struct task_struct
 		if (rt_prio(oldprio))
 			p->rt.timeout = 0;
 		p->sched_class = &fair_sched_class;
+
+		task_set_pi_weight(p);
 	}
 
 	p->prio = prio;
@@ -7933,6 +7935,12 @@ static void sched_change_group(struct ta
 	tg = container_of(task_css_check(tsk, cpu_cgrp_id, true),
 			  struct task_group, css);
 	tg = autogroup_task_group(tsk, tg);
+
+#ifdef CONFIG_RT_MUTEXES
+	tsk->orig_task_group = tg;
+
+	if (!tsk->pi_weight)
+#endif
 	tsk->sched_task_group = tg;
 
 #ifdef CONFIG_FAIR_GROUP_SCHED
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6646,6 +6646,32 @@ static void attach_tasks(struct lb_env *
 	raw_spin_unlock(&env->dst_rq->lock);
 }
 
+#ifdef CONFIG_RT_MUTEXES
+/*
+ * See set_load_weight().
+ */
+static inline unsigned long __task_normal_weight(struct task_struct *p)
+{
+	int prio = p->static_prio - MAX_RT_PRIO;
+
+	/*
+	 * SCHED_IDLE tasks get minimal weight:
+	 */
+	if (idle_policy(p->policy))
+		return scale_load(WEIGHT_IDLEPRIO);
+
+	return scale_load(sched_prio_to_weight[prio]);
+}
+
+
+static unsigned long task_normal_weight(struct task_struct *p);
+
+unsigned long task_pi_weight(struct task_struct *p)
+{
+	return task_normal_weight(p) + p->pi_weight;
+}
+#endif
+
 #ifdef CONFIG_FAIR_GROUP_SCHED
 static void update_blocked_averages(int cpu)
 {
@@ -6717,7 +6743,80 @@ static unsigned long task_h_load(struct
 	return div64_ul(p->se.avg.load_avg * cfs_rq->h_load,
 			cfs_rq_load_avg(cfs_rq) + 1);
 }
-#else
+
+#ifdef CONFIG_RT_MUTEXES
+/*
+ * Humongous horrid hack.. because cgroups bloody stink.
+ *
+ * The idea with PI on CFS is to sum the weight of all blocked tasks but with
+ * cgroups the weight of a task depends on the running state of tasks. Blocking
+ * changes the weight.
+ *
+ * Paper over that problem by using the regular averages, and hoping boosting
+ * is short enough to not actually matter much.
+ *
+ * The next problem is that getting that weight back to the boosted task
+ * requires lifting it out of its cgroup. Ideally we'd place it in the first
+ * common parent, but *shees* then we'd have to compute that, so bail and stick
+ * it in the root group.
+ */
+
+static unsigned long task_normal_weight(struct task_struct *p)
+{
+	struct cfs_rq *cfs_rq = task_cfs_rq(p);
+
+	/*
+	 * Once we have ->pi_weight, we'll get boosted into the root group
+	 * and the below falls apart.
+	 */
+	if (!p->pi_weight) {
+		update_cfs_rq_h_load(cfs_rq);
+		p->normal_weight = 
+			div64_ul(__task_normal_weight(p) * cfs_rq->h_load,
+					cfs_rq_load_avg(cfs_rq) + 1);
+	}
+
+	return p->normal_weight;
+}
+
+static void detach_task_cfs_rq(struct task_struct *p);
+static void attach_task_cfs_rq(struct task_struct *p);
+
+void task_set_pi_weight(struct task_struct *p)
+{
+	unsigned long normal_weight = p->normal_weight;
+	struct task_group *tg = &root_task_group;
+	bool move_group;
+
+	if (!p->pi_weight) {
+		tg = p->orig_task_group;
+		normal_weight = __task_normal_weight(p);
+	}
+
+	move_group = p->sched_task_group != tg;
+
+	if (move_group) {
+		p->sched_task_group = tg;
+
+		detach_task_cfs_rq(p);
+		set_task_rq(p, task_cpu(p));
+
+#ifdef CONFIG_SMP
+		/* Tell se's cfs_rq has been changed -- migrated */
+		p->se.avg.last_update_time = 0;
+#endif
+	}
+
+	update_load_set(&p->se.load, normal_weight + p->pi_weight);
+
+	if (move_group)
+		attach_task_cfs_rq(p);
+}
+
+#endif
+
+#else /* CONFIG_FAIR_GROUP_SCHED */
+
 static inline void update_blocked_averages(int cpu)
 {
 	struct rq *rq = cpu_rq(cpu);
@@ -6734,8 +6833,21 @@ static unsigned long task_h_load(struct
 {
 	return p->se.avg.load_avg;
 }
+
+#ifdef CONFIG_RT_MUTEXES
+static unsigned long task_normal_weight(struct task_struct *p)
+{
+	return __task_normal_weight(p);
+}
+
+void task_set_pi_weight(struct task_struct *p)
+{
+	update_load_set(&p->se.load, task_pi_weight(p));
+}
 #endif
 
+#endif /* CONFIG_FAIR_GROUP_SCHED */
+
 /********** Helpers for find_busiest_group ************************/
 
 enum group_type {
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1827,3 +1827,5 @@ static inline void cpufreq_trigger_updat
 #else /* arch_scale_freq_capacity */
 #define arch_scale_freq_invariant()	(false)
 #endif
+
+extern void task_set_pi_weight(struct task_struct *p);

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

#1485444

Thanks Peter. We'll give that patch a try as part of our refactoring.
Looking at finer-grained locking and we'll try going back to rt_mutex
plus this patch.

On Wed, Sep 14, 2016 at 9:55 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Sep 14, 2016 at 06:13:40PM +0200, Peter Zijlstra wrote:
>> On Wed, Sep 14, 2016 at 06:11:03PM +0200, Peter Zijlstra wrote:
>> > On Tue, Sep 13, 2016 at 12:53:27PM -0700, Arve Hjønnevåg wrote:
>> > > On Tue, Sep 13, 2016 at 12:32 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > > > cgroups should be irrelevant, PI is unaware of them.
>> > >
>> > > I don't think cgroups are irrelevant. PI being unaware of them
>> > > explains the problem I described. If the task that holds the lock is
>> > > in a cgroup that has a low cpu.shares value, then boosting the task's
>> > > priority within that group does necessarily make it any more likely to
>> > > run.
>> >
>> > Thing is, for FIFO/DL the important parameters (prio and deadline resp.)
>> > are not cgroup dependent.
>> >
>> > For CFS you're right, and as per usual, cgroups will be a royal pain.
>> > While we can compute the total weight in the block chain, getting that
>> > back to a task which is stuck in a cgroup is just not going to work
>> > well.
>>
>> Not to mention the fact that the weight depends on the current running
>> state. Having those tasks block insta changes the actual weight.
>>
>> > /me curses @ cgroups.. bloody stupid things.
>>
>> More of that.
>
>
> Something a little like so... completely untested, and has a fair number
> of corner cases still left open I think..
>
>
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1691,8 +1691,8 @@ struct task_struct {
>         struct seccomp seccomp;
>
>  /* Thread group tracking */
> -       u32 parent_exec_id;
> -       u32 self_exec_id;
> +       u32 parent_exec_id;
> +       u32 self_exec_id;
>  /* Protection of (de-)allocation: mm, files, fs, tty, keyrings, mems_allowed,
>   * mempolicy */
>         spinlock_t alloc_lock;
> @@ -1710,6 +1710,11 @@ struct task_struct {
>         struct task_struct *pi_top_task;
>         /* Deadlock detection and priority inheritance handling */
>         struct rt_mutex_waiter *pi_blocked_on;
> +       unsigned long pi_weight;
> +#ifdef CONFIG_CGROUP_SCHED
> +       struct task_group *orig_task_group;
> +       unsigned long normal_weight;
> +#endif
>  #endif
>
>  #ifdef CONFIG_DEBUG_MUTEXES
> @@ -1977,6 +1982,8 @@ static inline int tsk_nr_cpus_allowed(st
>         return p->nr_cpus_allowed;
>  }
>
> +extern unsigned long task_pi_weight(struct task_struct *p);
> +
>  #define TNF_MIGRATED   0x01
>  #define TNF_NO_GROUP   0x02
>  #define TNF_SHARED     0x04
> --- a/kernel/locking/rtmutex.c
> +++ b/kernel/locking/rtmutex.c
> @@ -20,6 +20,19 @@
>  #include "rtmutex_common.h"
>
>  /*
> + * !(rt_prio || dl_prio)
> + */
> +static inline bool other_prio(int prio)
> +{
> +       return prio >= MAX_RT_PRIO;
> +}
> +
> +static inline bool other_task(struct task_struct *task)
> +{
> +       return other_prio(task->prio);
> +}
> +
> +/*
>   * lock->owner state tracking:
>   *
>   * lock->owner holds the task_struct pointer of the owner. Bit 0
> @@ -226,6 +239,11 @@ rt_mutex_enqueue(struct rt_mutex *lock,
>
>         rb_link_node(&waiter->tree_entry, parent, link);
>         rb_insert_color(&waiter->tree_entry, &lock->waiters);
> +       /*
> +        * We want the lock->waiter_weight to reflect the sum of all queued
> +        * waiters.
> +        */
> +       lock->waiters_weight += waiter->weight;
>  }
>
>  static void
> @@ -239,6 +257,7 @@ rt_mutex_dequeue(struct rt_mutex *lock,
>
>         rb_erase(&waiter->tree_entry, &lock->waiters);
>         RB_CLEAR_NODE(&waiter->tree_entry);
> +       lock->waiters_weight += waiter->weight;
>  }
>
>  static void
> @@ -265,6 +284,18 @@ rt_mutex_enqueue_pi(struct task_struct *
>
>         rb_link_node(&waiter->pi_tree_entry, parent, link);
>         rb_insert_color(&waiter->pi_tree_entry, &task->pi_waiters);
> +
> +       /*
> +        * Since a task can own multiple locks, and we have one pi_waiter
> +        * for every lock, propagate the lock->waiter_weight, which is the sum
> +        * of all weights queued on that lock, into the top waiter, and add
> +        * that to the owning task's pi_weight.
> +        *
> +        * This results in pi_weight being the sum of all blocked waiters
> +        * on this task.
> +        */
> +       waiter->top_weight = waiter->lock->waiters_weight;
> +       task->pi_weight += waiter->top_weight;
>  }
>
>  static void
> @@ -278,6 +309,7 @@ rt_mutex_dequeue_pi(struct task_struct *
>
>         rb_erase(&waiter->pi_tree_entry, &task->pi_waiters);
>         RB_CLEAR_NODE(&waiter->pi_tree_entry);
> +       task->pi_weight -= waiter->top_weight;
>  }
>
>  static void rt_mutex_adjust_prio(struct task_struct *p)
> @@ -497,7 +529,7 @@ static int rt_mutex_adjust_prio_chain(st
>                  * detection is enabled we continue, but stop the
>                  * requeueing in the chain walk.
>                  */
> -               if (top_waiter != task_top_pi_waiter(task)) {
> +               if (top_waiter != task_top_pi_waiter(task) && !other_task(task)) {
>                         if (!detect_deadlock)
>                                 goto out_unlock_pi;
>                         else
> @@ -512,7 +544,7 @@ static int rt_mutex_adjust_prio_chain(st
>          * enabled we continue, but stop the requeueing in the chain
>          * walk.
>          */
> -       if (rt_mutex_waiter_equal(waiter, cmp_task(task))) {
> +       if (rt_mutex_waiter_equal(waiter, cmp_task(task)) && !other_task(task)) {
>                 if (!detect_deadlock)
>                         goto out_unlock_pi;
>                 else
> @@ -627,6 +659,11 @@ static int rt_mutex_adjust_prio_chain(st
>          */
>         waiter->prio = task->prio;
>         waiter->deadline = task->dl.deadline;
> +       /*
> +        * The weight of the task depends on the block chain, since
> +        * we're iterating that, its likely to have changed.
> +        */
> +       waiter->weight = task_pi_weight(task);
>
>         rt_mutex_enqueue(lock, waiter);
>
> @@ -685,6 +722,15 @@ static int rt_mutex_adjust_prio_chain(st
>                 waiter = rt_mutex_top_waiter(lock);
>                 rt_mutex_enqueue_pi(task, waiter);
>                 rt_mutex_adjust_prio(task);
> +
> +       } else if (other_task(task)) {
> +               /*
> +                * Propagate the lock->waiters_weight into task->pi_weight
> +                */
> +               rt_mutex_dequeue_pi(task, prerequeue_top_waiter);
> +               rt_mutex_enqueue_pi(task, prerequeue_top_waiter);
> +               rt_mutex_adjust_prio(task);
> +
>         } else {
>                 /*
>                  * Nothing changed. No need to do any priority
> @@ -728,7 +774,7 @@ static int rt_mutex_adjust_prio_chain(st
>          * then we can stop the chain walk here if we are not in full
>          * deadlock detection mode.
>          */
> -       if (!detect_deadlock && waiter != top_waiter)
> +       if (!detect_deadlock && waiter != top_waiter && !other_task(task))
>                 goto out_put_task;
>
>         goto again;
> @@ -904,6 +950,7 @@ static int task_blocks_on_rt_mutex(struc
>         waiter->lock = lock;
>         waiter->prio = task->prio;
>         waiter->deadline = task->dl.deadline;
> +       waiter->weight = task_pi_weight(task);
>
>         /* Get the top priority waiter on the lock */
>         if (rt_mutex_has_waiters(lock))
> @@ -918,7 +965,7 @@ static int task_blocks_on_rt_mutex(struc
>                 return 0;
>
>         raw_spin_lock(&owner->pi_lock);
> -       if (waiter == rt_mutex_top_waiter(lock)) {
> +       if (other_task(owner) || waiter == rt_mutex_top_waiter(lock)) {
>                 rt_mutex_dequeue_pi(owner, top_waiter);
>                 rt_mutex_enqueue_pi(owner, waiter);
>
> @@ -1017,7 +1064,8 @@ static void mark_wakeup_next_waiter(stru
>  static void remove_waiter(struct rt_mutex *lock,
>                           struct rt_mutex_waiter *waiter)
>  {
> -       bool is_top_waiter = (waiter == rt_mutex_top_waiter(lock));
> +       struct rt_mutex_waiter *top_waiter = rt_mutex_top_waiter(lock);
> +       bool is_top_waiter = (waiter == top_waiter);
>         struct task_struct *owner = rt_mutex_owner(lock);
>         struct rt_mutex *next_lock;
>
> @@ -1032,12 +1080,12 @@ static void remove_waiter(struct rt_mute
>          * Only update priority if the waiter was the highest priority
>          * waiter of the lock and there is an owner to update.
>          */
> -       if (!owner || !is_top_waiter)
> +       if (!owner || (!other_task(owner) && !is_top_waiter))
>                 return;
>
>         raw_spin_lock(&owner->pi_lock);
>
> -       rt_mutex_dequeue_pi(owner, waiter);
> +       rt_mutex_dequeue_pi(owner, top_waiter);
>
>         if (rt_mutex_has_waiters(lock))
>                 rt_mutex_enqueue_pi(owner, rt_mutex_top_waiter(lock));
> --- a/kernel/locking/rtmutex_common.h
> +++ b/kernel/locking/rtmutex_common.h
> @@ -34,6 +34,7 @@ struct rt_mutex_waiter {
>  #endif
>         int prio;
>         u64 deadline;
> +       unsigned weight, top_weight;
>  };
>
>  /*
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3725,6 +3725,8 @@ void rt_mutex_setprio(struct task_struct
>                 if (rt_prio(oldprio))
>                         p->rt.timeout = 0;
>                 p->sched_class = &fair_sched_class;
> +
> +               task_set_pi_weight(p);
>         }
>
>         p->prio = prio;
> @@ -7933,6 +7935,12 @@ static void sched_change_group(struct ta
>         tg = container_of(task_css_check(tsk, cpu_cgrp_id, true),
>                           struct task_group, css);
>         tg = autogroup_task_group(tsk, tg);
> +
> +#ifdef CONFIG_RT_MUTEXES
> +       tsk->orig_task_group = tg;
> +
> +       if (!tsk->pi_weight)
> +#endif
>         tsk->sched_task_group = tg;
>
>  #ifdef CONFIG_FAIR_GROUP_SCHED
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6646,6 +6646,32 @@ static void attach_tasks(struct lb_env *
>         raw_spin_unlock(&env->dst_rq->lock);
>  }
>
> +#ifdef CONFIG_RT_MUTEXES
> +/*
> + * See set_load_weight().
> + */
> +static inline unsigned long __task_normal_weight(struct task_struct *p)
> +{
> +       int prio = p->static_prio - MAX_RT_PRIO;
> +
> +       /*
> +        * SCHED_IDLE tasks get minimal weight:
> +        */
> +       if (idle_policy(p->policy))
> +               return scale_load(WEIGHT_IDLEPRIO);
> +
> +       return scale_load(sched_prio_to_weight[prio]);
> +}
> +
> +
> +static unsigned long task_normal_weight(struct task_struct *p);
> +
> +unsigned long task_pi_weight(struct task_struct *p)
> +{
> +       return task_normal_weight(p) + p->pi_weight;
> +}
> +#endif
> +
>  #ifdef CONFIG_FAIR_GROUP_SCHED
>  static void update_blocked_averages(int cpu)
>  {
> @@ -6717,7 +6743,80 @@ static unsigned long task_h_load(struct
>         return div64_ul(p->se.avg.load_avg * cfs_rq->h_load,
>                         cfs_rq_load_avg(cfs_rq) + 1);
>  }
> -#else
> +
> +#ifdef CONFIG_RT_MUTEXES
> +/*
> + * Humongous horrid hack.. because cgroups bloody stink.
> + *
> + * The idea with PI on CFS is to sum the weight of all blocked tasks but with
> + * cgroups the weight of a task depends on the running state of tasks. Blocking
> + * changes the weight.
> + *
> + * Paper over that problem by using the regular averages, and hoping boosting
> + * is short enough to not actually matter much.
> + *
> + * The next problem is that getting that weight back to the boosted task
> + * requires lifting it out of its cgroup. Ideally we'd place it in the first
> + * common parent, but *shees* then we'd have to compute that, so bail and stick
> + * it in the root group.
> + */
> +
> +static unsigned long task_normal_weight(struct task_struct *p)
> +{
> +       struct cfs_rq *cfs_rq = task_cfs_rq(p);
> +
> +       /*
> +        * Once we have ->pi_weight, we'll get boosted into the root group
> +        * and the below falls apart.
> +        */
> +       if (!p->pi_weight) {
> +               update_cfs_rq_h_load(cfs_rq);
> +               p->normal_weight =
> +                       div64_ul(__task_normal_weight(p) * cfs_rq->h_load,
> +                                       cfs_rq_load_avg(cfs_rq) + 1);
> +       }
> +
> +       return p->normal_weight;
> +}
> +
> +static void detach_task_cfs_rq(struct task_struct *p);
> +static void attach_task_cfs_rq(struct task_struct *p);
> +
> +void task_set_pi_weight(struct task_struct *p)
> +{
> +       unsigned long normal_weight = p->normal_weight;
> +       struct task_group *tg = &root_task_group;
> +       bool move_group;
> +
> +       if (!p->pi_weight) {
> +               tg = p->orig_task_group;
> +               normal_weight = __task_normal_weight(p);
> +       }
> +
> +       move_group = p->sched_task_group != tg;
> +
> +       if (move_group) {
> +               p->sched_task_group = tg;
> +
> +               detach_task_cfs_rq(p);
> +               set_task_rq(p, task_cpu(p));
> +
> +#ifdef CONFIG_SMP
> +               /* Tell se's cfs_rq has been changed -- migrated */
> +               p->se.avg.last_update_time = 0;
> +#endif
> +       }
> +
> +       update_load_set(&p->se.load, normal_weight + p->pi_weight);
> +
> +       if (move_group)
> +               attach_task_cfs_rq(p);
> +}
> +
> +#endif
> +
> +#else /* CONFIG_FAIR_GROUP_SCHED */
> +
>  static inline void update_blocked_averages(int cpu)
>  {
>         struct rq *rq = cpu_rq(cpu);
> @@ -6734,8 +6833,21 @@ static unsigned long task_h_load(struct
>  {
>         return p->se.avg.load_avg;
>  }
> +
> +#ifdef CONFIG_RT_MUTEXES
> +static unsigned long task_normal_weight(struct task_struct *p)
> +{
> +       return __task_normal_weight(p);
> +}
> +
> +void task_set_pi_weight(struct task_struct *p)
> +{
> +       update_load_set(&p->se.load, task_pi_weight(p));
> +}
>  #endif
>
> +#endif /* CONFIG_FAIR_GROUP_SCHED */
> +
>  /********** Helpers for find_busiest_group ************************/
>
>  enum group_type {
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -1827,3 +1827,5 @@ static inline void cpufreq_trigger_updat
>  #else /* arch_scale_freq_capacity */
>  #define arch_scale_freq_invariant()    (false)
>  #endif
> +
> +extern void task_set_pi_weight(struct task_struct *p);

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web