Re: [patch] staging: ion: use two separate locks for heaps and clients in ion_device

From: Laura Abbott
Date: Tue Oct 04 2016 - 14:02:33 EST


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
once piece at a time. It needs a fundamental review. There will be Ion
discussions at plumbers at the end of October. Let's come back to this
after plumbers.

Signed-off-by: Xu YiPing <xuyiping@xxxxxxxxxxxxx>
---
drivers/staging/android/ion/ion.c | 37 ++++++++++++++++++++-----------------
1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index a2cf93b..331ad0f 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -46,7 +46,8 @@
* @dev: the actual misc device
* @buffers: an rb tree of all the existing buffers
* @buffer_lock: lock protecting the tree of buffers
- * @lock: rwsem protecting the tree of heaps and clients
+ * @client_lock: rwsem protecting the tree of clients
+ * @heap_lock: rwsem protecting the tree of heaps
* @heaps: list of all the heaps in the system
* @user_clients: list of all the clients created from userspace
*/
@@ -54,7 +55,8 @@ struct ion_device {
struct miscdevice dev;
struct rb_root buffers;
struct mutex buffer_lock;
- struct rw_semaphore lock;
+ struct rw_semaphore client_lock;
+ struct rw_semaphore heap_lock;
struct plist_head heaps;
long (*custom_ioctl)(struct ion_client *client, unsigned int cmd,
unsigned long arg);
@@ -146,7 +148,7 @@ static inline void ion_buffer_page_clean(struct page **page)
*page = (struct page *)((unsigned long)(*page) & ~(1UL));
}

-/* this function should only be called while dev->lock is held */
+/* this function should only be called while dev->heap_lock is held */
static void ion_buffer_add(struct ion_device *dev,
struct ion_buffer *buffer)
{
@@ -172,7 +174,7 @@ static void ion_buffer_add(struct ion_device *dev,
rb_insert_color(&buffer->node, &dev->buffers);
}

-/* this function should only be called while dev->lock is held */
+/* this function should only be called while dev->heap_lock is held */
static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
struct ion_device *dev,
unsigned long len,
@@ -511,7 +513,7 @@ struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
if (!len)
return ERR_PTR(-EINVAL);

- down_read(&dev->lock);
+ down_read(&dev->heap_lock);
plist_for_each_entry(heap, &dev->heaps, node) {
/* if the caller didn't specify this heap id */
if (!((1 << heap->id) & heap_id_mask))
@@ -520,7 +522,7 @@ struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
if (!IS_ERR(buffer))
break;
}
- up_read(&dev->lock);
+ up_read(&dev->heap_lock);

if (buffer == NULL)
return ERR_PTR(-ENODEV);
@@ -713,7 +715,7 @@ static int is_client_alive(struct ion_client *client)
node = ion_root_client->rb_node;
dev = container_of(ion_root_client, struct ion_device, clients);

- down_read(&dev->lock);
+ down_read(&dev->client_lock);
while (node) {
tmp = rb_entry(node, struct ion_client, node);
if (client < tmp) {
@@ -721,12 +723,12 @@ static int is_client_alive(struct ion_client *client)
} else if (client > tmp) {
node = node->rb_right;
} else {
- up_read(&dev->lock);
+ up_read(&dev->client_lock);
return 1;
}
}

- up_read(&dev->lock);
+ up_read(&dev->client_lock);
return 0;
}

@@ -841,12 +843,12 @@ struct ion_client *ion_client_create(struct ion_device *dev,
if (!client->name)
goto err_free_client;

- down_write(&dev->lock);
+ down_write(&dev->client_lock);
client->display_serial = ion_get_client_serial(&dev->clients, name);
client->display_name = kasprintf(
GFP_KERNEL, "%s-%d", name, client->display_serial);
if (!client->display_name) {
- up_write(&dev->lock);
+ up_write(&dev->client_lock);
goto err_free_client_name;
}
p = &dev->clients.rb_node;
@@ -873,7 +875,7 @@ struct ion_client *ion_client_create(struct ion_device *dev,
path, client->display_name);
}

- up_write(&dev->lock);
+ up_write(&dev->client_lock);

return client;

@@ -903,12 +905,12 @@ void ion_client_destroy(struct ion_client *client)

idr_destroy(&client->idr);

- down_write(&dev->lock);
+ down_write(&dev->client_lock);
if (client->task)
put_task_struct(client->task);
rb_erase(&client->node, &dev->clients);
debugfs_remove_recursive(client->debug_root);
- up_write(&dev->lock);
+ up_write(&dev->client_lock);

kfree(client->display_name);
kfree(client->name);
@@ -1603,7 +1605,7 @@ void ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
ion_heap_init_shrinker(heap);

heap->dev = dev;
- down_write(&dev->lock);
+ down_write(&dev->heap_lock);
/*
* use negative heap->id to reverse the priority -- when traversing
* the list later attempt higher id numbers first
@@ -1638,7 +1640,7 @@ void ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
}
}

- up_write(&dev->lock);
+ up_write(&dev->heap_lock);
}
EXPORT_SYMBOL(ion_device_add_heap);

@@ -1685,7 +1687,8 @@ debugfs_done:
idev->custom_ioctl = custom_ioctl;
idev->buffers = RB_ROOT;
mutex_init(&idev->buffer_lock);
- init_rwsem(&idev->lock);
+ init_rwsem(&idev->client_lock);
+ init_rwsem(&idev->heap_lock);
plist_head_init(&idev->heaps);
idev->clients = RB_ROOT;
ion_root_client = &idev->clients;
--
2.7.4