* [PATCH 0/3] ION fixups for staging-next (v2)
@ 2013-12-17 5:07 John Stultz
2013-12-17 5:07 ` [PATCH 1/3] staging: ion: Add HAVE_MEMBLOCK config dependency John Stultz
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: John Stultz @ 2013-12-17 5:07 UTC (permalink / raw)
To: LKML; +Cc: John Stultz, Colin Cross, Android Kernel Team, Greg KH
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/3] staging: ion: Add HAVE_MEMBLOCK config dependency 2013-12-17 5:07 [PATCH 0/3] ION fixups for staging-next (v2) John Stultz @ 2013-12-17 5:07 ` John Stultz 2013-12-17 7:25 ` Colin Cross 2013-12-17 5:07 ` [PATCH 2/3] staging: ion: Fix possible null pointer dereference John Stultz 2013-12-17 5:07 ` [PATCH 3/3] staging: ion: Avoid using rt_mutexes directly John Stultz 2 siblings, 1 reply; 9+ messages in thread From: John Stultz @ 2013-12-17 5:07 UTC (permalink / raw) To: LKML Cc: John Stultz, Colin Cross, Android Kernel Team, Greg KH, kbuild test robot 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] staging: ion: Add HAVE_MEMBLOCK config dependency 2013-12-17 5:07 ` [PATCH 1/3] staging: ion: Add HAVE_MEMBLOCK config dependency John Stultz @ 2013-12-17 7:25 ` Colin Cross 0 siblings, 0 replies; 9+ messages in thread From: Colin Cross @ 2013-12-17 7:25 UTC (permalink / raw) To: John Stultz; +Cc: LKML, Android Kernel Team, Greg KH, kbuild test robot 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] staging: ion: Fix possible null pointer dereference 2013-12-17 5:07 [PATCH 0/3] ION fixups for staging-next (v2) John Stultz 2013-12-17 5:07 ` [PATCH 1/3] staging: ion: Add HAVE_MEMBLOCK config dependency John Stultz @ 2013-12-17 5:07 ` John Stultz 2013-12-17 7:26 ` Colin Cross 2013-12-17 5:07 ` [PATCH 3/3] staging: ion: Avoid using rt_mutexes directly John Stultz 2 siblings, 1 reply; 9+ messages in thread From: John Stultz @ 2013-12-17 5:07 UTC (permalink / raw) To: LKML Cc: John Stultz, Colin Cross, Greg KH, Android Kernel Team, kbuild test robot 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] staging: ion: Fix possible null pointer dereference 2013-12-17 5:07 ` [PATCH 2/3] staging: ion: Fix possible null pointer dereference John Stultz @ 2013-12-17 7:26 ` Colin Cross 0 siblings, 0 replies; 9+ messages in thread From: Colin Cross @ 2013-12-17 7:26 UTC (permalink / raw) To: John Stultz; +Cc: LKML, Greg KH, Android Kernel Team, kbuild test robot 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> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] staging: ion: Avoid using rt_mutexes directly. 2013-12-17 5:07 [PATCH 0/3] ION fixups for staging-next (v2) John Stultz 2013-12-17 5:07 ` [PATCH 1/3] staging: ion: Add HAVE_MEMBLOCK config dependency John Stultz 2013-12-17 5:07 ` [PATCH 2/3] staging: ion: Fix possible null pointer dereference John Stultz @ 2013-12-17 5:07 ` John Stultz 2013-12-17 7:25 ` Colin Cross 2 siblings, 1 reply; 9+ messages in thread From: John Stultz @ 2013-12-17 5:07 UTC (permalink / raw) To: LKML; +Cc: John Stultz, Colin Cross, Android Kernel Team, Greg KH 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] staging: ion: Avoid using rt_mutexes directly. 2013-12-17 5:07 ` [PATCH 3/3] staging: ion: Avoid using rt_mutexes directly John Stultz @ 2013-12-17 7:25 ` Colin Cross 2013-12-18 1:04 ` [PATCH v3] " John Stultz 0 siblings, 1 reply; 9+ messages in thread From: Colin Cross @ 2013-12-17 7:25 UTC (permalink / raw) To: John Stultz; +Cc: LKML, Android Kernel Team, Greg KH 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] staging: ion: Avoid using rt_mutexes directly 2013-12-17 7:25 ` Colin Cross @ 2013-12-18 1:04 ` John Stultz 2013-12-18 1:07 ` Colin Cross 0 siblings, 1 reply; 9+ messages in thread From: John Stultz @ 2013-12-18 1:04 UTC (permalink / raw) To: LKML; +Cc: John Stultz, Colin Cross, Android Kernel Team, Greg KH 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] staging: ion: Avoid using rt_mutexes directly 2013-12-18 1:04 ` [PATCH v3] " John Stultz @ 2013-12-18 1:07 ` Colin Cross 0 siblings, 0 replies; 9+ messages in thread From: Colin Cross @ 2013-12-18 1:07 UTC (permalink / raw) To: John Stultz; +Cc: LKML, Android Kernel Team, Greg KH 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> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-12-18 1:07 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-12-17 5:07 [PATCH 0/3] ION fixups for staging-next (v2) John Stultz 2013-12-17 5:07 ` [PATCH 1/3] staging: ion: Add HAVE_MEMBLOCK config dependency John Stultz 2013-12-17 7:25 ` Colin Cross 2013-12-17 5:07 ` [PATCH 2/3] staging: ion: Fix possible null pointer dereference John Stultz 2013-12-17 7:26 ` Colin Cross 2013-12-17 5:07 ` [PATCH 3/3] staging: ion: Avoid using rt_mutexes directly John Stultz 2013-12-17 7:25 ` Colin Cross 2013-12-18 1:04 ` [PATCH v3] " John Stultz 2013-12-18 1:07 ` Colin Cross
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.