linux-kernel - Re: [PATCH v5] ANDROID: binder: change down_write to down

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180402063448.GA250086@rodete-desktop-imager.corp.google.com>
Date:   Mon, 2 Apr 2018 15:34:48 +0900
From:   Minchan Kim <minchan@...nel.org>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Ganesh Mahendran <opensource.ganesh@...il.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Joe Perches <joe@...ches.com>,
        Arve Hjønnevåg <arve@...roid.com>,
        Todd Kjos <tkjos@...gle.com>, Martijn Coenen <maco@...roid.com>
Subject: Re: [PATCH v5] ANDROID: binder: change down_write to down_read

On Fri, Mar 30, 2018 at 12:04:07PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Mar 30, 2018 at 10:29:21AM +0900, Minchan Kim wrote:
> > Hi Ganesh,
> > 
> > On Fri, Mar 30, 2018 at 09:21:55AM +0800, Ganesh Mahendran wrote:
> > > 2018-03-29 14:54 GMT+08:00 Minchan Kim <minchan@...nel.org>:
> > > > binder_update_page_range needs down_write of mmap_sem because
> > > > vm_insert_page need to change vma->vm_flags to VM_MIXEDMAP unless
> > > > it is set. However, when I profile binder working, it seems
> > > > every binder buffers should be mapped in advance by binder_mmap.
> > > > It means we could set VM_MIXEDMAP in binder_mmap time which is
> > > > already hold a mmap_sem as down_write so binder_update_page_range
> > > > doesn't need to hold a mmap_sem as down_write.
> > > >
> > > > Android suffers from mmap_sem contention so let's reduce mmap_sem
> > > > down_write.
> > > 
> > > Hi, Minchan:
> > > 
> > > It seems there is performance regression of this patch.
> > 
> > You mean "This patch aims for solving performance regression" not "This patch
> > makes performance regression"?
> > 
> > > 
> > > Do you have some test result of android app launch time or binderThroughput?
> > 
> > Unfortunately, I don't have any number. The goal is to reduce the number of
> > call mmap_sem as write-side lock because it makes priority inversion of threads
> > easily and that's one of clear part I spot that we don't need write-side lock.
> 
> Please always run the binderThroughput tests when making binder changes
> (there is a binder test suite in the CTS Android tests), as that ensures
> that you are not causing performance regressions as well as just normal
> bug regressions :)

Thanks for the information. I didn't notice that such kinds of tests for
binder. I will keep it in mind.

Today, I have setup the testing for my phone and found testing was very
fluctuating even without my patch. It might be not good with my test
skill. I emulated user's behavior with various touch event. With it, I open
various apps and play with them several times. Before starting the test,
I did "adb shell stop && adb shell start && echo 3 > /proc/sys/vm/drop_caches"

Such 15% noise was very easy to make it.

Ganesh, How did you measure? What's the stddev?
Please let me know how you measure without noise so I'd like to reproduce
the result in my phone.

I will do binderThroghput test, too.