[PATCH 0/3] ION fixups for staging-next (v2)

[PATCH 0/3] ION fixups for staging-next (v2)

[John Stultz]

Since the ION patchset landed in staging-next, there have
been a few build issue reports, and this patchset tries
to address them.

Dropped the RFC, as I think the first two are good to go, but
I'd still like to get acks from Colin or someone else on the
Android team on all of these before merging these, as some bits
were a bit more complex then I'd like.

Changes in v2:
o Fixed up style issue Greg caught
o Reworked second patch to simplify error path and just return
  null if kmalloc fails
o Per Colin's suggestion, reworked third patch to use spinlocks.
  This ended up being a bit more complicated to get right, and
  I'd appreciate very close review, but it seems to be working
  ok in testing on both ARM and x86_64 w/o triggering any lockdep
  or other warnings.


I'd appreciate any thoughts or comments!

thanks
-john

Cc: Colin Cross <ccross@android.com>
Cc: Android Kernel Team <kernel-team@android.com>
Cc: Greg KH <gregkh@linuxfoundation.org>


John Stultz (3):
  staging: ion: Add HAVE_MEMBLOCK config dependency
  staging: ion: Fix possible null pointer dereference
  staging: ion: Avoid using rt_mutexes directly.

 drivers/staging/android/ion/Kconfig           |  1 +
 drivers/staging/android/ion/ion_heap.c        | 31 ++++++++++++++++-----------
 drivers/staging/android/ion/ion_priv.h        |  2 +-
 drivers/staging/android/ion/ion_system_heap.c |  7 +++++-
 4 files changed, 27 insertions(+), 14 deletions(-)

-- 
1.8.3.2

[PATCH 1/3] staging: ion: Add HAVE_MEMBLOCK config dependency

[John Stultz]

The kbuild test robot reported a build issue w/ ION on m68k:

drivers/staging/android/ion/ion.c: In function 'ion_reserve':
drivers/staging/android/ion/ion.c:1526:4: error: implicit declaration of function 'memblock_alloc_base' [-Werror=implicit-function-declaration]
drivers/staging/android/ion/ion.c:1528:11: error: 'MEMBLOCK_ALLOC_ANYWHERE' undeclared (first use in this function)
drivers/staging/android/ion/ion.c:1528:11: note: each undeclared identifier is reported only once for each function it appears in
drivers/staging/android/ion/ion.c:1537:4: error: implicit declaration of function 'memblock_reserve' [-Werror=implicit-function-declaration]
   cc1: some warnings being treated as errors

This is caused by ION using memblock functionality which m68k doesn't support.

This patch adds a HAVE_MEMBLOCK dependency to the ION config.

Cc: Colin Cross <ccross@android.com>
Cc: Android Kernel Team <kernel-team@android.com>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: kbuild test robot <fengguang.wu@intel.com>
Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/staging/android/ion/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
index b95281e..a9a64ea 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -1,5 +1,6 @@
 menuconfig ION
 	bool "Ion Memory Manager"
+	depends on HAVE_MEMBLOCK
 	select GENERIC_ALLOCATOR
 	select DMA_SHARED_BUFFER
 	---help---
-- 
1.8.3.2

[PATCH 2/3] staging: ion: Fix possible null pointer dereference

[John Stultz]

The kbuild test robot reported:

drivers/staging/android/ion/ion_system_heap.c:122 alloc_largest_available() error: potential null dereference 'info'.  (kmalloc returns null)

Where the pointer returned from kmalloc goes unchecked for failure.

This patch checks the return for NULL, and reworks the logic, as
suggested by Colin, so we allocate the page_info structure first.

Cc: Colin Cross <ccross@android.com>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Android Kernel Team <kernel-team@android.com>
Cc: kbuild test robot <fengguang.wu@intel.com>
Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/staging/android/ion/ion_system_heap.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
index 144b2272..7f07291 100644
--- a/drivers/staging/android/ion/ion_system_heap.c
+++ b/drivers/staging/android/ion/ion_system_heap.c
@@ -108,6 +108,10 @@ static struct page_info *alloc_largest_available(struct ion_system_heap *heap,
 	struct page_info *info;
 	int i;
 
+	info = kmalloc(sizeof(struct page_info), GFP_KERNEL);
+	if (!info)
+		return NULL;
+
 	for (i = 0; i < num_orders; i++) {
 		if (size < order_to_size(orders[i]))
 			continue;
@@ -118,11 +122,12 @@ static struct page_info *alloc_largest_available(struct ion_system_heap *heap,
 		if (!page)
 			continue;
 
-		info = kmalloc(sizeof(struct page_info), GFP_KERNEL);
 		info->page = page;
 		info->order = orders[i];
 		return info;
 	}
+	kfree(info);
+
 	return NULL;
 }
 
-- 
1.8.3.2

[PATCH 3/3] staging: ion: Avoid using rt_mutexes directly.

[John Stultz]

RT_MUTEXES can be configured out of the kernel, causing compile
problems with ION.

To quote Colin:
"rt_mutexes were added with the deferred freeing feature.  Heaps need
to return zeroed memory to userspace, but zeroing the memory on every
allocation was causing performance issues.  We added a SCHED_IDLE
thread to zero memory in the background after freeing, but locking the
heap from the SCHED_IDLE thread might block a high priority allocation
thread for a long time.

The lock is only used to protect the heap's free_list and
free_list_size members, and is not held for any long or sleeping
operations.  Converting to a spinlock should prevent priority
inversion without using the rt_mutex.  I'd also rename it to free_lock
to so it doesn't get used as a general heap lock."

Thus this patch converts the rt_mutex usage to a spinlock and
renames the lock free_lock to be more clear as to its use.

I also had to change a bit of logic in ion_heap_freelist_drain()
despite the for loop being a for_each_entry_safe(), I was still
seeing list corruption or buffer sg table corruption if I dropped
the lock before calling ion_buffer_destroy().

Not being able to sort out exactly why, I borrowed the loop structure
from ion_heap_deferred_free() and that works in my testing w/o issue.

Not sure if its the mixing of list traversal methods causing the issue?
Thoughts would be appreciated.

Cc: Colin Cross <ccross@android.com>
Cc: Android Kernel Team <kernel-team@android.com>
Cc: Greg KH <gregkh@linuxfoundation.org>
Reported-by: Jim Davis <jim.epost@gmail.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/staging/android/ion/ion_heap.c | 31 +++++++++++++++++++------------
 drivers/staging/android/ion/ion_priv.h |  2 +-
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/android/ion/ion_heap.c b/drivers/staging/android/ion/ion_heap.c
index 9cf5622..72fe74b 100644
--- a/drivers/staging/android/ion/ion_heap.c
+++ b/drivers/staging/android/ion/ion_heap.c
@@ -160,10 +160,10 @@ int ion_heap_pages_zero(struct page *page, size_t size, pgprot_t pgprot)
 
 void ion_heap_freelist_add(struct ion_heap *heap, struct ion_buffer *buffer)
 {
-	rt_mutex_lock(&heap->lock);
+	spin_lock(&heap->free_lock);
 	list_add(&buffer->list, &heap->free_list);
 	heap->free_list_size += buffer->size;
-	rt_mutex_unlock(&heap->lock);
+	spin_unlock(&heap->free_lock);
 	wake_up(&heap->waitqueue);
 }
 
@@ -171,34 +171,41 @@ size_t ion_heap_freelist_size(struct ion_heap *heap)
 {
 	size_t size;
 
-	rt_mutex_lock(&heap->lock);
+	spin_lock(&heap->free_lock);
 	size = heap->free_list_size;
-	rt_mutex_unlock(&heap->lock);
+	spin_unlock(&heap->free_lock);
 
 	return size;
 }
 
 size_t ion_heap_freelist_drain(struct ion_heap *heap, size_t size)
 {
-	struct ion_buffer *buffer, *tmp;
+	struct ion_buffer *buffer;
 	size_t total_drained = 0;
 
 	if (ion_heap_freelist_size(heap) == 0)
 		return 0;
 
-	rt_mutex_lock(&heap->lock);
+	spin_lock(&heap->free_lock);
 	if (size == 0)
 		size = heap->free_list_size;
 
-	list_for_each_entry_safe(buffer, tmp, &heap->free_list, list) {
+	while (true) {
 		if (total_drained >= size)
 			break;
+		if (list_empty(&heap->free_list))
+			break;
+
+		buffer = list_first_entry(&heap->free_list, struct ion_buffer,
+					  list);
 		list_del(&buffer->list);
 		heap->free_list_size -= buffer->size;
 		total_drained += buffer->size;
+		spin_unlock(&heap->free_lock);
 		ion_buffer_destroy(buffer);
+		spin_lock(&heap->free_lock);
 	}
-	rt_mutex_unlock(&heap->lock);
+	spin_unlock(&heap->free_lock);
 
 	return total_drained;
 }
@@ -213,16 +220,16 @@ static int ion_heap_deferred_free(void *data)
 		wait_event_freezable(heap->waitqueue,
 				     ion_heap_freelist_size(heap) > 0);
 
-		rt_mutex_lock(&heap->lock);
+		spin_lock(&heap->free_lock);
 		if (list_empty(&heap->free_list)) {
-			rt_mutex_unlock(&heap->lock);
+			spin_unlock(&heap->free_lock);
 			continue;
 		}
 		buffer = list_first_entry(&heap->free_list, struct ion_buffer,
 					  list);
 		list_del(&buffer->list);
 		heap->free_list_size -= buffer->size;
-		rt_mutex_unlock(&heap->lock);
+		spin_unlock(&heap->free_lock);
 		ion_buffer_destroy(buffer);
 	}
 
@@ -235,7 +242,7 @@ int ion_heap_init_deferred_free(struct ion_heap *heap)
 
 	INIT_LIST_HEAD(&heap->free_list);
 	heap->free_list_size = 0;
-	rt_mutex_init(&heap->lock);
+	spin_lock_init(&heap->free_lock);
 	init_waitqueue_head(&heap->waitqueue);
 	heap->task = kthread_run(ion_heap_deferred_free, heap,
 				 "%s", heap->name);
diff --git a/drivers/staging/android/ion/ion_priv.h b/drivers/staging/android/ion/ion_priv.h
index fc8a4c3..d986739 100644
--- a/drivers/staging/android/ion/ion_priv.h
+++ b/drivers/staging/android/ion/ion_priv.h
@@ -159,7 +159,7 @@ struct ion_heap {
 	struct shrinker shrinker;
 	struct list_head free_list;
 	size_t free_list_size;
-	struct rt_mutex lock;
+	spinlock_t free_lock;
 	wait_queue_head_t waitqueue;
 	struct task_struct *task;
 	int (*debug_show)(struct ion_heap *heap, struct seq_file *, void *);
-- 
1.8.3.2

Re: [PATCH 3/3] staging: ion: Avoid using rt_mutexes directly.

[Colin Cross]

On Mon, Dec 16, 2013 at 9:07 PM, John Stultz <john.stultz@linaro.org> wrote:
> RT_MUTEXES can be configured out of the kernel, causing compile
> problems with ION.
>
> To quote Colin:
> "rt_mutexes were added with the deferred freeing feature.  Heaps need
> to return zeroed memory to userspace, but zeroing the memory on every
> allocation was causing performance issues.  We added a SCHED_IDLE
> thread to zero memory in the background after freeing, but locking the
> heap from the SCHED_IDLE thread might block a high priority allocation
> thread for a long time.
>
> The lock is only used to protect the heap's free_list and
> free_list_size members, and is not held for any long or sleeping
> operations.  Converting to a spinlock should prevent priority
> inversion without using the rt_mutex.  I'd also rename it to free_lock
> to so it doesn't get used as a general heap lock."
>
> Thus this patch converts the rt_mutex usage to a spinlock and
> renames the lock free_lock to be more clear as to its use.
>
> I also had to change a bit of logic in ion_heap_freelist_drain()
> despite the for loop being a for_each_entry_safe(), I was still
> seeing list corruption or buffer sg table corruption if I dropped
> the lock before calling ion_buffer_destroy().
>
> Not being able to sort out exactly why, I borrowed the loop structure
> from ion_heap_deferred_free() and that works in my testing w/o issue.
>
> Not sure if its the mixing of list traversal methods causing the issue?
> Thoughts would be appreciated.

list_for_each_entry_safe just stores the next pointer to allow
deleting the current pointer, but that isn't safe if you follow my
suggestion to drop the lock because the next pointer could also become
invalid, so the while loop is necessary.

> Cc: Colin Cross <ccross@android.com>
> Cc: Android Kernel Team <kernel-team@android.com>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Reported-by: Jim Davis <jim.epost@gmail.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/staging/android/ion/ion_heap.c | 31 +++++++++++++++++++------------
>  drivers/staging/android/ion/ion_priv.h |  2 +-
>  2 files changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion_heap.c b/drivers/staging/android/ion/ion_heap.c
> index 9cf5622..72fe74b 100644
> --- a/drivers/staging/android/ion/ion_heap.c
> +++ b/drivers/staging/android/ion/ion_heap.c
> @@ -160,10 +160,10 @@ int ion_heap_pages_zero(struct page *page, size_t size, pgprot_t pgprot)
>
>  void ion_heap_freelist_add(struct ion_heap *heap, struct ion_buffer *buffer)
>  {
> -       rt_mutex_lock(&heap->lock);
> +       spin_lock(&heap->free_lock);
>         list_add(&buffer->list, &heap->free_list);
>         heap->free_list_size += buffer->size;
> -       rt_mutex_unlock(&heap->lock);
> +       spin_unlock(&heap->free_lock);
>         wake_up(&heap->waitqueue);
>  }
>
> @@ -171,34 +171,41 @@ size_t ion_heap_freelist_size(struct ion_heap *heap)
>  {
>         size_t size;
>
> -       rt_mutex_lock(&heap->lock);
> +       spin_lock(&heap->free_lock);
>         size = heap->free_list_size;
> -       rt_mutex_unlock(&heap->lock);
> +       spin_unlock(&heap->free_lock);
>
>         return size;
>  }
>
>  size_t ion_heap_freelist_drain(struct ion_heap *heap, size_t size)
>  {
> -       struct ion_buffer *buffer, *tmp;
> +       struct ion_buffer *buffer;
>         size_t total_drained = 0;
>
>         if (ion_heap_freelist_size(heap) == 0)
>                 return 0;
>
> -       rt_mutex_lock(&heap->lock);
> +       spin_lock(&heap->free_lock);
>         if (size == 0)
>                 size = heap->free_list_size;
>
> -       list_for_each_entry_safe(buffer, tmp, &heap->free_list, list) {
> +       while (true) {
I'd use while (!list_empty(&heap->free_list)), it makes it clearer
what the loop is for.

Re: [PATCH 1/3] staging: ion: Add HAVE_MEMBLOCK config dependency

[Colin Cross]

Acked-by: Colin Cross <ccross@android.com>

On Mon, Dec 16, 2013 at 9:07 PM, John Stultz <john.stultz@linaro.org> wrote:
> The kbuild test robot reported a build issue w/ ION on m68k:
>
> drivers/staging/android/ion/ion.c: In function 'ion_reserve':
> drivers/staging/android/ion/ion.c:1526:4: error: implicit declaration of function 'memblock_alloc_base' [-Werror=implicit-function-declaration]
> drivers/staging/android/ion/ion.c:1528:11: error: 'MEMBLOCK_ALLOC_ANYWHERE' undeclared (first use in this function)
> drivers/staging/android/ion/ion.c:1528:11: note: each undeclared identifier is reported only once for each function it appears in
> drivers/staging/android/ion/ion.c:1537:4: error: implicit declaration of function 'memblock_reserve' [-Werror=implicit-function-declaration]
>    cc1: some warnings being treated as errors
>
> This is caused by ION using memblock functionality which m68k doesn't support.
>
> This patch adds a HAVE_MEMBLOCK dependency to the ION config.
>
> Cc: Colin Cross <ccross@android.com>
> Cc: Android Kernel Team <kernel-team@android.com>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Cc: kbuild test robot <fengguang.wu@intel.com>
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/staging/android/ion/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
> index b95281e..a9a64ea 100644
> --- a/drivers/staging/android/ion/Kconfig
> +++ b/drivers/staging/android/ion/Kconfig
> @@ -1,5 +1,6 @@
>  menuconfig ION
>         bool "Ion Memory Manager"
> +       depends on HAVE_MEMBLOCK
>         select GENERIC_ALLOCATOR
>         select DMA_SHARED_BUFFER
>         ---help---
> --
> 1.8.3.2
>
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.

Re: [PATCH 2/3] staging: ion: Fix possible null pointer dereference

[Colin Cross]

On Mon, Dec 16, 2013 at 9:07 PM, John Stultz <john.stultz@linaro.org> wrote:
> The kbuild test robot reported:
>
> drivers/staging/android/ion/ion_system_heap.c:122 alloc_largest_available() error: potential null dereference 'info'.  (kmalloc returns null)
>
> Where the pointer returned from kmalloc goes unchecked for failure.
>
> This patch checks the return for NULL, and reworks the logic, as
> suggested by Colin, so we allocate the page_info structure first.
>
> Cc: Colin Cross <ccross@android.com>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Cc: Android Kernel Team <kernel-team@android.com>
> Cc: kbuild test robot <fengguang.wu@intel.com>
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/staging/android/ion/ion_system_heap.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
> index 144b2272..7f07291 100644
> --- a/drivers/staging/android/ion/ion_system_heap.c
> +++ b/drivers/staging/android/ion/ion_system_heap.c
> @@ -108,6 +108,10 @@ static struct page_info *alloc_largest_available(struct ion_system_heap *heap,
>         struct page_info *info;
>         int i;
>
> +       info = kmalloc(sizeof(struct page_info), GFP_KERNEL);
> +       if (!info)
> +               return NULL;
> +
>         for (i = 0; i < num_orders; i++) {
>                 if (size < order_to_size(orders[i]))
>                         continue;
> @@ -118,11 +122,12 @@ static struct page_info *alloc_largest_available(struct ion_system_heap *heap,
>                 if (!page)
>                         continue;
>
> -               info = kmalloc(sizeof(struct page_info), GFP_KERNEL);
>                 info->page = page;
>                 info->order = orders[i];
>                 return info;
>         }
> +       kfree(info);
> +
>         return NULL;
>  }
>

Acked-by: Colin Cross <ccross@android.com>

[PATCH v3] staging: ion: Avoid using rt_mutexes directly

[John Stultz]

RT_MUTEXES can be configured out of the kernel, causing compile
problems with ION.

To quote Colin:
"rt_mutexes were added with the deferred freeing feature.  Heaps need
to return zeroed memory to userspace, but zeroing the memory on every
allocation was causing performance issues.  We added a SCHED_IDLE
thread to zero memory in the background after freeing, but locking the
heap from the SCHED_IDLE thread might block a high priority allocation
thread for a long time.

The lock is only used to protect the heap's free_list and
free_list_size members, and is not held for any long or sleeping
operations.  Converting to a spinlock should prevent priority
inversion without using the rt_mutex.  I'd also rename it to free_lock
to so it doesn't get used as a general heap lock."

Thus this patch converts the rt_mutex usage to a spinlock and
renames the lock free_lock to be more clear as to its use.

I also had to change a bit of logic in ion_heap_freelist_drain()
to safely avoid list corruption.

Cc: Colin Cross <ccross@android.com>
Cc: Android Kernel Team <kernel-team@android.com>
Cc: Greg KH <gregkh@linuxfoundation.org>
Reported-by: Jim Davis <jim.epost@gmail.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---

Changes from v2:
- Modified while loop to be more clear, per Colin's suggestion


 drivers/staging/android/ion/ion_heap.c | 28 ++++++++++++++++------------
 drivers/staging/android/ion/ion_priv.h |  2 +-
 2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/android/ion/ion_heap.c b/drivers/staging/android/ion/ion_heap.c
index 9cf5622..296c74f 100644
--- a/drivers/staging/android/ion/ion_heap.c
+++ b/drivers/staging/android/ion/ion_heap.c
@@ -160,10 +160,10 @@ int ion_heap_pages_zero(struct page *page, size_t size, pgprot_t pgprot)
 
 void ion_heap_freelist_add(struct ion_heap *heap, struct ion_buffer *buffer)
 {
-	rt_mutex_lock(&heap->lock);
+	spin_lock(&heap->free_lock);
 	list_add(&buffer->list, &heap->free_list);
 	heap->free_list_size += buffer->size;
-	rt_mutex_unlock(&heap->lock);
+	spin_unlock(&heap->free_lock);
 	wake_up(&heap->waitqueue);
 }
 
@@ -171,34 +171,38 @@ size_t ion_heap_freelist_size(struct ion_heap *heap)
 {
 	size_t size;
 
-	rt_mutex_lock(&heap->lock);
+	spin_lock(&heap->free_lock);
 	size = heap->free_list_size;
-	rt_mutex_unlock(&heap->lock);
+	spin_unlock(&heap->free_lock);
 
 	return size;
 }
 
 size_t ion_heap_freelist_drain(struct ion_heap *heap, size_t size)
 {
-	struct ion_buffer *buffer, *tmp;
+	struct ion_buffer *buffer;
 	size_t total_drained = 0;
 
 	if (ion_heap_freelist_size(heap) == 0)
 		return 0;
 
-	rt_mutex_lock(&heap->lock);
+	spin_lock(&heap->free_lock);
 	if (size == 0)
 		size = heap->free_list_size;
 
-	list_for_each_entry_safe(buffer, tmp, &heap->free_list, list) {
+	while (!list_empty(&heap->free_list)) {
 		if (total_drained >= size)
 			break;
+		buffer = list_first_entry(&heap->free_list, struct ion_buffer,
+					  list);
 		list_del(&buffer->list);
 		heap->free_list_size -= buffer->size;
 		total_drained += buffer->size;
+		spin_unlock(&heap->free_lock);
 		ion_buffer_destroy(buffer);
+		spin_lock(&heap->free_lock);
 	}
-	rt_mutex_unlock(&heap->lock);
+	spin_unlock(&heap->free_lock);
 
 	return total_drained;
 }
@@ -213,16 +217,16 @@ static int ion_heap_deferred_free(void *data)
 		wait_event_freezable(heap->waitqueue,
 				     ion_heap_freelist_size(heap) > 0);
 
-		rt_mutex_lock(&heap->lock);
+		spin_lock(&heap->free_lock);
 		if (list_empty(&heap->free_list)) {
-			rt_mutex_unlock(&heap->lock);
+			spin_unlock(&heap->free_lock);
 			continue;
 		}
 		buffer = list_first_entry(&heap->free_list, struct ion_buffer,
 					  list);
 		list_del(&buffer->list);
 		heap->free_list_size -= buffer->size;
-		rt_mutex_unlock(&heap->lock);
+		spin_unlock(&heap->free_lock);
 		ion_buffer_destroy(buffer);
 	}
 
@@ -235,7 +239,7 @@ int ion_heap_init_deferred_free(struct ion_heap *heap)
 
 	INIT_LIST_HEAD(&heap->free_list);
 	heap->free_list_size = 0;
-	rt_mutex_init(&heap->lock);
+	spin_lock_init(&heap->free_lock);
 	init_waitqueue_head(&heap->waitqueue);
 	heap->task = kthread_run(ion_heap_deferred_free, heap,
 				 "%s", heap->name);
diff --git a/drivers/staging/android/ion/ion_priv.h b/drivers/staging/android/ion/ion_priv.h
index fc8a4c3..d986739 100644
--- a/drivers/staging/android/ion/ion_priv.h
+++ b/drivers/staging/android/ion/ion_priv.h
@@ -159,7 +159,7 @@ struct ion_heap {
 	struct shrinker shrinker;
 	struct list_head free_list;
 	size_t free_list_size;
-	struct rt_mutex lock;
+	spinlock_t free_lock;
 	wait_queue_head_t waitqueue;
 	struct task_struct *task;
 	int (*debug_show)(struct ion_heap *heap, struct seq_file *, void *);
-- 
1.8.3.2

Re: [PATCH v3] staging: ion: Avoid using rt_mutexes directly

[Colin Cross]

On Tue, Dec 17, 2013 at 5:04 PM, John Stultz <john.stultz@linaro.org> wrote:
> RT_MUTEXES can be configured out of the kernel, causing compile
> problems with ION.
>
> To quote Colin:
> "rt_mutexes were added with the deferred freeing feature.  Heaps need
> to return zeroed memory to userspace, but zeroing the memory on every
> allocation was causing performance issues.  We added a SCHED_IDLE
> thread to zero memory in the background after freeing, but locking the
> heap from the SCHED_IDLE thread might block a high priority allocation
> thread for a long time.
>
> The lock is only used to protect the heap's free_list and
> free_list_size members, and is not held for any long or sleeping
> operations.  Converting to a spinlock should prevent priority
> inversion without using the rt_mutex.  I'd also rename it to free_lock
> to so it doesn't get used as a general heap lock."
>
> Thus this patch converts the rt_mutex usage to a spinlock and
> renames the lock free_lock to be more clear as to its use.
>
> I also had to change a bit of logic in ion_heap_freelist_drain()
> to safely avoid list corruption.
>
> Cc: Colin Cross <ccross@android.com>
> Cc: Android Kernel Team <kernel-team@android.com>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Reported-by: Jim Davis <jim.epost@gmail.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>
> Changes from v2:
> - Modified while loop to be more clear, per Colin's suggestion
>
>
>  drivers/staging/android/ion/ion_heap.c | 28 ++++++++++++++++------------
>  drivers/staging/android/ion/ion_priv.h |  2 +-
>  2 files changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion_heap.c b/drivers/staging/android/ion/ion_heap.c
> index 9cf5622..296c74f 100644
> --- a/drivers/staging/android/ion/ion_heap.c
> +++ b/drivers/staging/android/ion/ion_heap.c
> @@ -160,10 +160,10 @@ int ion_heap_pages_zero(struct page *page, size_t size, pgprot_t pgprot)
>
>  void ion_heap_freelist_add(struct ion_heap *heap, struct ion_buffer *buffer)
>  {
> -       rt_mutex_lock(&heap->lock);
> +       spin_lock(&heap->free_lock);
>         list_add(&buffer->list, &heap->free_list);
>         heap->free_list_size += buffer->size;
> -       rt_mutex_unlock(&heap->lock);
> +       spin_unlock(&heap->free_lock);
>         wake_up(&heap->waitqueue);
>  }
>
> @@ -171,34 +171,38 @@ size_t ion_heap_freelist_size(struct ion_heap *heap)
>  {
>         size_t size;
>
> -       rt_mutex_lock(&heap->lock);
> +       spin_lock(&heap->free_lock);
>         size = heap->free_list_size;
> -       rt_mutex_unlock(&heap->lock);
> +       spin_unlock(&heap->free_lock);
>
>         return size;
>  }
>
>  size_t ion_heap_freelist_drain(struct ion_heap *heap, size_t size)
>  {
> -       struct ion_buffer *buffer, *tmp;
> +       struct ion_buffer *buffer;
>         size_t total_drained = 0;
>
>         if (ion_heap_freelist_size(heap) == 0)
>                 return 0;
>
> -       rt_mutex_lock(&heap->lock);
> +       spin_lock(&heap->free_lock);
>         if (size == 0)
>                 size = heap->free_list_size;
>
> -       list_for_each_entry_safe(buffer, tmp, &heap->free_list, list) {
> +       while (!list_empty(&heap->free_list)) {
>                 if (total_drained >= size)
>                         break;
> +               buffer = list_first_entry(&heap->free_list, struct ion_buffer,
> +                                         list);
>                 list_del(&buffer->list);
>                 heap->free_list_size -= buffer->size;
>                 total_drained += buffer->size;
> +               spin_unlock(&heap->free_lock);
>                 ion_buffer_destroy(buffer);
> +               spin_lock(&heap->free_lock);
>         }
> -       rt_mutex_unlock(&heap->lock);
> +       spin_unlock(&heap->free_lock);
>
>         return total_drained;
>  }
> @@ -213,16 +217,16 @@ static int ion_heap_deferred_free(void *data)
>                 wait_event_freezable(heap->waitqueue,
>                                      ion_heap_freelist_size(heap) > 0);
>
> -               rt_mutex_lock(&heap->lock);
> +               spin_lock(&heap->free_lock);
>                 if (list_empty(&heap->free_list)) {
> -                       rt_mutex_unlock(&heap->lock);
> +                       spin_unlock(&heap->free_lock);
>                         continue;
>                 }
>                 buffer = list_first_entry(&heap->free_list, struct ion_buffer,
>                                           list);
>                 list_del(&buffer->list);
>                 heap->free_list_size -= buffer->size;
> -               rt_mutex_unlock(&heap->lock);
> +               spin_unlock(&heap->free_lock);
>                 ion_buffer_destroy(buffer);
>         }
>
> @@ -235,7 +239,7 @@ int ion_heap_init_deferred_free(struct ion_heap *heap)
>
>         INIT_LIST_HEAD(&heap->free_list);
>         heap->free_list_size = 0;
> -       rt_mutex_init(&heap->lock);
> +       spin_lock_init(&heap->free_lock);
>         init_waitqueue_head(&heap->waitqueue);
>         heap->task = kthread_run(ion_heap_deferred_free, heap,
>                                  "%s", heap->name);
> diff --git a/drivers/staging/android/ion/ion_priv.h b/drivers/staging/android/ion/ion_priv.h
> index fc8a4c3..d986739 100644
> --- a/drivers/staging/android/ion/ion_priv.h
> +++ b/drivers/staging/android/ion/ion_priv.h
> @@ -159,7 +159,7 @@ struct ion_heap {
>         struct shrinker shrinker;
>         struct list_head free_list;
>         size_t free_list_size;
> -       struct rt_mutex lock;
> +       spinlock_t free_lock;
>         wait_queue_head_t waitqueue;
>         struct task_struct *task;
>         int (*debug_show)(struct ion_heap *heap, struct seq_file *, void *);
> --
> 1.8.3.2
>

Acked-by: Colin Cross <ccross@android.com>