Hi John, Just a couple nits, otherwise looks sane to me.
On Thu, Dec 17, 2020 at 3:06 PM John Stultz john.stultz@linaro.org wrote:
This patch provides infrastructure for deferring buffer frees.
This is a feature ION provided which when used with some form of a page pool, provides a nice performance boost in an allocation microbenchmark. The reason it helps is it allows the page-zeroing to be done out of the normal allocation/free path, and pushed off to a kthread.
I suggest adding some more description for this API and how it can be used. IIUC there are 2 uses: lazy deferred freeing using kthread, and object pooling. no_pool parameter I think deserves some explanation (disallows pooling when system is under memory pressure).
As not all heaps will find this useful, its implemented as a optional helper library that heaps can utilize.
Cc: Sumit Semwal sumit.semwal@linaro.org Cc: Liam Mark lmark@codeaurora.org Cc: Chris Goldsworthy cgoldswo@codeaurora.org Cc: Laura Abbott labbott@kernel.org Cc: Brian Starkey Brian.Starkey@arm.com Cc: Hridya Valsaraju hridya@google.com Cc: Suren Baghdasaryan surenb@google.com Cc: Sandeep Patil sspatil@google.com Cc: Daniel Mentz danielmentz@google.com Cc: Ørjan Eide orjan.eide@arm.com Cc: Robin Murphy robin.murphy@arm.com Cc: Ezequiel Garcia ezequiel@collabora.com Cc: Simon Ser contact@emersion.fr Cc: James Jones jajones@nvidia.com Cc: linux-media@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Signed-off-by: John Stultz john.stultz@linaro.org
drivers/dma-buf/heaps/Kconfig | 3 + drivers/dma-buf/heaps/Makefile | 1 + drivers/dma-buf/heaps/deferred-free-helper.c | 136 +++++++++++++++++++ drivers/dma-buf/heaps/deferred-free-helper.h | 15 ++ 4 files changed, 155 insertions(+) create mode 100644 drivers/dma-buf/heaps/deferred-free-helper.c create mode 100644 drivers/dma-buf/heaps/deferred-free-helper.h
diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig index a5eef06c4226..ecf65204f714 100644 --- a/drivers/dma-buf/heaps/Kconfig +++ b/drivers/dma-buf/heaps/Kconfig @@ -1,3 +1,6 @@ +config DMABUF_HEAPS_DEFERRED_FREE
boolconfig DMABUF_HEAPS_SYSTEM bool "DMA-BUF System Heap" depends on DMABUF_HEAPS diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile index 974467791032..4e7839875615 100644 --- a/drivers/dma-buf/heaps/Makefile +++ b/drivers/dma-buf/heaps/Makefile @@ -1,3 +1,4 @@ # SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_DMABUF_HEAPS_DEFERRED_FREE) += deferred-free-helper.o obj-$(CONFIG_DMABUF_HEAPS_SYSTEM) += system_heap.o obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o diff --git a/drivers/dma-buf/heaps/deferred-free-helper.c b/drivers/dma-buf/heaps/deferred-free-helper.c new file mode 100644 index 000000000000..b8f54860454f --- /dev/null +++ b/drivers/dma-buf/heaps/deferred-free-helper.c @@ -0,0 +1,136 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Deferred dmabuf freeing helper
- Copyright (C) 2020 Linaro, Ltd.
- Based on the ION page pool code
- Copyright (C) 2011 Google, Inc.
- */
+#include <linux/freezer.h> +#include <linux/list.h> +#include <linux/slab.h> +#include <linux/swap.h> +#include <linux/sched/signal.h>
+#include "deferred-free-helper.h"
+static LIST_HEAD(free_list); +static size_t list_size; +wait_queue_head_t freelist_waitqueue; +struct task_struct *freelist_task; +static DEFINE_MUTEX(free_list_lock);
+enum {
USE_POOL = 0,SKIP_POOL = 1,+};
This enum is used for a bool parameter. Either make it part of the public API or eliminate and use bool instead.
+void deferred_free(struct deferred_freelist_item *item,
void (*free)(struct deferred_freelist_item*, bool),size_t size)+{
INIT_LIST_HEAD(&item->list);item->size = size;item->free = free;mutex_lock(&free_list_lock);list_add(&item->list, &free_list);list_size += size;mutex_unlock(&free_list_lock);wake_up(&freelist_waitqueue);+}
+static size_t free_one_item(bool nopool) +{
size_t size = 0;struct deferred_freelist_item *item;mutex_lock(&free_list_lock);if (list_empty(&free_list)) {mutex_unlock(&free_list_lock);return 0;}item = list_first_entry(&free_list, struct deferred_freelist_item, list);list_del(&item->list);size = item->size;list_size -= size;mutex_unlock(&free_list_lock);item->free(item, nopool);return size;+}
+static unsigned long get_freelist_size(void) +{
unsigned long size;mutex_lock(&free_list_lock);size = list_size;mutex_unlock(&free_list_lock);return size;+}
+static unsigned long freelist_shrink_count(struct shrinker *shrinker,
struct shrink_control *sc)+{
return get_freelist_size();+}
+static unsigned long freelist_shrink_scan(struct shrinker *shrinker,
struct shrink_control *sc)+{
int total_freed = 0;if (sc->nr_to_scan == 0)return 0;while (total_freed < sc->nr_to_scan) {int freed = free_one_item(SKIP_POOL);if (!freed)break;total_freed += freed;}return total_freed;+}
+static struct shrinker freelist_shrinker = {
.count_objects = freelist_shrink_count,.scan_objects = freelist_shrink_scan,.seeks = DEFAULT_SEEKS,.batch = 0,+};
+static int deferred_free_thread(void *data) +{
while (true) {wait_event_freezable(freelist_waitqueue,get_freelist_size() > 0);free_one_item(USE_POOL);}return 0;+}
+static int deferred_freelist_init(void) +{
list_size = 0;init_waitqueue_head(&freelist_waitqueue);freelist_task = kthread_run(deferred_free_thread, NULL,"%s", "dmabuf-deferred-free-worker");if (IS_ERR(freelist_task)) {pr_err("%s: creating thread for deferred free failed\n",__func__);return -1;}sched_set_normal(freelist_task, 19);return register_shrinker(&freelist_shrinker);+} +device_initcall(deferred_freelist_init); diff --git a/drivers/dma-buf/heaps/deferred-free-helper.h b/drivers/dma-buf/heaps/deferred-free-helper.h new file mode 100644 index 000000000000..09a2274a897c --- /dev/null +++ b/drivers/dma-buf/heaps/deferred-free-helper.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef DEFERRED_FREE_HELPER_H +#define DEFERRED_FREE_HELPER_H
+struct deferred_freelist_item {
size_t size;void (*free)(struct deferred_freelist_item *i, bool no_pool);struct list_head list;+};
I assume deferred_freelist_item gets embedded in a structure that is actually being freed?
+void deferred_free(struct deferred_freelist_item *item,
void (*free)(struct deferred_freelist_item *i, bool no_pool),size_t size);+#endif
2.17.1