Date: Sun, 9 Oct 2016 15:58:43 +0200
From: Greg KH <gregkh@linuxfoundation.org>
To: YiPing Xu <xuyiping@hisilicon.com>
Cc: Laura Abbott <labbott@redhat.com>, sumit.semwal@linaro.org,
        arve@android.com, riandrews@android.com, devel@driverdev.osuosl.org,
        linux-kernel@vger.kernel.org, puck.chen@hisilicon.com,
        lili53@huawei.com, suzhuangluan@hisilicon.com
Subject: Re: [patch] staging: ion: use two separate locks for heaps and
 clients in ion_device
Message-ID: <20161009135842.GD30914@kroah.com>
References: <1475223503-65906-1-git-send-email-xuyiping@hisilicon.com>
 <37a3350a-d934-8d52-e5ab-0634687aaf46@redhat.com>
 <57F75C85.5050406@hisilicon.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <57F75C85.5050406@hisilicon.com>
User-Agent: Mutt/1.7.0 (2016-08-17)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1471
Lines: 48

On Fri, Oct 07, 2016 at 04:27:49PM +0800, YiPing Xu wrote:
> 
> 
> On 2016/10/5 2:02, Laura Abbott wrote:
> > On 09/30/2016 01:18 AM, Xu YiPing wrote:
> > > ion_alloc may get into slow path to get free page,
> > > the call stack:
> > > 
> > > __alloc_pages_slowpath
> > > ion_page_pool_alloc_pages
> > > alloc_buffer_page
> > > ion_system_heap_allocate
> > > ion_buffer_create  <-- hold ion_device->lock
> > > ion_alloc
> > > 
> > > after that, kernel invokes low-memory killer to kill some apps in
> > > order to free memory in android system. However, sometimes, the
> > > killing is blocked,
> > > the app's call stack:
> > > 
> > > rwsem_down_write_failed
> > > down_write
> > > ion_client_destroy
> > > ion_release
> > > fput
> > > do_signal
> > > 
> > > the killing is blocked because ion_device->lock is held by ion_alloc.
> > > 
> > > ion_alloc hold the lock for accessing the heaps list,
> > > ion_destroy_client hold the lock for accessing the clients list.
> > > 
> > > so, we can use two separate locks for heaps and clients, to avoid the
> > > unnecessary race.
> > > 
> > 
> > I've reviewed this and it looks okay at first pass but I don't want it
> > applied just yet. Ion locking is a bit of a mess and has been added
> 
> yes, and now "debugfs_mutex" and "ion_root_client" is redundant, after
> commit 49d200deaa680501f19a247b1fffb29301e51d2b and
> 9fd4dcece43a53e5a9e65a973df5693702ee6401.

Ok, now dropping this patch from my queue.

thanks,

greg k-h