From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1755944AbcIJR2n (ORCPT <rfc822;w@1wt.eu>);
        Sat, 10 Sep 2016 13:28:43 -0400
Received: from mail.linuxfoundation.org ([140.211.169.12]:42928 "EHLO
        mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1755393AbcIJR2k (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Sat, 10 Sep 2016 13:28:40 -0400
Date: Sat, 10 Sep 2016 19:28:43 +0200
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>, devel@driverdev.osuosl.org,
        Riley Andrews <riandrews@android.com>, linux-kernel@vger.kernel.org,
        Christoph Hellwig <hch@infradead.org>,
        Arve Hj??nnev??g <arve@android.com>, Todd Kjos <tkjos@google.com>
Subject: Re: [PATCH] android: binder: Disable preemption while holding the
 global binder lock
Message-ID: <20160910172843.GA17876@kroah.com>
References: <CAHRSSEwkj-VxstqCm6JzVQcmZzxLQcob0JNiEhzes9oT0-tXSg@mail.gmail.com>
 <20160910161659.GA7987@infradead.org>
 <20160910162210.GK10153@twins.programming.kicks-ass.net>
 <alpine.DEB.2.20.1609101835360.5560@nanos>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.DEB.2.20.1609101835360.5560@nanos>
User-Agent: Mutt/1.7.0 (2016-08-17)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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