* [PATCH v8 0/5] Generic page pool & deferred freeing for system dmabuf heap
@ 2021-03-04 23:20 ` John Stultz
0 siblings, 0 replies; 24+ messages in thread
From: John Stultz @ 2021-03-04 23:20 UTC (permalink / raw)
To: lkml
Cc: dri-devel, Sandeep Patil, Ezequiel Garcia, Robin Murphy,
James Jones, Liam Mark, Laura Abbott, Chris Goldsworthy,
Hridya Valsaraju, Ørjan Eide, linux-media,
Suren Baghdasaryan, Christian Koenig, Daniel Mentz
Apologies for letting so much time pass since the last revision!
The point of this series is trying to add both deferred-freeing
logic as well as a page pool to the DMA-BUF system heap to improve
allocation performance.
This is desired, as the combination of deferred freeing along
with the page pool allows us to offload page-zeroing out of
the allocation hot path. This was done originally with ION
and this patch series allows the DMA-BUF system heap to match
ION's system heap allocation performance in a simple
microbenchmark [1] (ION re-added to the kernel for comparision,
running on an x86 vm image):
./dmabuf-heap-bench -i 0 1 system
Testing dmabuf system vs ion heaptype 0 (flags: 0x1)
---------------------------------------------
dmabuf heap: alloc 4096 bytes 5000 times in 88092722 ns 17618 ns/call
ion heap: alloc 4096 bytes 5000 times in 103043547 ns 20608 ns/call
dmabuf heap: alloc 1048576 bytes 5000 times in 252416639 ns 50483 ns/call
ion heap: alloc 1048576 bytes 5000 times in 358190744 ns 71638 ns/call
dmabuf heap: alloc 8388608 bytes 5000 times in 2854351310 ns 570870 ns/call
ion heap: alloc 8388608 bytes 5000 times in 3676328905 ns 735265 ns/call
dmabuf heap: alloc 33554432 bytes 5000 times in 13208119197 ns 2641623 ns/call
ion heap: alloc 33554432 bytes 5000 times in 15306975287 ns 3061395 ns/call
Daniel didn't like earlier attempts to re-use the network
page-pool code to achieve this, and suggested the ttm_pool be
used instead, so this series pulls the page pool functionality
out of the ttm_pool logic and creates a generic page pool
that can be shared.
New in v7 (never submitted):
* Reworked how I integrated the page pool with the ttm logic
to use container of to avoid allocating structures per page.
New in v8:
* Due to the dual license requirement from Christian König
I completely threw away the earlier shared page pool
implementation (which had evolved from ion code), and
rewrote it using just the ttm_pool logic. My apologies
for any previously reviewed issues that I've reintroduced
in doing so.
Input would be greatly appreciated. Testing as well, as I don't
have any development hardware that utilizes the ttm pool.
thanks
-john
[1] https://android.googlesource.com/platform/system/memory/libdmabufheap/+/refs/heads/master/tests/dmabuf_heap_bench.c
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Christian Koenig <christian.koenig@amd.com>
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
John Stultz (5):
drm: Add a sharable drm page-pool implementation
drm: ttm_pool: Rework ttm_pool to use drm_page_pool
dma-buf: heaps: Add deferred-free-helper library code
dma-buf: system_heap: Add drm pagepool support to system heap
dma-buf: system_heap: Add deferred freeing to the system heap
drivers/dma-buf/heaps/Kconfig | 5 +
drivers/dma-buf/heaps/Makefile | 1 +
drivers/dma-buf/heaps/deferred-free-helper.c | 138 ++++++++++++
drivers/dma-buf/heaps/deferred-free-helper.h | 55 +++++
drivers/dma-buf/heaps/system_heap.c | 47 +++-
drivers/gpu/drm/Kconfig | 5 +
drivers/gpu/drm/Makefile | 2 +
drivers/gpu/drm/page_pool.c | 214 +++++++++++++++++++
drivers/gpu/drm/ttm/ttm_pool.c | 156 +++-----------
include/drm/page_pool.h | 65 ++++++
include/drm/ttm/ttm_pool.h | 6 +-
11 files changed, 557 insertions(+), 137 deletions(-)
create mode 100644 drivers/dma-buf/heaps/deferred-free-helper.c
create mode 100644 drivers/dma-buf/heaps/deferred-free-helper.h
create mode 100644 drivers/gpu/drm/page_pool.c
create mode 100644 include/drm/page_pool.h
--
2.25.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v8 1/5] drm: Add a sharable drm page-pool implementation
2021-03-04 23:20 ` John Stultz
@ 2021-03-04 23:20 ` John Stultz
-1 siblings, 0 replies; 24+ messages in thread
From: John Stultz @ 2021-03-04 23:20 UTC (permalink / raw)
To: lkml
Cc: John Stultz, Daniel Vetter, Christian Koenig, Sumit Semwal,
Liam Mark, Chris Goldsworthy, Laura Abbott, Brian Starkey,
Hridya Valsaraju, Suren Baghdasaryan, Sandeep Patil,
Daniel Mentz, Ørjan Eide, Robin Murphy, Ezequiel Garcia,
Simon Ser, James Jones, linux-media, dri-devel
This adds a shrinker controlled page pool, extracted
out of the ttm_pool logic, and abstracted out a bit
so it can be used by other non-ttm drivers.
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Christian Koenig <christian.koenig@amd.com>
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>
---
v8:
* Completely rewritten from scratch, using only the
ttm_pool logic so it can be dual licensed.
---
drivers/gpu/drm/Kconfig | 4 +
drivers/gpu/drm/Makefile | 2 +
drivers/gpu/drm/page_pool.c | 214 ++++++++++++++++++++++++++++++++++++
include/drm/page_pool.h | 65 +++++++++++
4 files changed, 285 insertions(+)
create mode 100644 drivers/gpu/drm/page_pool.c
create mode 100644 include/drm/page_pool.h
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index e392a90ca687..7cbcecb8f7df 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -177,6 +177,10 @@ config DRM_DP_CEC
Note: not all adapters support this feature, and even for those
that do support this they often do not hook up the CEC pin.
+config DRM_PAGE_POOL
+ bool
+ depends on DRM
+
config DRM_TTM
tristate
depends on DRM && MMU
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 926adef289db..2dc7b2fe3fe5 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -39,6 +39,8 @@ obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o
drm_ttm_helper-y := drm_gem_ttm_helper.o
obj-$(CONFIG_DRM_TTM_HELPER) += drm_ttm_helper.o
+drm-$(CONFIG_DRM_PAGE_POOL) += page_pool.o
+
drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o drm_dp_helper.o \
drm_dsc.o drm_probe_helper.o \
drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
diff --git a/drivers/gpu/drm/page_pool.c b/drivers/gpu/drm/page_pool.c
new file mode 100644
index 000000000000..a60b954cfe0f
--- /dev/null
+++ b/drivers/gpu/drm/page_pool.c
@@ -0,0 +1,214 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/*
+ * Sharable page pool implementation
+ *
+ * Extracted from drivers/gpu/drm/ttm/ttm_pool.c
+ * Copyright 2020 Advanced Micro Devices, Inc.
+ * Copyright 2021 Linaro Ltd.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors: Christian König, John Stultz
+ */
+
+#include <linux/module.h>
+#include <linux/highmem.h>
+#include <linux/shrinker.h>
+#include <drm/page_pool.h>
+
+static unsigned long page_pool_size;
+
+MODULE_PARM_DESC(page_pool_size, "Number of pages in the WC/UC/DMA pool");
+module_param(page_pool_size, ulong, 0644);
+
+static atomic_long_t allocated_pages;
+
+static struct mutex shrinker_lock;
+static struct list_head shrinker_list;
+static struct shrinker mm_shrinker;
+
+void drm_page_pool_set_max(unsigned long max)
+{
+ if (!page_pool_size)
+ page_pool_size = max;
+}
+
+unsigned long drm_page_pool_get_max(void)
+{
+ return page_pool_size;
+}
+
+unsigned long drm_page_pool_get_total(void)
+{
+ return atomic_long_read(&allocated_pages);
+}
+
+unsigned long drm_page_pool_get_size(struct drm_page_pool *pool)
+{
+ unsigned long size;
+
+ spin_lock(&pool->lock);
+ size = pool->page_count;
+ spin_unlock(&pool->lock);
+ return size;
+}
+
+/* Give pages into a specific pool */
+void drm_page_pool_add(struct drm_page_pool *pool, struct page *p)
+{
+ unsigned int i, num_pages = 1 << pool->order;
+
+ for (i = 0; i < num_pages; ++i) {
+ if (PageHighMem(p))
+ clear_highpage(p + i);
+ else
+ clear_page(page_address(p + i));
+ }
+
+ spin_lock(&pool->lock);
+ list_add(&p->lru, &pool->pages);
+ pool->page_count += 1 << pool->order;
+ spin_unlock(&pool->lock);
+ atomic_long_add(1 << pool->order, &allocated_pages);
+}
+
+/* Take pages from a specific pool, return NULL when nothing available */
+struct page *drm_page_pool_remove(struct drm_page_pool *pool)
+{
+ struct page *p;
+
+ spin_lock(&pool->lock);
+ p = list_first_entry_or_null(&pool->pages, typeof(*p), lru);
+ if (p) {
+ atomic_long_sub(1 << pool->order, &allocated_pages);
+ pool->page_count -= 1 << pool->order;
+ list_del(&p->lru);
+ }
+ spin_unlock(&pool->lock);
+
+ return p;
+}
+
+/* Initialize and add a pool type to the global shrinker list */
+void drm_page_pool_init(struct drm_page_pool *pool, unsigned int order,
+ unsigned long (*free_page)(struct drm_page_pool *pool, struct page *p))
+{
+ pool->order = order;
+ spin_lock_init(&pool->lock);
+ INIT_LIST_HEAD(&pool->pages);
+ pool->free = free_page;
+ pool->page_count = 0;
+
+ mutex_lock(&shrinker_lock);
+ list_add_tail(&pool->shrinker_list, &shrinker_list);
+ mutex_unlock(&shrinker_lock);
+}
+
+/* Remove a pool_type from the global shrinker list and free all pages */
+void drm_page_pool_fini(struct drm_page_pool *pool)
+{
+ struct page *p, *tmp;
+
+ mutex_lock(&shrinker_lock);
+ list_del(&pool->shrinker_list);
+ mutex_unlock(&shrinker_lock);
+
+ list_for_each_entry_safe(p, tmp, &pool->pages, lru)
+ pool->free(pool, p);
+}
+
+/* Free pages using the global shrinker list */
+static unsigned int drm_page_pool_shrink(void)
+{
+ struct drm_page_pool *pool;
+ unsigned int num_freed;
+ struct page *p;
+
+ mutex_lock(&shrinker_lock);
+ pool = list_first_entry(&shrinker_list, typeof(*pool), shrinker_list);
+
+ p = drm_page_pool_remove(pool);
+
+ list_move_tail(&pool->shrinker_list, &shrinker_list);
+ mutex_unlock(&shrinker_lock);
+
+ if (p) {
+ pool->free(pool, p);
+ num_freed = 1 << pool->order;
+ } else {
+ num_freed = 0;
+ }
+
+ return num_freed;
+}
+
+/* As long as pages are available make sure to release at least one */
+static unsigned long drm_page_pool_shrinker_scan(struct shrinker *shrink,
+ struct shrink_control *sc)
+{
+ unsigned long num_freed = 0;
+
+ do
+ num_freed += drm_page_pool_shrink();
+ while (!num_freed && atomic_long_read(&allocated_pages));
+
+ return num_freed;
+}
+
+/* Return the number of pages available or SHRINK_EMPTY if we have none */
+static unsigned long drm_page_pool_shrinker_count(struct shrinker *shrink,
+ struct shrink_control *sc)
+{
+ unsigned long num_pages = atomic_long_read(&allocated_pages);
+
+ return num_pages ? num_pages : SHRINK_EMPTY;
+}
+
+/**
+ * drm_page_pool_shrinker_setup - Initialize globals
+ *
+ * @num_pages: default number of pages
+ *
+ * Initialize the global locks and lists for the shrinker.
+ */
+int drm_page_pool_shrinker_setup(void)
+{
+ mutex_init(&shrinker_lock);
+ INIT_LIST_HEAD(&shrinker_list);
+
+ mm_shrinker.count_objects = drm_page_pool_shrinker_count;
+ mm_shrinker.scan_objects = drm_page_pool_shrinker_scan;
+ mm_shrinker.seeks = 1;
+ return register_shrinker(&mm_shrinker);
+}
+
+/**
+ * drm_page_pool_shrinker_teardown - Finalize globals
+ *
+ * Unregister the shrinker.
+ */
+void drm_page_pool_shrinker_teardown(void)
+{
+ unregister_shrinker(&mm_shrinker);
+ WARN_ON(!list_empty(&shrinker_list));
+}
+
+module_init(drm_page_pool_shrinker_setup);
+module_exit(drm_page_pool_shrinker_teardown);
+MODULE_LICENSE("Dual MIT/GPL");
diff --git a/include/drm/page_pool.h b/include/drm/page_pool.h
new file mode 100644
index 000000000000..d8b8a8415629
--- /dev/null
+++ b/include/drm/page_pool.h
@@ -0,0 +1,65 @@
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+/*
+ * Extracted from include/drm/ttm/ttm_pool.h
+ * Copyright 2020 Advanced Micro Devices, Inc.
+ * Copyright 2021 Linaro Ltd
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors: Christian König, John Stultz
+ */
+
+#ifndef _DRM_PAGE_POOL_H_
+#define _DRM_PAGE_POOL_H_
+
+#include <linux/mmzone.h>
+#include <linux/llist.h>
+#include <linux/spinlock.h>
+
+/**
+ * drm_page_pool - Page Pool for a certain memory type
+ *
+ * @order: the allocation order our pages have
+ * @pages: the list of pages in the pool
+ * @shrinker_list: our place on the global shrinker list
+ * @lock: protection of the page list
+ * @page_count: number of pages currently in the pool
+ * @free: Function pointer to free the page
+ */
+struct drm_page_pool {
+ unsigned int order;
+ struct list_head pages;
+ struct list_head shrinker_list;
+ spinlock_t lock;
+
+ unsigned long page_count;
+ unsigned long (*free)(struct drm_page_pool *pool, struct page *p);
+};
+
+void drm_page_pool_set_max(unsigned long max);
+unsigned long drm_page_pool_get_max(void);
+unsigned long drm_page_pool_get_total(void);
+unsigned long drm_page_pool_get_size(struct drm_page_pool *pool);
+void drm_page_pool_add(struct drm_page_pool *pool, struct page *p);
+struct page *drm_page_pool_remove(struct drm_page_pool *pool);
+void drm_page_pool_init(struct drm_page_pool *pool, unsigned int order,
+ unsigned long (*free_page)(struct drm_page_pool *pool, struct page *p));
+void drm_page_pool_fini(struct drm_page_pool *pool);
+
+#endif
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v8 1/5] drm: Add a sharable drm page-pool implementation
@ 2021-03-04 23:20 ` John Stultz
0 siblings, 0 replies; 24+ messages in thread
From: John Stultz @ 2021-03-04 23:20 UTC (permalink / raw)
To: lkml
Cc: dri-devel, Sandeep Patil, Ezequiel Garcia, Robin Murphy,
James Jones, Liam Mark, Laura Abbott, Chris Goldsworthy,
Hridya Valsaraju, Ørjan Eide, linux-media,
Suren Baghdasaryan, Christian Koenig, Daniel Mentz
This adds a shrinker controlled page pool, extracted
out of the ttm_pool logic, and abstracted out a bit
so it can be used by other non-ttm drivers.
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Christian Koenig <christian.koenig@amd.com>
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>
---
v8:
* Completely rewritten from scratch, using only the
ttm_pool logic so it can be dual licensed.
---
drivers/gpu/drm/Kconfig | 4 +
drivers/gpu/drm/Makefile | 2 +
drivers/gpu/drm/page_pool.c | 214 ++++++++++++++++++++++++++++++++++++
include/drm/page_pool.h | 65 +++++++++++
4 files changed, 285 insertions(+)
create mode 100644 drivers/gpu/drm/page_pool.c
create mode 100644 include/drm/page_pool.h
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index e392a90ca687..7cbcecb8f7df 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -177,6 +177,10 @@ config DRM_DP_CEC
Note: not all adapters support this feature, and even for those
that do support this they often do not hook up the CEC pin.
+config DRM_PAGE_POOL
+ bool
+ depends on DRM
+
config DRM_TTM
tristate
depends on DRM && MMU
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 926adef289db..2dc7b2fe3fe5 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -39,6 +39,8 @@ obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o
drm_ttm_helper-y := drm_gem_ttm_helper.o
obj-$(CONFIG_DRM_TTM_HELPER) += drm_ttm_helper.o
+drm-$(CONFIG_DRM_PAGE_POOL) += page_pool.o
+
drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o drm_dp_helper.o \
drm_dsc.o drm_probe_helper.o \
drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
diff --git a/drivers/gpu/drm/page_pool.c b/drivers/gpu/drm/page_pool.c
new file mode 100644
index 000000000000..a60b954cfe0f
--- /dev/null
+++ b/drivers/gpu/drm/page_pool.c
@@ -0,0 +1,214 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/*
+ * Sharable page pool implementation
+ *
+ * Extracted from drivers/gpu/drm/ttm/ttm_pool.c
+ * Copyright 2020 Advanced Micro Devices, Inc.
+ * Copyright 2021 Linaro Ltd.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors: Christian König, John Stultz
+ */
+
+#include <linux/module.h>
+#include <linux/highmem.h>
+#include <linux/shrinker.h>
+#include <drm/page_pool.h>
+
+static unsigned long page_pool_size;
+
+MODULE_PARM_DESC(page_pool_size, "Number of pages in the WC/UC/DMA pool");
+module_param(page_pool_size, ulong, 0644);
+
+static atomic_long_t allocated_pages;
+
+static struct mutex shrinker_lock;
+static struct list_head shrinker_list;
+static struct shrinker mm_shrinker;
+
+void drm_page_pool_set_max(unsigned long max)
+{
+ if (!page_pool_size)
+ page_pool_size = max;
+}
+
+unsigned long drm_page_pool_get_max(void)
+{
+ return page_pool_size;
+}
+
+unsigned long drm_page_pool_get_total(void)
+{
+ return atomic_long_read(&allocated_pages);
+}
+
+unsigned long drm_page_pool_get_size(struct drm_page_pool *pool)
+{
+ unsigned long size;
+
+ spin_lock(&pool->lock);
+ size = pool->page_count;
+ spin_unlock(&pool->lock);
+ return size;
+}
+
+/* Give pages into a specific pool */
+void drm_page_pool_add(struct drm_page_pool *pool, struct page *p)
+{
+ unsigned int i, num_pages = 1 << pool->order;
+
+ for (i = 0; i < num_pages; ++i) {
+ if (PageHighMem(p))
+ clear_highpage(p + i);
+ else
+ clear_page(page_address(p + i));
+ }
+
+ spin_lock(&pool->lock);
+ list_add(&p->lru, &pool->pages);
+ pool->page_count += 1 << pool->order;
+ spin_unlock(&pool->lock);
+ atomic_long_add(1 << pool->order, &allocated_pages);
+}
+
+/* Take pages from a specific pool, return NULL when nothing available */
+struct page *drm_page_pool_remove(struct drm_page_pool *pool)
+{
+ struct page *p;
+
+ spin_lock(&pool->lock);
+ p = list_first_entry_or_null(&pool->pages, typeof(*p), lru);
+ if (p) {
+ atomic_long_sub(1 << pool->order, &allocated_pages);
+ pool->page_count -= 1 << pool->order;
+ list_del(&p->lru);
+ }
+ spin_unlock(&pool->lock);
+
+ return p;
+}
+
+/* Initialize and add a pool type to the global shrinker list */
+void drm_page_pool_init(struct drm_page_pool *pool, unsigned int order,
+ unsigned long (*free_page)(struct drm_page_pool *pool, struct page *p))
+{
+ pool->order = order;
+ spin_lock_init(&pool->lock);
+ INIT_LIST_HEAD(&pool->pages);
+ pool->free = free_page;
+ pool->page_count = 0;
+
+ mutex_lock(&shrinker_lock);
+ list_add_tail(&pool->shrinker_list, &shrinker_list);
+ mutex_unlock(&shrinker_lock);
+}
+
+/* Remove a pool_type from the global shrinker list and free all pages */
+void drm_page_pool_fini(struct drm_page_pool *pool)
+{
+ struct page *p, *tmp;
+
+ mutex_lock(&shrinker_lock);
+ list_del(&pool->shrinker_list);
+ mutex_unlock(&shrinker_lock);
+
+ list_for_each_entry_safe(p, tmp, &pool->pages, lru)
+ pool->free(pool, p);
+}
+
+/* Free pages using the global shrinker list */
+static unsigned int drm_page_pool_shrink(void)
+{
+ struct drm_page_pool *pool;
+ unsigned int num_freed;
+ struct page *p;
+
+ mutex_lock(&shrinker_lock);
+ pool = list_first_entry(&shrinker_list, typeof(*pool), shrinker_list);
+
+ p = drm_page_pool_remove(pool);
+
+ list_move_tail(&pool->shrinker_list, &shrinker_list);
+ mutex_unlock(&shrinker_lock);
+
+ if (p) {
+ pool->free(pool, p);
+ num_freed = 1 << pool->order;
+ } else {
+ num_freed = 0;
+ }
+
+ return num_freed;
+}
+
+/* As long as pages are available make sure to release at least one */
+static unsigned long drm_page_pool_shrinker_scan(struct shrinker *shrink,
+ struct shrink_control *sc)
+{
+ unsigned long num_freed = 0;
+
+ do
+ num_freed += drm_page_pool_shrink();
+ while (!num_freed && atomic_long_read(&allocated_pages));
+
+ return num_freed;
+}
+
+/* Return the number of pages available or SHRINK_EMPTY if we have none */
+static unsigned long drm_page_pool_shrinker_count(struct shrinker *shrink,
+ struct shrink_control *sc)
+{
+ unsigned long num_pages = atomic_long_read(&allocated_pages);
+
+ return num_pages ? num_pages : SHRINK_EMPTY;
+}
+
+/**
+ * drm_page_pool_shrinker_setup - Initialize globals
+ *
+ * @num_pages: default number of pages
+ *
+ * Initialize the global locks and lists for the shrinker.
+ */
+int drm_page_pool_shrinker_setup(void)
+{
+ mutex_init(&shrinker_lock);
+ INIT_LIST_HEAD(&shrinker_list);
+
+ mm_shrinker.count_objects = drm_page_pool_shrinker_count;
+ mm_shrinker.scan_objects = drm_page_pool_shrinker_scan;
+ mm_shrinker.seeks = 1;
+ return register_shrinker(&mm_shrinker);
+}
+
+/**
+ * drm_page_pool_shrinker_teardown - Finalize globals
+ *
+ * Unregister the shrinker.
+ */
+void drm_page_pool_shrinker_teardown(void)
+{
+ unregister_shrinker(&mm_shrinker);
+ WARN_ON(!list_empty(&shrinker_list));
+}
+
+module_init(drm_page_pool_shrinker_setup);
+module_exit(drm_page_pool_shrinker_teardown);
+MODULE_LICENSE("Dual MIT/GPL");
diff --git a/include/drm/page_pool.h b/include/drm/page_pool.h
new file mode 100644
index 000000000000..d8b8a8415629
--- /dev/null
+++ b/include/drm/page_pool.h
@@ -0,0 +1,65 @@
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+/*
+ * Extracted from include/drm/ttm/ttm_pool.h
+ * Copyright 2020 Advanced Micro Devices, Inc.
+ * Copyright 2021 Linaro Ltd
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors: Christian König, John Stultz
+ */
+
+#ifndef _DRM_PAGE_POOL_H_
+#define _DRM_PAGE_POOL_H_
+
+#include <linux/mmzone.h>
+#include <linux/llist.h>
+#include <linux/spinlock.h>
+
+/**
+ * drm_page_pool - Page Pool for a certain memory type
+ *
+ * @order: the allocation order our pages have
+ * @pages: the list of pages in the pool
+ * @shrinker_list: our place on the global shrinker list
+ * @lock: protection of the page list
+ * @page_count: number of pages currently in the pool
+ * @free: Function pointer to free the page
+ */
+struct drm_page_pool {
+ unsigned int order;
+ struct list_head pages;
+ struct list_head shrinker_list;
+ spinlock_t lock;
+
+ unsigned long page_count;
+ unsigned long (*free)(struct drm_page_pool *pool, struct page *p);
+};
+
+void drm_page_pool_set_max(unsigned long max);
+unsigned long drm_page_pool_get_max(void);
+unsigned long drm_page_pool_get_total(void);
+unsigned long drm_page_pool_get_size(struct drm_page_pool *pool);
+void drm_page_pool_add(struct drm_page_pool *pool, struct page *p);
+struct page *drm_page_pool_remove(struct drm_page_pool *pool);
+void drm_page_pool_init(struct drm_page_pool *pool, unsigned int order,
+ unsigned long (*free_page)(struct drm_page_pool *pool, struct page *p));
+void drm_page_pool_fini(struct drm_page_pool *pool);
+
+#endif
--
2.25.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v8 1/5] drm: Add a sharable drm page-pool implementation
2021-03-04 23:20 ` John Stultz
@ 2021-03-05 9:59 ` Christian König
-1 siblings, 0 replies; 24+ messages in thread
From: Christian König @ 2021-03-05 9:59 UTC (permalink / raw)
To: John Stultz, lkml
Cc: Daniel Vetter, Sumit Semwal, Liam Mark, Chris Goldsworthy,
Laura Abbott, Brian Starkey, Hridya Valsaraju,
Suren Baghdasaryan, Sandeep Patil, Daniel Mentz, Ørjan Eide,
Robin Murphy, Ezequiel Garcia, Simon Ser, James Jones,
linux-media, dri-devel
Am 05.03.21 um 00:20 schrieb John Stultz:
> This adds a shrinker controlled page pool, extracted
> out of the ttm_pool logic, and abstracted out a bit
> so it can be used by other non-ttm drivers.
In general please keep the kernel doc which is in TTMs pool.
>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Christian Koenig <christian.koenig@amd.com>
> 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>
> ---
> v8:
> * Completely rewritten from scratch, using only the
> ttm_pool logic so it can be dual licensed.
> ---
> drivers/gpu/drm/Kconfig | 4 +
> drivers/gpu/drm/Makefile | 2 +
> drivers/gpu/drm/page_pool.c | 214 ++++++++++++++++++++++++++++++++++++
> include/drm/page_pool.h | 65 +++++++++++
> 4 files changed, 285 insertions(+)
> create mode 100644 drivers/gpu/drm/page_pool.c
> create mode 100644 include/drm/page_pool.h
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index e392a90ca687..7cbcecb8f7df 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -177,6 +177,10 @@ config DRM_DP_CEC
> Note: not all adapters support this feature, and even for those
> that do support this they often do not hook up the CEC pin.
>
> +config DRM_PAGE_POOL
> + bool
> + depends on DRM
> +
> config DRM_TTM
> tristate
> depends on DRM && MMU
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 926adef289db..2dc7b2fe3fe5 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -39,6 +39,8 @@ obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o
> drm_ttm_helper-y := drm_gem_ttm_helper.o
> obj-$(CONFIG_DRM_TTM_HELPER) += drm_ttm_helper.o
>
> +drm-$(CONFIG_DRM_PAGE_POOL) += page_pool.o
> +
> drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o drm_dp_helper.o \
> drm_dsc.o drm_probe_helper.o \
> drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
> diff --git a/drivers/gpu/drm/page_pool.c b/drivers/gpu/drm/page_pool.c
> new file mode 100644
> index 000000000000..a60b954cfe0f
> --- /dev/null
> +++ b/drivers/gpu/drm/page_pool.c
> @@ -0,0 +1,214 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * Sharable page pool implementation
> + *
> + * Extracted from drivers/gpu/drm/ttm/ttm_pool.c
> + * Copyright 2020 Advanced Micro Devices, Inc.
> + * Copyright 2021 Linaro Ltd.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors: Christian König, John Stultz
> + */
> +
> +#include <linux/module.h>
> +#include <linux/highmem.h>
> +#include <linux/shrinker.h>
> +#include <drm/page_pool.h>
> +
> +static unsigned long page_pool_size;
> +
> +MODULE_PARM_DESC(page_pool_size, "Number of pages in the WC/UC/DMA pool");
> +module_param(page_pool_size, ulong, 0644);
> +
> +static atomic_long_t allocated_pages;
> +
> +static struct mutex shrinker_lock;
> +static struct list_head shrinker_list;
> +static struct shrinker mm_shrinker;
> +
> +void drm_page_pool_set_max(unsigned long max)
This function and a whole bunch of other can be static.
And in general I'm not sure if we really need wrappers for functionality
like this, e.g. it is only used once during startup IIRC.
> +{
> + if (!page_pool_size)
> + page_pool_size = max;
> +}
> +
> +unsigned long drm_page_pool_get_max(void)
> +{
> + return page_pool_size;
> +}
> +
> +unsigned long drm_page_pool_get_total(void)
> +{
> + return atomic_long_read(&allocated_pages);
> +}
> +
> +unsigned long drm_page_pool_get_size(struct drm_page_pool *pool)
> +{
> + unsigned long size;
> +
> + spin_lock(&pool->lock);
> + size = pool->page_count;
> + spin_unlock(&pool->lock);
> + return size;
> +}
> +
> +/* Give pages into a specific pool */
> +void drm_page_pool_add(struct drm_page_pool *pool, struct page *p)
> +{
> + unsigned int i, num_pages = 1 << pool->order;
> +
> + for (i = 0; i < num_pages; ++i) {
> + if (PageHighMem(p))
> + clear_highpage(p + i);
> + else
> + clear_page(page_address(p + i));
> + }
> +
> + spin_lock(&pool->lock);
> + list_add(&p->lru, &pool->pages);
> + pool->page_count += 1 << pool->order;
> + spin_unlock(&pool->lock);
> + atomic_long_add(1 << pool->order, &allocated_pages);
> +}
> +
> +/* Take pages from a specific pool, return NULL when nothing available */
> +struct page *drm_page_pool_remove(struct drm_page_pool *pool)
> +{
> + struct page *p;
> +
> + spin_lock(&pool->lock);
> + p = list_first_entry_or_null(&pool->pages, typeof(*p), lru);
> + if (p) {
> + atomic_long_sub(1 << pool->order, &allocated_pages);
> + pool->page_count -= 1 << pool->order;
> + list_del(&p->lru);
> + }
> + spin_unlock(&pool->lock);
> +
> + return p;
> +}
> +
> +/* Initialize and add a pool type to the global shrinker list */
> +void drm_page_pool_init(struct drm_page_pool *pool, unsigned int order,
> + unsigned long (*free_page)(struct drm_page_pool *pool, struct page *p))
> +{
> + pool->order = order;
> + spin_lock_init(&pool->lock);
> + INIT_LIST_HEAD(&pool->pages);
> + pool->free = free_page;
> + pool->page_count = 0;
> +
> + mutex_lock(&shrinker_lock);
> + list_add_tail(&pool->shrinker_list, &shrinker_list);
> + mutex_unlock(&shrinker_lock);
> +}
> +
> +/* Remove a pool_type from the global shrinker list and free all pages */
> +void drm_page_pool_fini(struct drm_page_pool *pool)
> +{
> + struct page *p, *tmp;
> +
> + mutex_lock(&shrinker_lock);
> + list_del(&pool->shrinker_list);
> + mutex_unlock(&shrinker_lock);
> +
> + list_for_each_entry_safe(p, tmp, &pool->pages, lru)
This is buggy, see the recently committed patch to the TTM page pool on
drm-misc-next.
> + pool->free(pool, p);
> +}
> +
> +/* Free pages using the global shrinker list */
> +static unsigned int drm_page_pool_shrink(void)
> +{
> + struct drm_page_pool *pool;
> + unsigned int num_freed;
> + struct page *p;
> +
> + mutex_lock(&shrinker_lock);
> + pool = list_first_entry(&shrinker_list, typeof(*pool), shrinker_list);
> +
> + p = drm_page_pool_remove(pool);
> +
> + list_move_tail(&pool->shrinker_list, &shrinker_list);
> + mutex_unlock(&shrinker_lock);
> +
> + if (p) {
> + pool->free(pool, p);
> + num_freed = 1 << pool->order;
> + } else {
> + num_freed = 0;
> + }
> +
> + return num_freed;
The problem with this approach is that you somehow need to make sure
that the pool isn't freed while the shrinker is run.
The easiest way to archive that is to add a new helper to the shrinker
code which just grabs the write side of their semaphore and release it
again.
With that done the shrinker_lock can also be switched back to a spinlock
again.
> +}
> +
> +/* As long as pages are available make sure to release at least one */
> +static unsigned long drm_page_pool_shrinker_scan(struct shrinker *shrink,
> + struct shrink_control *sc)
> +{
> + unsigned long num_freed = 0;
> +
> + do
> + num_freed += drm_page_pool_shrink();
> + while (!num_freed && atomic_long_read(&allocated_pages));
> +
> + return num_freed;
IIRC this also need a "num_pages ? num_pages : SHRINK_EMPTY", but please
double check with the shrinker code.
> +}
> +
> +/* Return the number of pages available or SHRINK_EMPTY if we have none */
> +static unsigned long drm_page_pool_shrinker_count(struct shrinker *shrink,
> + struct shrink_control *sc)
> +{
> + unsigned long num_pages = atomic_long_read(&allocated_pages);
> +
> + return num_pages ? num_pages : SHRINK_EMPTY;
> +}
> +
> +/**
> + * drm_page_pool_shrinker_setup - Initialize globals
> + *
> + * @num_pages: default number of pages
> + *
> + * Initialize the global locks and lists for the shrinker.
> + */
> +int drm_page_pool_shrinker_setup(void)
Please call that one _init() and maybe give the pool size as parameter.
> +{
> + mutex_init(&shrinker_lock);
> + INIT_LIST_HEAD(&shrinker_list);
> +
> + mm_shrinker.count_objects = drm_page_pool_shrinker_count;
> + mm_shrinker.scan_objects = drm_page_pool_shrinker_scan;
> + mm_shrinker.seeks = 1;
> + return register_shrinker(&mm_shrinker);
> +}
> +
> +/**
> + * drm_page_pool_shrinker_teardown - Finalize globals
> + *
> + * Unregister the shrinker.
> + */
> +void drm_page_pool_shrinker_teardown(void)
Please call that _fini().
Regards,
Christian.
> +{
> + unregister_shrinker(&mm_shrinker);
> + WARN_ON(!list_empty(&shrinker_list));
> +}
> +
> +module_init(drm_page_pool_shrinker_setup);
> +module_exit(drm_page_pool_shrinker_teardown);
> +MODULE_LICENSE("Dual MIT/GPL");
> diff --git a/include/drm/page_pool.h b/include/drm/page_pool.h
> new file mode 100644
> index 000000000000..d8b8a8415629
> --- /dev/null
> +++ b/include/drm/page_pool.h
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> +/*
> + * Extracted from include/drm/ttm/ttm_pool.h
> + * Copyright 2020 Advanced Micro Devices, Inc.
> + * Copyright 2021 Linaro Ltd
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors: Christian König, John Stultz
> + */
> +
> +#ifndef _DRM_PAGE_POOL_H_
> +#define _DRM_PAGE_POOL_H_
> +
> +#include <linux/mmzone.h>
> +#include <linux/llist.h>
> +#include <linux/spinlock.h>
> +
> +/**
> + * drm_page_pool - Page Pool for a certain memory type
> + *
> + * @order: the allocation order our pages have
> + * @pages: the list of pages in the pool
> + * @shrinker_list: our place on the global shrinker list
> + * @lock: protection of the page list
> + * @page_count: number of pages currently in the pool
> + * @free: Function pointer to free the page
> + */
> +struct drm_page_pool {
> + unsigned int order;
> + struct list_head pages;
> + struct list_head shrinker_list;
> + spinlock_t lock;
> +
> + unsigned long page_count;
> + unsigned long (*free)(struct drm_page_pool *pool, struct page *p);
> +};
> +
> +void drm_page_pool_set_max(unsigned long max);
> +unsigned long drm_page_pool_get_max(void);
> +unsigned long drm_page_pool_get_total(void);
> +unsigned long drm_page_pool_get_size(struct drm_page_pool *pool);
> +void drm_page_pool_add(struct drm_page_pool *pool, struct page *p);
> +struct page *drm_page_pool_remove(struct drm_page_pool *pool);
> +void drm_page_pool_init(struct drm_page_pool *pool, unsigned int order,
> + unsigned long (*free_page)(struct drm_page_pool *pool, struct page *p));
> +void drm_page_pool_fini(struct drm_page_pool *pool);
> +
> +#endif
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v8 1/5] drm: Add a sharable drm page-pool implementation
@ 2021-03-05 9:59 ` Christian König
0 siblings, 0 replies; 24+ messages in thread
From: Christian König @ 2021-03-05 9:59 UTC (permalink / raw)
To: John Stultz, lkml
Cc: dri-devel, Sandeep Patil, Chris Goldsworthy, Ezequiel Garcia,
Robin Murphy, James Jones, Liam Mark, Laura Abbott,
Hridya Valsaraju, Ørjan Eide, linux-media,
Suren Baghdasaryan, Daniel Mentz
Am 05.03.21 um 00:20 schrieb John Stultz:
> This adds a shrinker controlled page pool, extracted
> out of the ttm_pool logic, and abstracted out a bit
> so it can be used by other non-ttm drivers.
In general please keep the kernel doc which is in TTMs pool.
>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Christian Koenig <christian.koenig@amd.com>
> 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>
> ---
> v8:
> * Completely rewritten from scratch, using only the
> ttm_pool logic so it can be dual licensed.
> ---
> drivers/gpu/drm/Kconfig | 4 +
> drivers/gpu/drm/Makefile | 2 +
> drivers/gpu/drm/page_pool.c | 214 ++++++++++++++++++++++++++++++++++++
> include/drm/page_pool.h | 65 +++++++++++
> 4 files changed, 285 insertions(+)
> create mode 100644 drivers/gpu/drm/page_pool.c
> create mode 100644 include/drm/page_pool.h
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index e392a90ca687..7cbcecb8f7df 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -177,6 +177,10 @@ config DRM_DP_CEC
> Note: not all adapters support this feature, and even for those
> that do support this they often do not hook up the CEC pin.
>
> +config DRM_PAGE_POOL
> + bool
> + depends on DRM
> +
> config DRM_TTM
> tristate
> depends on DRM && MMU
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 926adef289db..2dc7b2fe3fe5 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -39,6 +39,8 @@ obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o
> drm_ttm_helper-y := drm_gem_ttm_helper.o
> obj-$(CONFIG_DRM_TTM_HELPER) += drm_ttm_helper.o
>
> +drm-$(CONFIG_DRM_PAGE_POOL) += page_pool.o
> +
> drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o drm_dp_helper.o \
> drm_dsc.o drm_probe_helper.o \
> drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
> diff --git a/drivers/gpu/drm/page_pool.c b/drivers/gpu/drm/page_pool.c
> new file mode 100644
> index 000000000000..a60b954cfe0f
> --- /dev/null
> +++ b/drivers/gpu/drm/page_pool.c
> @@ -0,0 +1,214 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * Sharable page pool implementation
> + *
> + * Extracted from drivers/gpu/drm/ttm/ttm_pool.c
> + * Copyright 2020 Advanced Micro Devices, Inc.
> + * Copyright 2021 Linaro Ltd.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors: Christian König, John Stultz
> + */
> +
> +#include <linux/module.h>
> +#include <linux/highmem.h>
> +#include <linux/shrinker.h>
> +#include <drm/page_pool.h>
> +
> +static unsigned long page_pool_size;
> +
> +MODULE_PARM_DESC(page_pool_size, "Number of pages in the WC/UC/DMA pool");
> +module_param(page_pool_size, ulong, 0644);
> +
> +static atomic_long_t allocated_pages;
> +
> +static struct mutex shrinker_lock;
> +static struct list_head shrinker_list;
> +static struct shrinker mm_shrinker;
> +
> +void drm_page_pool_set_max(unsigned long max)
This function and a whole bunch of other can be static.
And in general I'm not sure if we really need wrappers for functionality
like this, e.g. it is only used once during startup IIRC.
> +{
> + if (!page_pool_size)
> + page_pool_size = max;
> +}
> +
> +unsigned long drm_page_pool_get_max(void)
> +{
> + return page_pool_size;
> +}
> +
> +unsigned long drm_page_pool_get_total(void)
> +{
> + return atomic_long_read(&allocated_pages);
> +}
> +
> +unsigned long drm_page_pool_get_size(struct drm_page_pool *pool)
> +{
> + unsigned long size;
> +
> + spin_lock(&pool->lock);
> + size = pool->page_count;
> + spin_unlock(&pool->lock);
> + return size;
> +}
> +
> +/* Give pages into a specific pool */
> +void drm_page_pool_add(struct drm_page_pool *pool, struct page *p)
> +{
> + unsigned int i, num_pages = 1 << pool->order;
> +
> + for (i = 0; i < num_pages; ++i) {
> + if (PageHighMem(p))
> + clear_highpage(p + i);
> + else
> + clear_page(page_address(p + i));
> + }
> +
> + spin_lock(&pool->lock);
> + list_add(&p->lru, &pool->pages);
> + pool->page_count += 1 << pool->order;
> + spin_unlock(&pool->lock);
> + atomic_long_add(1 << pool->order, &allocated_pages);
> +}
> +
> +/* Take pages from a specific pool, return NULL when nothing available */
> +struct page *drm_page_pool_remove(struct drm_page_pool *pool)
> +{
> + struct page *p;
> +
> + spin_lock(&pool->lock);
> + p = list_first_entry_or_null(&pool->pages, typeof(*p), lru);
> + if (p) {
> + atomic_long_sub(1 << pool->order, &allocated_pages);
> + pool->page_count -= 1 << pool->order;
> + list_del(&p->lru);
> + }
> + spin_unlock(&pool->lock);
> +
> + return p;
> +}
> +
> +/* Initialize and add a pool type to the global shrinker list */
> +void drm_page_pool_init(struct drm_page_pool *pool, unsigned int order,
> + unsigned long (*free_page)(struct drm_page_pool *pool, struct page *p))
> +{
> + pool->order = order;
> + spin_lock_init(&pool->lock);
> + INIT_LIST_HEAD(&pool->pages);
> + pool->free = free_page;
> + pool->page_count = 0;
> +
> + mutex_lock(&shrinker_lock);
> + list_add_tail(&pool->shrinker_list, &shrinker_list);
> + mutex_unlock(&shrinker_lock);
> +}
> +
> +/* Remove a pool_type from the global shrinker list and free all pages */
> +void drm_page_pool_fini(struct drm_page_pool *pool)
> +{
> + struct page *p, *tmp;
> +
> + mutex_lock(&shrinker_lock);
> + list_del(&pool->shrinker_list);
> + mutex_unlock(&shrinker_lock);
> +
> + list_for_each_entry_safe(p, tmp, &pool->pages, lru)
This is buggy, see the recently committed patch to the TTM page pool on
drm-misc-next.
> + pool->free(pool, p);
> +}
> +
> +/* Free pages using the global shrinker list */
> +static unsigned int drm_page_pool_shrink(void)
> +{
> + struct drm_page_pool *pool;
> + unsigned int num_freed;
> + struct page *p;
> +
> + mutex_lock(&shrinker_lock);
> + pool = list_first_entry(&shrinker_list, typeof(*pool), shrinker_list);
> +
> + p = drm_page_pool_remove(pool);
> +
> + list_move_tail(&pool->shrinker_list, &shrinker_list);
> + mutex_unlock(&shrinker_lock);
> +
> + if (p) {
> + pool->free(pool, p);
> + num_freed = 1 << pool->order;
> + } else {
> + num_freed = 0;
> + }
> +
> + return num_freed;
The problem with this approach is that you somehow need to make sure
that the pool isn't freed while the shrinker is run.
The easiest way to archive that is to add a new helper to the shrinker
code which just grabs the write side of their semaphore and release it
again.
With that done the shrinker_lock can also be switched back to a spinlock
again.
> +}
> +
> +/* As long as pages are available make sure to release at least one */
> +static unsigned long drm_page_pool_shrinker_scan(struct shrinker *shrink,
> + struct shrink_control *sc)
> +{
> + unsigned long num_freed = 0;
> +
> + do
> + num_freed += drm_page_pool_shrink();
> + while (!num_freed && atomic_long_read(&allocated_pages));
> +
> + return num_freed;
IIRC this also need a "num_pages ? num_pages : SHRINK_EMPTY", but please
double check with the shrinker code.
> +}
> +
> +/* Return the number of pages available or SHRINK_EMPTY if we have none */
> +static unsigned long drm_page_pool_shrinker_count(struct shrinker *shrink,
> + struct shrink_control *sc)
> +{
> + unsigned long num_pages = atomic_long_read(&allocated_pages);
> +
> + return num_pages ? num_pages : SHRINK_EMPTY;
> +}
> +
> +/**
> + * drm_page_pool_shrinker_setup - Initialize globals
> + *
> + * @num_pages: default number of pages
> + *
> + * Initialize the global locks and lists for the shrinker.
> + */
> +int drm_page_pool_shrinker_setup(void)
Please call that one _init() and maybe give the pool size as parameter.
> +{
> + mutex_init(&shrinker_lock);
> + INIT_LIST_HEAD(&shrinker_list);
> +
> + mm_shrinker.count_objects = drm_page_pool_shrinker_count;
> + mm_shrinker.scan_objects = drm_page_pool_shrinker_scan;
> + mm_shrinker.seeks = 1;
> + return register_shrinker(&mm_shrinker);
> +}
> +
> +/**
> + * drm_page_pool_shrinker_teardown - Finalize globals
> + *
> + * Unregister the shrinker.
> + */
> +void drm_page_pool_shrinker_teardown(void)
Please call that _fini().
Regards,
Christian.
> +{
> + unregister_shrinker(&mm_shrinker);
> + WARN_ON(!list_empty(&shrinker_list));
> +}
> +
> +module_init(drm_page_pool_shrinker_setup);
> +module_exit(drm_page_pool_shrinker_teardown);
> +MODULE_LICENSE("Dual MIT/GPL");
> diff --git a/include/drm/page_pool.h b/include/drm/page_pool.h
> new file mode 100644
> index 000000000000..d8b8a8415629
> --- /dev/null
> +++ b/include/drm/page_pool.h
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> +/*
> + * Extracted from include/drm/ttm/ttm_pool.h
> + * Copyright 2020 Advanced Micro Devices, Inc.
> + * Copyright 2021 Linaro Ltd
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors: Christian König, John Stultz
> + */
> +
> +#ifndef _DRM_PAGE_POOL_H_
> +#define _DRM_PAGE_POOL_H_
> +
> +#include <linux/mmzone.h>
> +#include <linux/llist.h>
> +#include <linux/spinlock.h>
> +
> +/**
> + * drm_page_pool - Page Pool for a certain memory type
> + *
> + * @order: the allocation order our pages have
> + * @pages: the list of pages in the pool
> + * @shrinker_list: our place on the global shrinker list
> + * @lock: protection of the page list
> + * @page_count: number of pages currently in the pool
> + * @free: Function pointer to free the page
> + */
> +struct drm_page_pool {
> + unsigned int order;
> + struct list_head pages;
> + struct list_head shrinker_list;
> + spinlock_t lock;
> +
> + unsigned long page_count;
> + unsigned long (*free)(struct drm_page_pool *pool, struct page *p);
> +};
> +
> +void drm_page_pool_set_max(unsigned long max);
> +unsigned long drm_page_pool_get_max(void);
> +unsigned long drm_page_pool_get_total(void);
> +unsigned long drm_page_pool_get_size(struct drm_page_pool *pool);
> +void drm_page_pool_add(struct drm_page_pool *pool, struct page *p);
> +struct page *drm_page_pool_remove(struct drm_page_pool *pool);
> +void drm_page_pool_init(struct drm_page_pool *pool, unsigned int order,
> + unsigned long (*free_page)(struct drm_page_pool *pool, struct page *p));
> +void drm_page_pool_fini(struct drm_page_pool *pool);
> +
> +#endif
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v8 2/5] drm: ttm_pool: Rework ttm_pool to use drm_page_pool
2021-03-04 23:20 ` John Stultz
@ 2021-03-04 23:20 ` John Stultz
-1 siblings, 0 replies; 24+ messages in thread
From: John Stultz @ 2021-03-04 23:20 UTC (permalink / raw)
To: lkml
Cc: John Stultz, Daniel Vetter, Christian Koenig, Sumit Semwal,
Liam Mark, Chris Goldsworthy, Laura Abbott, Brian Starkey,
Hridya Valsaraju, Suren Baghdasaryan, Sandeep Patil,
Daniel Mentz, Ørjan Eide, Robin Murphy, Ezequiel Garcia,
Simon Ser, James Jones, linux-media, dri-devel
This patch reworks the ttm_pool logic to utilize the recently
added drm_page_pool code.
This adds drm_page_pool structures to the ttm_pool_type
structures, and then removes all the ttm_pool_type shrinker
logic (as its handled in the drm_page_pool shrinker).
NOTE: There is one mismatch in the interfaces I'm not totally
happy with. The ttm_pool tracks all of its pooled pages across
a number of different pools, and tries to keep this size under
the specified page_pool_size value. With the drm_page_pool,
there may other users, however there is still one global
shrinker list of pools. So we can't easily reduce the ttm
pool under the ttm specified size without potentially doing
a lot of shrinking to other non-ttm pools. So either we can:
1) Try to split it so each user of drm_page_pools manages its
own pool shrinking.
2) Push the max value into the drm_page_pool, and have it
manage shrinking to fit under that global max. Then share
those size/max values out so the ttm_pool debug output
can have more context.
I've taken the second path in this patch set, but wanted to call
it out so folks could look closely.
Thoughts would be greatly appreciated here!
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Christian Koenig <christian.koenig@amd.com>
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>
---
v7:
* Major refactoring to use drm_page_pools inside the
ttm_pool_type structure. This allows us to use container_of to
get the needed context to free a page. This also means less
code is changed overall.
v8:
* Reworked to use the new cleanly rewritten drm_page_pool logic
---
drivers/gpu/drm/Kconfig | 1 +
drivers/gpu/drm/ttm/ttm_pool.c | 156 ++++++---------------------------
include/drm/ttm/ttm_pool.h | 6 +-
3 files changed, 31 insertions(+), 132 deletions(-)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 7cbcecb8f7df..a6cbdb63f6c7 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -184,6 +184,7 @@ config DRM_PAGE_POOL
config DRM_TTM
tristate
depends on DRM && MMU
+ select DRM_PAGE_POOL
help
GPU memory management subsystem for devices with multiple
GPU memory types. Will be enabled automatically if a device driver
diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
index 6e27cb1bf48b..f74ea801d7ab 100644
--- a/drivers/gpu/drm/ttm/ttm_pool.c
+++ b/drivers/gpu/drm/ttm/ttm_pool.c
@@ -39,6 +39,7 @@
#include <asm/set_memory.h>
#endif
+#include <drm/page_pool.h>
#include <drm/ttm/ttm_pool.h>
#include <drm/ttm/ttm_bo_driver.h>
#include <drm/ttm/ttm_tt.h>
@@ -68,8 +69,6 @@ static struct ttm_pool_type global_dma32_write_combined[MAX_ORDER];
static struct ttm_pool_type global_dma32_uncached[MAX_ORDER];
static struct mutex shrinker_lock;
-static struct list_head shrinker_list;
-static struct shrinker mm_shrinker;
/* Allocate pages of size 1 << order with the given gfp_flags */
static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t gfp_flags,
@@ -125,8 +124,9 @@ static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t gfp_flags,
}
/* Reset the caching and pages of size 1 << order */
-static void ttm_pool_free_page(struct ttm_pool *pool, enum ttm_caching caching,
- unsigned int order, struct page *p)
+static unsigned long ttm_pool_free_page(struct ttm_pool *pool,
+ enum ttm_caching caching,
+ unsigned int order, struct page *p)
{
unsigned long attr = DMA_ATTR_FORCE_CONTIGUOUS;
struct ttm_pool_dma *dma;
@@ -142,7 +142,7 @@ static void ttm_pool_free_page(struct ttm_pool *pool, enum ttm_caching caching,
if (!pool || !pool->use_dma_alloc) {
__free_pages(p, order);
- return;
+ return 1UL << order;
}
if (order)
@@ -153,6 +153,16 @@ static void ttm_pool_free_page(struct ttm_pool *pool, enum ttm_caching caching,
dma_free_attrs(pool->dev, (1UL << order) * PAGE_SIZE, vaddr, dma->addr,
attr);
kfree(dma);
+ return 1UL << order;
+}
+
+static unsigned long ttm_subpool_free_page(struct drm_page_pool *subpool,
+ struct page *p)
+{
+ struct ttm_pool_type *pt;
+
+ pt = container_of(subpool, struct ttm_pool_type, subpool);
+ return ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
}
/* Apply a new caching to an array of pages */
@@ -216,40 +226,6 @@ static void ttm_pool_unmap(struct ttm_pool *pool, dma_addr_t dma_addr,
DMA_BIDIRECTIONAL);
}
-/* Give pages into a specific pool_type */
-static void ttm_pool_type_give(struct ttm_pool_type *pt, struct page *p)
-{
- unsigned int i, num_pages = 1 << pt->order;
-
- for (i = 0; i < num_pages; ++i) {
- if (PageHighMem(p))
- clear_highpage(p + i);
- else
- clear_page(page_address(p + i));
- }
-
- spin_lock(&pt->lock);
- list_add(&p->lru, &pt->pages);
- spin_unlock(&pt->lock);
- atomic_long_add(1 << pt->order, &allocated_pages);
-}
-
-/* Take pages from a specific pool_type, return NULL when nothing available */
-static struct page *ttm_pool_type_take(struct ttm_pool_type *pt)
-{
- struct page *p;
-
- spin_lock(&pt->lock);
- p = list_first_entry_or_null(&pt->pages, typeof(*p), lru);
- if (p) {
- atomic_long_sub(1 << pt->order, &allocated_pages);
- list_del(&p->lru);
- }
- spin_unlock(&pt->lock);
-
- return p;
-}
-
/* Initialize and add a pool type to the global shrinker list */
static void ttm_pool_type_init(struct ttm_pool_type *pt, struct ttm_pool *pool,
enum ttm_caching caching, unsigned int order)
@@ -257,25 +233,14 @@ static void ttm_pool_type_init(struct ttm_pool_type *pt, struct ttm_pool *pool,
pt->pool = pool;
pt->caching = caching;
pt->order = order;
- spin_lock_init(&pt->lock);
- INIT_LIST_HEAD(&pt->pages);
- mutex_lock(&shrinker_lock);
- list_add_tail(&pt->shrinker_list, &shrinker_list);
- mutex_unlock(&shrinker_lock);
+ drm_page_pool_init(&pt->subpool, order, ttm_subpool_free_page);
}
/* Remove a pool_type from the global shrinker list and free all pages */
static void ttm_pool_type_fini(struct ttm_pool_type *pt)
{
- struct page *p, *tmp;
-
- mutex_lock(&shrinker_lock);
- list_del(&pt->shrinker_list);
- mutex_unlock(&shrinker_lock);
-
- list_for_each_entry_safe(p, tmp, &pt->pages, lru)
- ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
+ drm_page_pool_fini(&pt->subpool);
}
/* Return the pool_type to use for the given caching and order */
@@ -306,30 +271,6 @@ static struct ttm_pool_type *ttm_pool_select_type(struct ttm_pool *pool,
return NULL;
}
-/* Free pages using the global shrinker list */
-static unsigned int ttm_pool_shrink(void)
-{
- struct ttm_pool_type *pt;
- unsigned int num_freed;
- struct page *p;
-
- mutex_lock(&shrinker_lock);
- pt = list_first_entry(&shrinker_list, typeof(*pt), shrinker_list);
-
- p = ttm_pool_type_take(pt);
- if (p) {
- ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
- num_freed = 1 << pt->order;
- } else {
- num_freed = 0;
- }
-
- list_move_tail(&pt->shrinker_list, &shrinker_list);
- mutex_unlock(&shrinker_lock);
-
- return num_freed;
-}
-
/* Return the allocation order based for a page */
static unsigned int ttm_pool_page_order(struct ttm_pool *pool, struct page *p)
{
@@ -386,7 +327,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt,
struct ttm_pool_type *pt;
pt = ttm_pool_select_type(pool, tt->caching, order);
- p = pt ? ttm_pool_type_take(pt) : NULL;
+ p = pt ? drm_page_pool_remove(&pt->subpool) : NULL;
if (p) {
apply_caching = true;
} else {
@@ -479,16 +420,13 @@ void ttm_pool_free(struct ttm_pool *pool, struct ttm_tt *tt)
pt = ttm_pool_select_type(pool, tt->caching, order);
if (pt)
- ttm_pool_type_give(pt, tt->pages[i]);
+ drm_page_pool_add(&pt->subpool, tt->pages[i]);
else
ttm_pool_free_page(pool, tt->caching, order,
tt->pages[i]);
i += num_pages;
}
-
- while (atomic_long_read(&allocated_pages) > page_pool_size)
- ttm_pool_shrink();
}
EXPORT_SYMBOL(ttm_pool_free);
@@ -537,21 +475,6 @@ void ttm_pool_fini(struct ttm_pool *pool)
}
#ifdef CONFIG_DEBUG_FS
-/* Count the number of pages available in a pool_type */
-static unsigned int ttm_pool_type_count(struct ttm_pool_type *pt)
-{
- unsigned int count = 0;
- struct page *p;
-
- spin_lock(&pt->lock);
- /* Only used for debugfs, the overhead doesn't matter */
- list_for_each_entry(p, &pt->pages, lru)
- ++count;
- spin_unlock(&pt->lock);
-
- return count;
-}
-
/* Dump information about the different pool types */
static void ttm_pool_debugfs_orders(struct ttm_pool_type *pt,
struct seq_file *m)
@@ -559,7 +482,8 @@ static void ttm_pool_debugfs_orders(struct ttm_pool_type *pt,
unsigned int i;
for (i = 0; i < MAX_ORDER; ++i)
- seq_printf(m, " %8u", ttm_pool_type_count(&pt[i]));
+ seq_printf(m, " %8lu",
+ drm_page_pool_get_size(&pt[i].subpool));
seq_puts(m, "\n");
}
@@ -609,7 +533,10 @@ int ttm_pool_debugfs(struct ttm_pool *pool, struct seq_file *m)
}
seq_printf(m, "\ntotal\t: %8lu of %8lu\n",
- atomic_long_read(&allocated_pages), page_pool_size);
+ atomic_long_read(&allocated_pages),
+ drm_page_pool_get_max());
+ seq_printf(m, "(%8lu in non-ttm pools)\n", drm_page_pool_get_total() -
+ atomic_long_read(&allocated_pages));
mutex_unlock(&shrinker_lock);
@@ -619,28 +546,6 @@ EXPORT_SYMBOL(ttm_pool_debugfs);
#endif
-/* As long as pages are available make sure to release at least one */
-static unsigned long ttm_pool_shrinker_scan(struct shrinker *shrink,
- struct shrink_control *sc)
-{
- unsigned long num_freed = 0;
-
- do
- num_freed += ttm_pool_shrink();
- while (!num_freed && atomic_long_read(&allocated_pages));
-
- return num_freed;
-}
-
-/* Return the number of pages available or SHRINK_EMPTY if we have none */
-static unsigned long ttm_pool_shrinker_count(struct shrinker *shrink,
- struct shrink_control *sc)
-{
- unsigned long num_pages = atomic_long_read(&allocated_pages);
-
- return num_pages ? num_pages : SHRINK_EMPTY;
-}
-
/**
* ttm_pool_mgr_init - Initialize globals
*
@@ -655,8 +560,9 @@ int ttm_pool_mgr_init(unsigned long num_pages)
if (!page_pool_size)
page_pool_size = num_pages;
+ drm_page_pool_set_max(page_pool_size);
+
mutex_init(&shrinker_lock);
- INIT_LIST_HEAD(&shrinker_list);
for (i = 0; i < MAX_ORDER; ++i) {
ttm_pool_type_init(&global_write_combined[i], NULL,
@@ -669,10 +575,7 @@ int ttm_pool_mgr_init(unsigned long num_pages)
ttm_uncached, i);
}
- mm_shrinker.count_objects = ttm_pool_shrinker_count;
- mm_shrinker.scan_objects = ttm_pool_shrinker_scan;
- mm_shrinker.seeks = 1;
- return register_shrinker(&mm_shrinker);
+ return 0;
}
/**
@@ -691,7 +594,4 @@ void ttm_pool_mgr_fini(void)
ttm_pool_type_fini(&global_dma32_write_combined[i]);
ttm_pool_type_fini(&global_dma32_uncached[i]);
}
-
- unregister_shrinker(&mm_shrinker);
- WARN_ON(!list_empty(&shrinker_list));
}
diff --git a/include/drm/ttm/ttm_pool.h b/include/drm/ttm/ttm_pool.h
index 4321728bdd11..3d975888ce47 100644
--- a/include/drm/ttm/ttm_pool.h
+++ b/include/drm/ttm/ttm_pool.h
@@ -30,6 +30,7 @@
#include <linux/llist.h>
#include <linux/spinlock.h>
#include <drm/ttm/ttm_caching.h>
+#include <drm/page_pool.h>
struct device;
struct ttm_tt;
@@ -51,10 +52,7 @@ struct ttm_pool_type {
unsigned int order;
enum ttm_caching caching;
- struct list_head shrinker_list;
-
- spinlock_t lock;
- struct list_head pages;
+ struct drm_page_pool subpool;
};
/**
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v8 2/5] drm: ttm_pool: Rework ttm_pool to use drm_page_pool
@ 2021-03-04 23:20 ` John Stultz
0 siblings, 0 replies; 24+ messages in thread
From: John Stultz @ 2021-03-04 23:20 UTC (permalink / raw)
To: lkml
Cc: dri-devel, Sandeep Patil, Ezequiel Garcia, Robin Murphy,
James Jones, Liam Mark, Laura Abbott, Chris Goldsworthy,
Hridya Valsaraju, Ørjan Eide, linux-media,
Suren Baghdasaryan, Christian Koenig, Daniel Mentz
This patch reworks the ttm_pool logic to utilize the recently
added drm_page_pool code.
This adds drm_page_pool structures to the ttm_pool_type
structures, and then removes all the ttm_pool_type shrinker
logic (as its handled in the drm_page_pool shrinker).
NOTE: There is one mismatch in the interfaces I'm not totally
happy with. The ttm_pool tracks all of its pooled pages across
a number of different pools, and tries to keep this size under
the specified page_pool_size value. With the drm_page_pool,
there may other users, however there is still one global
shrinker list of pools. So we can't easily reduce the ttm
pool under the ttm specified size without potentially doing
a lot of shrinking to other non-ttm pools. So either we can:
1) Try to split it so each user of drm_page_pools manages its
own pool shrinking.
2) Push the max value into the drm_page_pool, and have it
manage shrinking to fit under that global max. Then share
those size/max values out so the ttm_pool debug output
can have more context.
I've taken the second path in this patch set, but wanted to call
it out so folks could look closely.
Thoughts would be greatly appreciated here!
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Christian Koenig <christian.koenig@amd.com>
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>
---
v7:
* Major refactoring to use drm_page_pools inside the
ttm_pool_type structure. This allows us to use container_of to
get the needed context to free a page. This also means less
code is changed overall.
v8:
* Reworked to use the new cleanly rewritten drm_page_pool logic
---
drivers/gpu/drm/Kconfig | 1 +
drivers/gpu/drm/ttm/ttm_pool.c | 156 ++++++---------------------------
include/drm/ttm/ttm_pool.h | 6 +-
3 files changed, 31 insertions(+), 132 deletions(-)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 7cbcecb8f7df..a6cbdb63f6c7 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -184,6 +184,7 @@ config DRM_PAGE_POOL
config DRM_TTM
tristate
depends on DRM && MMU
+ select DRM_PAGE_POOL
help
GPU memory management subsystem for devices with multiple
GPU memory types. Will be enabled automatically if a device driver
diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
index 6e27cb1bf48b..f74ea801d7ab 100644
--- a/drivers/gpu/drm/ttm/ttm_pool.c
+++ b/drivers/gpu/drm/ttm/ttm_pool.c
@@ -39,6 +39,7 @@
#include <asm/set_memory.h>
#endif
+#include <drm/page_pool.h>
#include <drm/ttm/ttm_pool.h>
#include <drm/ttm/ttm_bo_driver.h>
#include <drm/ttm/ttm_tt.h>
@@ -68,8 +69,6 @@ static struct ttm_pool_type global_dma32_write_combined[MAX_ORDER];
static struct ttm_pool_type global_dma32_uncached[MAX_ORDER];
static struct mutex shrinker_lock;
-static struct list_head shrinker_list;
-static struct shrinker mm_shrinker;
/* Allocate pages of size 1 << order with the given gfp_flags */
static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t gfp_flags,
@@ -125,8 +124,9 @@ static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t gfp_flags,
}
/* Reset the caching and pages of size 1 << order */
-static void ttm_pool_free_page(struct ttm_pool *pool, enum ttm_caching caching,
- unsigned int order, struct page *p)
+static unsigned long ttm_pool_free_page(struct ttm_pool *pool,
+ enum ttm_caching caching,
+ unsigned int order, struct page *p)
{
unsigned long attr = DMA_ATTR_FORCE_CONTIGUOUS;
struct ttm_pool_dma *dma;
@@ -142,7 +142,7 @@ static void ttm_pool_free_page(struct ttm_pool *pool, enum ttm_caching caching,
if (!pool || !pool->use_dma_alloc) {
__free_pages(p, order);
- return;
+ return 1UL << order;
}
if (order)
@@ -153,6 +153,16 @@ static void ttm_pool_free_page(struct ttm_pool *pool, enum ttm_caching caching,
dma_free_attrs(pool->dev, (1UL << order) * PAGE_SIZE, vaddr, dma->addr,
attr);
kfree(dma);
+ return 1UL << order;
+}
+
+static unsigned long ttm_subpool_free_page(struct drm_page_pool *subpool,
+ struct page *p)
+{
+ struct ttm_pool_type *pt;
+
+ pt = container_of(subpool, struct ttm_pool_type, subpool);
+ return ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
}
/* Apply a new caching to an array of pages */
@@ -216,40 +226,6 @@ static void ttm_pool_unmap(struct ttm_pool *pool, dma_addr_t dma_addr,
DMA_BIDIRECTIONAL);
}
-/* Give pages into a specific pool_type */
-static void ttm_pool_type_give(struct ttm_pool_type *pt, struct page *p)
-{
- unsigned int i, num_pages = 1 << pt->order;
-
- for (i = 0; i < num_pages; ++i) {
- if (PageHighMem(p))
- clear_highpage(p + i);
- else
- clear_page(page_address(p + i));
- }
-
- spin_lock(&pt->lock);
- list_add(&p->lru, &pt->pages);
- spin_unlock(&pt->lock);
- atomic_long_add(1 << pt->order, &allocated_pages);
-}
-
-/* Take pages from a specific pool_type, return NULL when nothing available */
-static struct page *ttm_pool_type_take(struct ttm_pool_type *pt)
-{
- struct page *p;
-
- spin_lock(&pt->lock);
- p = list_first_entry_or_null(&pt->pages, typeof(*p), lru);
- if (p) {
- atomic_long_sub(1 << pt->order, &allocated_pages);
- list_del(&p->lru);
- }
- spin_unlock(&pt->lock);
-
- return p;
-}
-
/* Initialize and add a pool type to the global shrinker list */
static void ttm_pool_type_init(struct ttm_pool_type *pt, struct ttm_pool *pool,
enum ttm_caching caching, unsigned int order)
@@ -257,25 +233,14 @@ static void ttm_pool_type_init(struct ttm_pool_type *pt, struct ttm_pool *pool,
pt->pool = pool;
pt->caching = caching;
pt->order = order;
- spin_lock_init(&pt->lock);
- INIT_LIST_HEAD(&pt->pages);
- mutex_lock(&shrinker_lock);
- list_add_tail(&pt->shrinker_list, &shrinker_list);
- mutex_unlock(&shrinker_lock);
+ drm_page_pool_init(&pt->subpool, order, ttm_subpool_free_page);
}
/* Remove a pool_type from the global shrinker list and free all pages */
static void ttm_pool_type_fini(struct ttm_pool_type *pt)
{
- struct page *p, *tmp;
-
- mutex_lock(&shrinker_lock);
- list_del(&pt->shrinker_list);
- mutex_unlock(&shrinker_lock);
-
- list_for_each_entry_safe(p, tmp, &pt->pages, lru)
- ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
+ drm_page_pool_fini(&pt->subpool);
}
/* Return the pool_type to use for the given caching and order */
@@ -306,30 +271,6 @@ static struct ttm_pool_type *ttm_pool_select_type(struct ttm_pool *pool,
return NULL;
}
-/* Free pages using the global shrinker list */
-static unsigned int ttm_pool_shrink(void)
-{
- struct ttm_pool_type *pt;
- unsigned int num_freed;
- struct page *p;
-
- mutex_lock(&shrinker_lock);
- pt = list_first_entry(&shrinker_list, typeof(*pt), shrinker_list);
-
- p = ttm_pool_type_take(pt);
- if (p) {
- ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
- num_freed = 1 << pt->order;
- } else {
- num_freed = 0;
- }
-
- list_move_tail(&pt->shrinker_list, &shrinker_list);
- mutex_unlock(&shrinker_lock);
-
- return num_freed;
-}
-
/* Return the allocation order based for a page */
static unsigned int ttm_pool_page_order(struct ttm_pool *pool, struct page *p)
{
@@ -386,7 +327,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt,
struct ttm_pool_type *pt;
pt = ttm_pool_select_type(pool, tt->caching, order);
- p = pt ? ttm_pool_type_take(pt) : NULL;
+ p = pt ? drm_page_pool_remove(&pt->subpool) : NULL;
if (p) {
apply_caching = true;
} else {
@@ -479,16 +420,13 @@ void ttm_pool_free(struct ttm_pool *pool, struct ttm_tt *tt)
pt = ttm_pool_select_type(pool, tt->caching, order);
if (pt)
- ttm_pool_type_give(pt, tt->pages[i]);
+ drm_page_pool_add(&pt->subpool, tt->pages[i]);
else
ttm_pool_free_page(pool, tt->caching, order,
tt->pages[i]);
i += num_pages;
}
-
- while (atomic_long_read(&allocated_pages) > page_pool_size)
- ttm_pool_shrink();
}
EXPORT_SYMBOL(ttm_pool_free);
@@ -537,21 +475,6 @@ void ttm_pool_fini(struct ttm_pool *pool)
}
#ifdef CONFIG_DEBUG_FS
-/* Count the number of pages available in a pool_type */
-static unsigned int ttm_pool_type_count(struct ttm_pool_type *pt)
-{
- unsigned int count = 0;
- struct page *p;
-
- spin_lock(&pt->lock);
- /* Only used for debugfs, the overhead doesn't matter */
- list_for_each_entry(p, &pt->pages, lru)
- ++count;
- spin_unlock(&pt->lock);
-
- return count;
-}
-
/* Dump information about the different pool types */
static void ttm_pool_debugfs_orders(struct ttm_pool_type *pt,
struct seq_file *m)
@@ -559,7 +482,8 @@ static void ttm_pool_debugfs_orders(struct ttm_pool_type *pt,
unsigned int i;
for (i = 0; i < MAX_ORDER; ++i)
- seq_printf(m, " %8u", ttm_pool_type_count(&pt[i]));
+ seq_printf(m, " %8lu",
+ drm_page_pool_get_size(&pt[i].subpool));
seq_puts(m, "\n");
}
@@ -609,7 +533,10 @@ int ttm_pool_debugfs(struct ttm_pool *pool, struct seq_file *m)
}
seq_printf(m, "\ntotal\t: %8lu of %8lu\n",
- atomic_long_read(&allocated_pages), page_pool_size);
+ atomic_long_read(&allocated_pages),
+ drm_page_pool_get_max());
+ seq_printf(m, "(%8lu in non-ttm pools)\n", drm_page_pool_get_total() -
+ atomic_long_read(&allocated_pages));
mutex_unlock(&shrinker_lock);
@@ -619,28 +546,6 @@ EXPORT_SYMBOL(ttm_pool_debugfs);
#endif
-/* As long as pages are available make sure to release at least one */
-static unsigned long ttm_pool_shrinker_scan(struct shrinker *shrink,
- struct shrink_control *sc)
-{
- unsigned long num_freed = 0;
-
- do
- num_freed += ttm_pool_shrink();
- while (!num_freed && atomic_long_read(&allocated_pages));
-
- return num_freed;
-}
-
-/* Return the number of pages available or SHRINK_EMPTY if we have none */
-static unsigned long ttm_pool_shrinker_count(struct shrinker *shrink,
- struct shrink_control *sc)
-{
- unsigned long num_pages = atomic_long_read(&allocated_pages);
-
- return num_pages ? num_pages : SHRINK_EMPTY;
-}
-
/**
* ttm_pool_mgr_init - Initialize globals
*
@@ -655,8 +560,9 @@ int ttm_pool_mgr_init(unsigned long num_pages)
if (!page_pool_size)
page_pool_size = num_pages;
+ drm_page_pool_set_max(page_pool_size);
+
mutex_init(&shrinker_lock);
- INIT_LIST_HEAD(&shrinker_list);
for (i = 0; i < MAX_ORDER; ++i) {
ttm_pool_type_init(&global_write_combined[i], NULL,
@@ -669,10 +575,7 @@ int ttm_pool_mgr_init(unsigned long num_pages)
ttm_uncached, i);
}
- mm_shrinker.count_objects = ttm_pool_shrinker_count;
- mm_shrinker.scan_objects = ttm_pool_shrinker_scan;
- mm_shrinker.seeks = 1;
- return register_shrinker(&mm_shrinker);
+ return 0;
}
/**
@@ -691,7 +594,4 @@ void ttm_pool_mgr_fini(void)
ttm_pool_type_fini(&global_dma32_write_combined[i]);
ttm_pool_type_fini(&global_dma32_uncached[i]);
}
-
- unregister_shrinker(&mm_shrinker);
- WARN_ON(!list_empty(&shrinker_list));
}
diff --git a/include/drm/ttm/ttm_pool.h b/include/drm/ttm/ttm_pool.h
index 4321728bdd11..3d975888ce47 100644
--- a/include/drm/ttm/ttm_pool.h
+++ b/include/drm/ttm/ttm_pool.h
@@ -30,6 +30,7 @@
#include <linux/llist.h>
#include <linux/spinlock.h>
#include <drm/ttm/ttm_caching.h>
+#include <drm/page_pool.h>
struct device;
struct ttm_tt;
@@ -51,10 +52,7 @@ struct ttm_pool_type {
unsigned int order;
enum ttm_caching caching;
- struct list_head shrinker_list;
-
- spinlock_t lock;
- struct list_head pages;
+ struct drm_page_pool subpool;
};
/**
--
2.25.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v8 2/5] drm: ttm_pool: Rework ttm_pool to use drm_page_pool
2021-03-04 23:20 ` John Stultz
(?)
@ 2021-03-05 7:02 ` kernel test robot
-1 siblings, 0 replies; 24+ messages in thread
From: kernel test robot @ 2021-03-05 7:02 UTC (permalink / raw)
To: John Stultz, lkml
Cc: kbuild-all, dri-devel, Sandeep Patil, Ezequiel Garcia,
Robin Murphy, James Jones, Liam Mark, Laura Abbott,
Chris Goldsworthy, Hridya Valsaraju
[-- Attachment #1: Type: text/plain, Size: 2174 bytes --]
Hi John,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.12-rc1]
[cannot apply to linux/master drm-intel/for-linux-next drm-tip/drm-tip next-20210305]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/John-Stultz/Generic-page-pool-deferred-freeing-for-system-dmabuf-heap/20210305-072137
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 3cb60ee6323968b694208c4cbd56a7176396e931
config: parisc-randconfig-r024-20210305 (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/701300f424bbe73234f3dc3b62581a5e6ddef78a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review John-Stultz/Generic-page-pool-deferred-freeing-for-system-dmabuf-heap/20210305-072137
git checkout 701300f424bbe73234f3dc3b62581a5e6ddef78a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
hppa-linux-ld: drivers/gpu/drm/page_pool.o: in function `drm_page_pool_shrinker_setup':
>> (.text.drm_page_pool_shrinker_setup+0x0): multiple definition of `init_module'; drivers/gpu/drm/drm_drv.o:(.init.text+0x0): first defined here
hppa-linux-ld: drivers/gpu/drm/page_pool.o: in function `drm_page_pool_shrinker_teardown':
>> (.text.drm_page_pool_shrinker_teardown+0x0): multiple definition of `cleanup_module'; drivers/gpu/drm/drm_drv.o:(.text.drm_core_exit+0x0): first defined here
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26694 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v8 2/5] drm: ttm_pool: Rework ttm_pool to use drm_page_pool
@ 2021-03-05 7:02 ` kernel test robot
0 siblings, 0 replies; 24+ messages in thread
From: kernel test robot @ 2021-03-05 7:02 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2214 bytes --]
Hi John,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.12-rc1]
[cannot apply to linux/master drm-intel/for-linux-next drm-tip/drm-tip next-20210305]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/John-Stultz/Generic-page-pool-deferred-freeing-for-system-dmabuf-heap/20210305-072137
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 3cb60ee6323968b694208c4cbd56a7176396e931
config: parisc-randconfig-r024-20210305 (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/701300f424bbe73234f3dc3b62581a5e6ddef78a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review John-Stultz/Generic-page-pool-deferred-freeing-for-system-dmabuf-heap/20210305-072137
git checkout 701300f424bbe73234f3dc3b62581a5e6ddef78a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
hppa-linux-ld: drivers/gpu/drm/page_pool.o: in function `drm_page_pool_shrinker_setup':
>> (.text.drm_page_pool_shrinker_setup+0x0): multiple definition of `init_module'; drivers/gpu/drm/drm_drv.o:(.init.text+0x0): first defined here
hppa-linux-ld: drivers/gpu/drm/page_pool.o: in function `drm_page_pool_shrinker_teardown':
>> (.text.drm_page_pool_shrinker_teardown+0x0): multiple definition of `cleanup_module'; drivers/gpu/drm/drm_drv.o:(.text.drm_core_exit+0x0): first defined here
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 26694 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v8 2/5] drm: ttm_pool: Rework ttm_pool to use drm_page_pool
@ 2021-03-05 7:02 ` kernel test robot
0 siblings, 0 replies; 24+ messages in thread
From: kernel test robot @ 2021-03-05 7:02 UTC (permalink / raw)
To: John Stultz, lkml
Cc: Sandeep Patil, Liam Mark, Robin Murphy, James Jones, dri-devel,
kbuild-all, Chris Goldsworthy, Hridya Valsaraju, Ezequiel Garcia,
Laura Abbott
[-- Attachment #1: Type: text/plain, Size: 2174 bytes --]
Hi John,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.12-rc1]
[cannot apply to linux/master drm-intel/for-linux-next drm-tip/drm-tip next-20210305]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/John-Stultz/Generic-page-pool-deferred-freeing-for-system-dmabuf-heap/20210305-072137
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 3cb60ee6323968b694208c4cbd56a7176396e931
config: parisc-randconfig-r024-20210305 (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/701300f424bbe73234f3dc3b62581a5e6ddef78a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review John-Stultz/Generic-page-pool-deferred-freeing-for-system-dmabuf-heap/20210305-072137
git checkout 701300f424bbe73234f3dc3b62581a5e6ddef78a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
hppa-linux-ld: drivers/gpu/drm/page_pool.o: in function `drm_page_pool_shrinker_setup':
>> (.text.drm_page_pool_shrinker_setup+0x0): multiple definition of `init_module'; drivers/gpu/drm/drm_drv.o:(.init.text+0x0): first defined here
hppa-linux-ld: drivers/gpu/drm/page_pool.o: in function `drm_page_pool_shrinker_teardown':
>> (.text.drm_page_pool_shrinker_teardown+0x0): multiple definition of `cleanup_module'; drivers/gpu/drm/drm_drv.o:(.text.drm_core_exit+0x0): first defined here
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26694 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v8 2/5] drm: ttm_pool: Rework ttm_pool to use drm_page_pool
2021-03-04 23:20 ` John Stultz
@ 2021-03-05 10:43 ` Christian König
-1 siblings, 0 replies; 24+ messages in thread
From: Christian König @ 2021-03-05 10:43 UTC (permalink / raw)
To: John Stultz, lkml
Cc: Daniel Vetter, Sumit Semwal, Liam Mark, Chris Goldsworthy,
Laura Abbott, Brian Starkey, Hridya Valsaraju,
Suren Baghdasaryan, Sandeep Patil, Daniel Mentz, Ørjan Eide,
Robin Murphy, Ezequiel Garcia, Simon Ser, James Jones,
linux-media, dri-devel
Am 05.03.21 um 00:20 schrieb John Stultz:
> This patch reworks the ttm_pool logic to utilize the recently
> added drm_page_pool code.
>
> This adds drm_page_pool structures to the ttm_pool_type
> structures, and then removes all the ttm_pool_type shrinker
> logic (as its handled in the drm_page_pool shrinker).
>
> NOTE: There is one mismatch in the interfaces I'm not totally
> happy with. The ttm_pool tracks all of its pooled pages across
> a number of different pools, and tries to keep this size under
> the specified page_pool_size value. With the drm_page_pool,
> there may other users, however there is still one global
> shrinker list of pools. So we can't easily reduce the ttm
> pool under the ttm specified size without potentially doing
> a lot of shrinking to other non-ttm pools. So either we can:
> 1) Try to split it so each user of drm_page_pools manages its
> own pool shrinking.
> 2) Push the max value into the drm_page_pool, and have it
> manage shrinking to fit under that global max. Then share
> those size/max values out so the ttm_pool debug output
> can have more context.
>
> I've taken the second path in this patch set, but wanted to call
> it out so folks could look closely.
That's perfectly fine with me. A global approach for the different page
pool types is desired anyway as far as I can see.
>
> Thoughts would be greatly appreciated here!
>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Christian Koenig <christian.koenig@amd.com>
> 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>
> ---
> v7:
> * Major refactoring to use drm_page_pools inside the
> ttm_pool_type structure. This allows us to use container_of to
> get the needed context to free a page. This also means less
> code is changed overall.
> v8:
> * Reworked to use the new cleanly rewritten drm_page_pool logic
> ---
> drivers/gpu/drm/Kconfig | 1 +
> drivers/gpu/drm/ttm/ttm_pool.c | 156 ++++++---------------------------
> include/drm/ttm/ttm_pool.h | 6 +-
> 3 files changed, 31 insertions(+), 132 deletions(-)
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 7cbcecb8f7df..a6cbdb63f6c7 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -184,6 +184,7 @@ config DRM_PAGE_POOL
> config DRM_TTM
> tristate
> depends on DRM && MMU
> + select DRM_PAGE_POOL
> help
> GPU memory management subsystem for devices with multiple
> GPU memory types. Will be enabled automatically if a device driver
> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index 6e27cb1bf48b..f74ea801d7ab 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> @@ -39,6 +39,7 @@
> #include <asm/set_memory.h>
> #endif
>
> +#include <drm/page_pool.h>
> #include <drm/ttm/ttm_pool.h>
> #include <drm/ttm/ttm_bo_driver.h>
> #include <drm/ttm/ttm_tt.h>
> @@ -68,8 +69,6 @@ static struct ttm_pool_type global_dma32_write_combined[MAX_ORDER];
> static struct ttm_pool_type global_dma32_uncached[MAX_ORDER];
>
> static struct mutex shrinker_lock;
> -static struct list_head shrinker_list;
> -static struct shrinker mm_shrinker;
>
> /* Allocate pages of size 1 << order with the given gfp_flags */
> static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t gfp_flags,
> @@ -125,8 +124,9 @@ static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t gfp_flags,
> }
>
> /* Reset the caching and pages of size 1 << order */
> -static void ttm_pool_free_page(struct ttm_pool *pool, enum ttm_caching caching,
> - unsigned int order, struct page *p)
> +static unsigned long ttm_pool_free_page(struct ttm_pool *pool,
> + enum ttm_caching caching,
> + unsigned int order, struct page *p)
> {
> unsigned long attr = DMA_ATTR_FORCE_CONTIGUOUS;
> struct ttm_pool_dma *dma;
> @@ -142,7 +142,7 @@ static void ttm_pool_free_page(struct ttm_pool *pool, enum ttm_caching caching,
>
> if (!pool || !pool->use_dma_alloc) {
> __free_pages(p, order);
> - return;
> + return 1UL << order;
> }
>
> if (order)
> @@ -153,6 +153,16 @@ static void ttm_pool_free_page(struct ttm_pool *pool, enum ttm_caching caching,
> dma_free_attrs(pool->dev, (1UL << order) * PAGE_SIZE, vaddr, dma->addr,
> attr);
> kfree(dma);
> + return 1UL << order;
The returned value is always the same. So you wrapper can do this and we
don't really need to change the function here.
> +}
> +
> +static unsigned long ttm_subpool_free_page(struct drm_page_pool *subpool,
> + struct page *p)
Better call this ttm_pool_free_callback.
> +{
> + struct ttm_pool_type *pt;
> +
> + pt = container_of(subpool, struct ttm_pool_type, subpool);
> + return ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
> }
>
> /* Apply a new caching to an array of pages */
> @@ -216,40 +226,6 @@ static void ttm_pool_unmap(struct ttm_pool *pool, dma_addr_t dma_addr,
> DMA_BIDIRECTIONAL);
> }
>
> -/* Give pages into a specific pool_type */
> -static void ttm_pool_type_give(struct ttm_pool_type *pt, struct page *p)
> -{
> - unsigned int i, num_pages = 1 << pt->order;
> -
> - for (i = 0; i < num_pages; ++i) {
> - if (PageHighMem(p))
> - clear_highpage(p + i);
> - else
> - clear_page(page_address(p + i));
> - }
> -
> - spin_lock(&pt->lock);
> - list_add(&p->lru, &pt->pages);
> - spin_unlock(&pt->lock);
> - atomic_long_add(1 << pt->order, &allocated_pages);
> -}
> -
> -/* Take pages from a specific pool_type, return NULL when nothing available */
> -static struct page *ttm_pool_type_take(struct ttm_pool_type *pt)
> -{
> - struct page *p;
> -
> - spin_lock(&pt->lock);
> - p = list_first_entry_or_null(&pt->pages, typeof(*p), lru);
> - if (p) {
> - atomic_long_sub(1 << pt->order, &allocated_pages);
> - list_del(&p->lru);
> - }
> - spin_unlock(&pt->lock);
> -
> - return p;
> -}
> -
> /* Initialize and add a pool type to the global shrinker list */
> static void ttm_pool_type_init(struct ttm_pool_type *pt, struct ttm_pool *pool,
> enum ttm_caching caching, unsigned int order)
> @@ -257,25 +233,14 @@ static void ttm_pool_type_init(struct ttm_pool_type *pt, struct ttm_pool *pool,
> pt->pool = pool;
> pt->caching = caching;
> pt->order = order;
The order is now duplicated and can probably be dropped from the TTM pool.
> - spin_lock_init(&pt->lock);
> - INIT_LIST_HEAD(&pt->pages);
>
> - mutex_lock(&shrinker_lock);
> - list_add_tail(&pt->shrinker_list, &shrinker_list);
> - mutex_unlock(&shrinker_lock);
> + drm_page_pool_init(&pt->subpool, order, ttm_subpool_free_page);
> }
>
> /* Remove a pool_type from the global shrinker list and free all pages */
> static void ttm_pool_type_fini(struct ttm_pool_type *pt)
> {
> - struct page *p, *tmp;
> -
> - mutex_lock(&shrinker_lock);
> - list_del(&pt->shrinker_list);
> - mutex_unlock(&shrinker_lock);
> -
> - list_for_each_entry_safe(p, tmp, &pt->pages, lru)
> - ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
> + drm_page_pool_fini(&pt->subpool);
> }
>
> /* Return the pool_type to use for the given caching and order */
> @@ -306,30 +271,6 @@ static struct ttm_pool_type *ttm_pool_select_type(struct ttm_pool *pool,
> return NULL;
> }
>
> -/* Free pages using the global shrinker list */
> -static unsigned int ttm_pool_shrink(void)
> -{
> - struct ttm_pool_type *pt;
> - unsigned int num_freed;
> - struct page *p;
> -
> - mutex_lock(&shrinker_lock);
> - pt = list_first_entry(&shrinker_list, typeof(*pt), shrinker_list);
> -
> - p = ttm_pool_type_take(pt);
> - if (p) {
> - ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
> - num_freed = 1 << pt->order;
> - } else {
> - num_freed = 0;
> - }
> -
> - list_move_tail(&pt->shrinker_list, &shrinker_list);
> - mutex_unlock(&shrinker_lock);
> -
> - return num_freed;
> -}
> -
> /* Return the allocation order based for a page */
> static unsigned int ttm_pool_page_order(struct ttm_pool *pool, struct page *p)
> {
> @@ -386,7 +327,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt,
> struct ttm_pool_type *pt;
>
> pt = ttm_pool_select_type(pool, tt->caching, order);
> - p = pt ? ttm_pool_type_take(pt) : NULL;
> + p = pt ? drm_page_pool_remove(&pt->subpool) : NULL;
> if (p) {
> apply_caching = true;
> } else {
> @@ -479,16 +420,13 @@ void ttm_pool_free(struct ttm_pool *pool, struct ttm_tt *tt)
>
> pt = ttm_pool_select_type(pool, tt->caching, order);
> if (pt)
> - ttm_pool_type_give(pt, tt->pages[i]);
> + drm_page_pool_add(&pt->subpool, tt->pages[i]);
> else
> ttm_pool_free_page(pool, tt->caching, order,
> tt->pages[i]);
>
> i += num_pages;
> }
> -
> - while (atomic_long_read(&allocated_pages) > page_pool_size)
> - ttm_pool_shrink();
That won't work. You still need to make sure that we shrink the pool to
be under the maximum.
> }
> EXPORT_SYMBOL(ttm_pool_free);
>
> @@ -537,21 +475,6 @@ void ttm_pool_fini(struct ttm_pool *pool)
> }
>
> #ifdef CONFIG_DEBUG_FS
> -/* Count the number of pages available in a pool_type */
> -static unsigned int ttm_pool_type_count(struct ttm_pool_type *pt)
> -{
> - unsigned int count = 0;
> - struct page *p;
> -
> - spin_lock(&pt->lock);
> - /* Only used for debugfs, the overhead doesn't matter */
> - list_for_each_entry(p, &pt->pages, lru)
> - ++count;
> - spin_unlock(&pt->lock);
> -
> - return count;
> -}
> -
> /* Dump information about the different pool types */
> static void ttm_pool_debugfs_orders(struct ttm_pool_type *pt,
> struct seq_file *m)
> @@ -559,7 +482,8 @@ static void ttm_pool_debugfs_orders(struct ttm_pool_type *pt,
> unsigned int i;
>
> for (i = 0; i < MAX_ORDER; ++i)
> - seq_printf(m, " %8u", ttm_pool_type_count(&pt[i]));
> + seq_printf(m, " %8lu",
> + drm_page_pool_get_size(&pt[i].subpool));
> seq_puts(m, "\n");
> }
>
> @@ -609,7 +533,10 @@ int ttm_pool_debugfs(struct ttm_pool *pool, struct seq_file *m)
> }
>
> seq_printf(m, "\ntotal\t: %8lu of %8lu\n",
> - atomic_long_read(&allocated_pages), page_pool_size);
> + atomic_long_read(&allocated_pages),
> + drm_page_pool_get_max());
> + seq_printf(m, "(%8lu in non-ttm pools)\n", drm_page_pool_get_total() -
> + atomic_long_read(&allocated_pages));
>
> mutex_unlock(&shrinker_lock);
That won't work. You need to move the debugfs functions into the DRM
pool as well or otherwise you have two separate shrinker_lock instances
and the lock protection is not correct any more.
Regards,
Christian.
>
> @@ -619,28 +546,6 @@ EXPORT_SYMBOL(ttm_pool_debugfs);
>
> #endif
>
> -/* As long as pages are available make sure to release at least one */
> -static unsigned long ttm_pool_shrinker_scan(struct shrinker *shrink,
> - struct shrink_control *sc)
> -{
> - unsigned long num_freed = 0;
> -
> - do
> - num_freed += ttm_pool_shrink();
> - while (!num_freed && atomic_long_read(&allocated_pages));
> -
> - return num_freed;
> -}
> -
> -/* Return the number of pages available or SHRINK_EMPTY if we have none */
> -static unsigned long ttm_pool_shrinker_count(struct shrinker *shrink,
> - struct shrink_control *sc)
> -{
> - unsigned long num_pages = atomic_long_read(&allocated_pages);
> -
> - return num_pages ? num_pages : SHRINK_EMPTY;
> -}
> -
> /**
> * ttm_pool_mgr_init - Initialize globals
> *
> @@ -655,8 +560,9 @@ int ttm_pool_mgr_init(unsigned long num_pages)
> if (!page_pool_size)
> page_pool_size = num_pages;
>
> + drm_page_pool_set_max(page_pool_size);
> +
> mutex_init(&shrinker_lock);
> - INIT_LIST_HEAD(&shrinker_list);
>
> for (i = 0; i < MAX_ORDER; ++i) {
> ttm_pool_type_init(&global_write_combined[i], NULL,
> @@ -669,10 +575,7 @@ int ttm_pool_mgr_init(unsigned long num_pages)
> ttm_uncached, i);
> }
>
> - mm_shrinker.count_objects = ttm_pool_shrinker_count;
> - mm_shrinker.scan_objects = ttm_pool_shrinker_scan;
> - mm_shrinker.seeks = 1;
> - return register_shrinker(&mm_shrinker);
> + return 0;
> }
>
> /**
> @@ -691,7 +594,4 @@ void ttm_pool_mgr_fini(void)
> ttm_pool_type_fini(&global_dma32_write_combined[i]);
> ttm_pool_type_fini(&global_dma32_uncached[i]);
> }
> -
> - unregister_shrinker(&mm_shrinker);
> - WARN_ON(!list_empty(&shrinker_list));
> }
> diff --git a/include/drm/ttm/ttm_pool.h b/include/drm/ttm/ttm_pool.h
> index 4321728bdd11..3d975888ce47 100644
> --- a/include/drm/ttm/ttm_pool.h
> +++ b/include/drm/ttm/ttm_pool.h
> @@ -30,6 +30,7 @@
> #include <linux/llist.h>
> #include <linux/spinlock.h>
> #include <drm/ttm/ttm_caching.h>
> +#include <drm/page_pool.h>
>
> struct device;
> struct ttm_tt;
> @@ -51,10 +52,7 @@ struct ttm_pool_type {
> unsigned int order;
> enum ttm_caching caching;
>
> - struct list_head shrinker_list;
> -
> - spinlock_t lock;
> - struct list_head pages;
> + struct drm_page_pool subpool;
> };
>
> /**
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v8 2/5] drm: ttm_pool: Rework ttm_pool to use drm_page_pool
@ 2021-03-05 10:43 ` Christian König
0 siblings, 0 replies; 24+ messages in thread
From: Christian König @ 2021-03-05 10:43 UTC (permalink / raw)
To: John Stultz, lkml
Cc: dri-devel, Sandeep Patil, Chris Goldsworthy, Ezequiel Garcia,
Robin Murphy, James Jones, Liam Mark, Laura Abbott,
Hridya Valsaraju, Ørjan Eide, linux-media,
Suren Baghdasaryan, Daniel Mentz
Am 05.03.21 um 00:20 schrieb John Stultz:
> This patch reworks the ttm_pool logic to utilize the recently
> added drm_page_pool code.
>
> This adds drm_page_pool structures to the ttm_pool_type
> structures, and then removes all the ttm_pool_type shrinker
> logic (as its handled in the drm_page_pool shrinker).
>
> NOTE: There is one mismatch in the interfaces I'm not totally
> happy with. The ttm_pool tracks all of its pooled pages across
> a number of different pools, and tries to keep this size under
> the specified page_pool_size value. With the drm_page_pool,
> there may other users, however there is still one global
> shrinker list of pools. So we can't easily reduce the ttm
> pool under the ttm specified size without potentially doing
> a lot of shrinking to other non-ttm pools. So either we can:
> 1) Try to split it so each user of drm_page_pools manages its
> own pool shrinking.
> 2) Push the max value into the drm_page_pool, and have it
> manage shrinking to fit under that global max. Then share
> those size/max values out so the ttm_pool debug output
> can have more context.
>
> I've taken the second path in this patch set, but wanted to call
> it out so folks could look closely.
That's perfectly fine with me. A global approach for the different page
pool types is desired anyway as far as I can see.
>
> Thoughts would be greatly appreciated here!
>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Christian Koenig <christian.koenig@amd.com>
> 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>
> ---
> v7:
> * Major refactoring to use drm_page_pools inside the
> ttm_pool_type structure. This allows us to use container_of to
> get the needed context to free a page. This also means less
> code is changed overall.
> v8:
> * Reworked to use the new cleanly rewritten drm_page_pool logic
> ---
> drivers/gpu/drm/Kconfig | 1 +
> drivers/gpu/drm/ttm/ttm_pool.c | 156 ++++++---------------------------
> include/drm/ttm/ttm_pool.h | 6 +-
> 3 files changed, 31 insertions(+), 132 deletions(-)
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 7cbcecb8f7df..a6cbdb63f6c7 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -184,6 +184,7 @@ config DRM_PAGE_POOL
> config DRM_TTM
> tristate
> depends on DRM && MMU
> + select DRM_PAGE_POOL
> help
> GPU memory management subsystem for devices with multiple
> GPU memory types. Will be enabled automatically if a device driver
> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index 6e27cb1bf48b..f74ea801d7ab 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> @@ -39,6 +39,7 @@
> #include <asm/set_memory.h>
> #endif
>
> +#include <drm/page_pool.h>
> #include <drm/ttm/ttm_pool.h>
> #include <drm/ttm/ttm_bo_driver.h>
> #include <drm/ttm/ttm_tt.h>
> @@ -68,8 +69,6 @@ static struct ttm_pool_type global_dma32_write_combined[MAX_ORDER];
> static struct ttm_pool_type global_dma32_uncached[MAX_ORDER];
>
> static struct mutex shrinker_lock;
> -static struct list_head shrinker_list;
> -static struct shrinker mm_shrinker;
>
> /* Allocate pages of size 1 << order with the given gfp_flags */
> static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t gfp_flags,
> @@ -125,8 +124,9 @@ static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t gfp_flags,
> }
>
> /* Reset the caching and pages of size 1 << order */
> -static void ttm_pool_free_page(struct ttm_pool *pool, enum ttm_caching caching,
> - unsigned int order, struct page *p)
> +static unsigned long ttm_pool_free_page(struct ttm_pool *pool,
> + enum ttm_caching caching,
> + unsigned int order, struct page *p)
> {
> unsigned long attr = DMA_ATTR_FORCE_CONTIGUOUS;
> struct ttm_pool_dma *dma;
> @@ -142,7 +142,7 @@ static void ttm_pool_free_page(struct ttm_pool *pool, enum ttm_caching caching,
>
> if (!pool || !pool->use_dma_alloc) {
> __free_pages(p, order);
> - return;
> + return 1UL << order;
> }
>
> if (order)
> @@ -153,6 +153,16 @@ static void ttm_pool_free_page(struct ttm_pool *pool, enum ttm_caching caching,
> dma_free_attrs(pool->dev, (1UL << order) * PAGE_SIZE, vaddr, dma->addr,
> attr);
> kfree(dma);
> + return 1UL << order;
The returned value is always the same. So you wrapper can do this and we
don't really need to change the function here.
> +}
> +
> +static unsigned long ttm_subpool_free_page(struct drm_page_pool *subpool,
> + struct page *p)
Better call this ttm_pool_free_callback.
> +{
> + struct ttm_pool_type *pt;
> +
> + pt = container_of(subpool, struct ttm_pool_type, subpool);
> + return ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
> }
>
> /* Apply a new caching to an array of pages */
> @@ -216,40 +226,6 @@ static void ttm_pool_unmap(struct ttm_pool *pool, dma_addr_t dma_addr,
> DMA_BIDIRECTIONAL);
> }
>
> -/* Give pages into a specific pool_type */
> -static void ttm_pool_type_give(struct ttm_pool_type *pt, struct page *p)
> -{
> - unsigned int i, num_pages = 1 << pt->order;
> -
> - for (i = 0; i < num_pages; ++i) {
> - if (PageHighMem(p))
> - clear_highpage(p + i);
> - else
> - clear_page(page_address(p + i));
> - }
> -
> - spin_lock(&pt->lock);
> - list_add(&p->lru, &pt->pages);
> - spin_unlock(&pt->lock);
> - atomic_long_add(1 << pt->order, &allocated_pages);
> -}
> -
> -/* Take pages from a specific pool_type, return NULL when nothing available */
> -static struct page *ttm_pool_type_take(struct ttm_pool_type *pt)
> -{
> - struct page *p;
> -
> - spin_lock(&pt->lock);
> - p = list_first_entry_or_null(&pt->pages, typeof(*p), lru);
> - if (p) {
> - atomic_long_sub(1 << pt->order, &allocated_pages);
> - list_del(&p->lru);
> - }
> - spin_unlock(&pt->lock);
> -
> - return p;
> -}
> -
> /* Initialize and add a pool type to the global shrinker list */
> static void ttm_pool_type_init(struct ttm_pool_type *pt, struct ttm_pool *pool,
> enum ttm_caching caching, unsigned int order)
> @@ -257,25 +233,14 @@ static void ttm_pool_type_init(struct ttm_pool_type *pt, struct ttm_pool *pool,
> pt->pool = pool;
> pt->caching = caching;
> pt->order = order;
The order is now duplicated and can probably be dropped from the TTM pool.
> - spin_lock_init(&pt->lock);
> - INIT_LIST_HEAD(&pt->pages);
>
> - mutex_lock(&shrinker_lock);
> - list_add_tail(&pt->shrinker_list, &shrinker_list);
> - mutex_unlock(&shrinker_lock);
> + drm_page_pool_init(&pt->subpool, order, ttm_subpool_free_page);
> }
>
> /* Remove a pool_type from the global shrinker list and free all pages */
> static void ttm_pool_type_fini(struct ttm_pool_type *pt)
> {
> - struct page *p, *tmp;
> -
> - mutex_lock(&shrinker_lock);
> - list_del(&pt->shrinker_list);
> - mutex_unlock(&shrinker_lock);
> -
> - list_for_each_entry_safe(p, tmp, &pt->pages, lru)
> - ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
> + drm_page_pool_fini(&pt->subpool);
> }
>
> /* Return the pool_type to use for the given caching and order */
> @@ -306,30 +271,6 @@ static struct ttm_pool_type *ttm_pool_select_type(struct ttm_pool *pool,
> return NULL;
> }
>
> -/* Free pages using the global shrinker list */
> -static unsigned int ttm_pool_shrink(void)
> -{
> - struct ttm_pool_type *pt;
> - unsigned int num_freed;
> - struct page *p;
> -
> - mutex_lock(&shrinker_lock);
> - pt = list_first_entry(&shrinker_list, typeof(*pt), shrinker_list);
> -
> - p = ttm_pool_type_take(pt);
> - if (p) {
> - ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
> - num_freed = 1 << pt->order;
> - } else {
> - num_freed = 0;
> - }
> -
> - list_move_tail(&pt->shrinker_list, &shrinker_list);
> - mutex_unlock(&shrinker_lock);
> -
> - return num_freed;
> -}
> -
> /* Return the allocation order based for a page */
> static unsigned int ttm_pool_page_order(struct ttm_pool *pool, struct page *p)
> {
> @@ -386,7 +327,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt,
> struct ttm_pool_type *pt;
>
> pt = ttm_pool_select_type(pool, tt->caching, order);
> - p = pt ? ttm_pool_type_take(pt) : NULL;
> + p = pt ? drm_page_pool_remove(&pt->subpool) : NULL;
> if (p) {
> apply_caching = true;
> } else {
> @@ -479,16 +420,13 @@ void ttm_pool_free(struct ttm_pool *pool, struct ttm_tt *tt)
>
> pt = ttm_pool_select_type(pool, tt->caching, order);
> if (pt)
> - ttm_pool_type_give(pt, tt->pages[i]);
> + drm_page_pool_add(&pt->subpool, tt->pages[i]);
> else
> ttm_pool_free_page(pool, tt->caching, order,
> tt->pages[i]);
>
> i += num_pages;
> }
> -
> - while (atomic_long_read(&allocated_pages) > page_pool_size)
> - ttm_pool_shrink();
That won't work. You still need to make sure that we shrink the pool to
be under the maximum.
> }
> EXPORT_SYMBOL(ttm_pool_free);
>
> @@ -537,21 +475,6 @@ void ttm_pool_fini(struct ttm_pool *pool)
> }
>
> #ifdef CONFIG_DEBUG_FS
> -/* Count the number of pages available in a pool_type */
> -static unsigned int ttm_pool_type_count(struct ttm_pool_type *pt)
> -{
> - unsigned int count = 0;
> - struct page *p;
> -
> - spin_lock(&pt->lock);
> - /* Only used for debugfs, the overhead doesn't matter */
> - list_for_each_entry(p, &pt->pages, lru)
> - ++count;
> - spin_unlock(&pt->lock);
> -
> - return count;
> -}
> -
> /* Dump information about the different pool types */
> static void ttm_pool_debugfs_orders(struct ttm_pool_type *pt,
> struct seq_file *m)
> @@ -559,7 +482,8 @@ static void ttm_pool_debugfs_orders(struct ttm_pool_type *pt,
> unsigned int i;
>
> for (i = 0; i < MAX_ORDER; ++i)
> - seq_printf(m, " %8u", ttm_pool_type_count(&pt[i]));
> + seq_printf(m, " %8lu",
> + drm_page_pool_get_size(&pt[i].subpool));
> seq_puts(m, "\n");
> }
>
> @@ -609,7 +533,10 @@ int ttm_pool_debugfs(struct ttm_pool *pool, struct seq_file *m)
> }
>
> seq_printf(m, "\ntotal\t: %8lu of %8lu\n",
> - atomic_long_read(&allocated_pages), page_pool_size);
> + atomic_long_read(&allocated_pages),
> + drm_page_pool_get_max());
> + seq_printf(m, "(%8lu in non-ttm pools)\n", drm_page_pool_get_total() -
> + atomic_long_read(&allocated_pages));
>
> mutex_unlock(&shrinker_lock);
That won't work. You need to move the debugfs functions into the DRM
pool as well or otherwise you have two separate shrinker_lock instances
and the lock protection is not correct any more.
Regards,
Christian.
>
> @@ -619,28 +546,6 @@ EXPORT_SYMBOL(ttm_pool_debugfs);
>
> #endif
>
> -/* As long as pages are available make sure to release at least one */
> -static unsigned long ttm_pool_shrinker_scan(struct shrinker *shrink,
> - struct shrink_control *sc)
> -{
> - unsigned long num_freed = 0;
> -
> - do
> - num_freed += ttm_pool_shrink();
> - while (!num_freed && atomic_long_read(&allocated_pages));
> -
> - return num_freed;
> -}
> -
> -/* Return the number of pages available or SHRINK_EMPTY if we have none */
> -static unsigned long ttm_pool_shrinker_count(struct shrinker *shrink,
> - struct shrink_control *sc)
> -{
> - unsigned long num_pages = atomic_long_read(&allocated_pages);
> -
> - return num_pages ? num_pages : SHRINK_EMPTY;
> -}
> -
> /**
> * ttm_pool_mgr_init - Initialize globals
> *
> @@ -655,8 +560,9 @@ int ttm_pool_mgr_init(unsigned long num_pages)
> if (!page_pool_size)
> page_pool_size = num_pages;
>
> + drm_page_pool_set_max(page_pool_size);
> +
> mutex_init(&shrinker_lock);
> - INIT_LIST_HEAD(&shrinker_list);
>
> for (i = 0; i < MAX_ORDER; ++i) {
> ttm_pool_type_init(&global_write_combined[i], NULL,
> @@ -669,10 +575,7 @@ int ttm_pool_mgr_init(unsigned long num_pages)
> ttm_uncached, i);
> }
>
> - mm_shrinker.count_objects = ttm_pool_shrinker_count;
> - mm_shrinker.scan_objects = ttm_pool_shrinker_scan;
> - mm_shrinker.seeks = 1;
> - return register_shrinker(&mm_shrinker);
> + return 0;
> }
>
> /**
> @@ -691,7 +594,4 @@ void ttm_pool_mgr_fini(void)
> ttm_pool_type_fini(&global_dma32_write_combined[i]);
> ttm_pool_type_fini(&global_dma32_uncached[i]);
> }
> -
> - unregister_shrinker(&mm_shrinker);
> - WARN_ON(!list_empty(&shrinker_list));
> }
> diff --git a/include/drm/ttm/ttm_pool.h b/include/drm/ttm/ttm_pool.h
> index 4321728bdd11..3d975888ce47 100644
> --- a/include/drm/ttm/ttm_pool.h
> +++ b/include/drm/ttm/ttm_pool.h
> @@ -30,6 +30,7 @@
> #include <linux/llist.h>
> #include <linux/spinlock.h>
> #include <drm/ttm/ttm_caching.h>
> +#include <drm/page_pool.h>
>
> struct device;
> struct ttm_tt;
> @@ -51,10 +52,7 @@ struct ttm_pool_type {
> unsigned int order;
> enum ttm_caching caching;
>
> - struct list_head shrinker_list;
> -
> - spinlock_t lock;
> - struct list_head pages;
> + struct drm_page_pool subpool;
> };
>
> /**
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v8 3/5] dma-buf: heaps: Add deferred-free-helper library code
2021-03-04 23:20 ` John Stultz
@ 2021-03-04 23:20 ` John Stultz
-1 siblings, 0 replies; 24+ messages in thread
From: John Stultz @ 2021-03-04 23:20 UTC (permalink / raw)
To: lkml
Cc: John Stultz, Daniel Vetter, Christian Koenig, Sumit Semwal,
Liam Mark, Chris Goldsworthy, Laura Abbott, Brian Starkey,
Hridya Valsaraju, Suren Baghdasaryan, Sandeep Patil,
Daniel Mentz, Ørjan Eide, Robin Murphy, Ezequiel Garcia,
Simon Ser, James Jones, linux-media, dri-devel
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.
As not all heaps will find this useful, its implemented as
a optional helper library that heaps can utilize.
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Christian Koenig <christian.koenig@amd.com>
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>
---
v2:
* Fix sleep in atomic issue from using a mutex, by switching
to a spinlock as Reported-by: kernel test robot <oliver.sang@intel.com>
* Cleanup API to use a reason enum for clarity and add some documentation
comments as suggested by Suren Baghdasaryan.
v3:
* Minor tweaks so it can be built as a module
* A few small fixups suggested by Daniel Mentz
v4:
* Tweak from Daniel Mentz to make sure the shrinker
count/freed values are tracked in pages not bytes
v5:
* Fix up page count tracking as suggested by Suren Baghdasaryan
v7:
* Rework accounting to use nr_pages rather then size, as suggested
by Suren Baghdasaryan
---
drivers/dma-buf/heaps/Kconfig | 3 +
drivers/dma-buf/heaps/Makefile | 1 +
drivers/dma-buf/heaps/deferred-free-helper.c | 138 +++++++++++++++++++
drivers/dma-buf/heaps/deferred-free-helper.h | 55 ++++++++
4 files changed, 197 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..f7aef8bc7119 100644
--- a/drivers/dma-buf/heaps/Kconfig
+++ b/drivers/dma-buf/heaps/Kconfig
@@ -1,3 +1,6 @@
+config DMABUF_HEAPS_DEFERRED_FREE
+ tristate
+
config 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..e19c8b68dfeb
--- /dev/null
+++ b/drivers/dma-buf/heaps/deferred-free-helper.c
@@ -0,0 +1,138 @@
+// 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_nr_pages;
+wait_queue_head_t freelist_waitqueue;
+struct task_struct *freelist_task;
+static DEFINE_SPINLOCK(free_list_lock);
+
+void deferred_free(struct deferred_freelist_item *item,
+ void (*free)(struct deferred_freelist_item*,
+ enum df_reason),
+ size_t nr_pages)
+{
+ unsigned long flags;
+
+ INIT_LIST_HEAD(&item->list);
+ item->nr_pages = nr_pages;
+ item->free = free;
+
+ spin_lock_irqsave(&free_list_lock, flags);
+ list_add(&item->list, &free_list);
+ list_nr_pages += nr_pages;
+ spin_unlock_irqrestore(&free_list_lock, flags);
+ wake_up(&freelist_waitqueue);
+}
+EXPORT_SYMBOL_GPL(deferred_free);
+
+static size_t free_one_item(enum df_reason reason)
+{
+ unsigned long flags;
+ size_t nr_pages;
+ struct deferred_freelist_item *item;
+
+ spin_lock_irqsave(&free_list_lock, flags);
+ if (list_empty(&free_list)) {
+ spin_unlock_irqrestore(&free_list_lock, flags);
+ return 0;
+ }
+ item = list_first_entry(&free_list, struct deferred_freelist_item, list);
+ list_del(&item->list);
+ nr_pages = item->nr_pages;
+ list_nr_pages -= nr_pages;
+ spin_unlock_irqrestore(&free_list_lock, flags);
+
+ item->free(item, reason);
+ return nr_pages;
+}
+
+static unsigned long get_freelist_nr_pages(void)
+{
+ unsigned long nr_pages;
+ unsigned long flags;
+
+ spin_lock_irqsave(&free_list_lock, flags);
+ nr_pages = list_nr_pages;
+ spin_unlock_irqrestore(&free_list_lock, flags);
+ return nr_pages;
+}
+
+static unsigned long freelist_shrink_count(struct shrinker *shrinker,
+ struct shrink_control *sc)
+{
+ return get_freelist_nr_pages();
+}
+
+static unsigned long freelist_shrink_scan(struct shrinker *shrinker,
+ struct shrink_control *sc)
+{
+ unsigned long total_freed = 0;
+
+ if (sc->nr_to_scan == 0)
+ return 0;
+
+ while (total_freed < sc->nr_to_scan) {
+ size_t pages_freed = free_one_item(DF_UNDER_PRESSURE);
+
+ if (!pages_freed)
+ break;
+
+ total_freed += pages_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_nr_pages() > 0);
+
+ free_one_item(DF_NORMAL);
+ }
+
+ return 0;
+}
+
+static int deferred_freelist_init(void)
+{
+ list_nr_pages = 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("Creating thread for deferred free failed\n");
+ return -1;
+ }
+ sched_set_normal(freelist_task, 19);
+
+ return register_shrinker(&freelist_shrinker);
+}
+module_init(deferred_freelist_init);
+MODULE_LICENSE("GPL v2");
+
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..11940328ce3f
--- /dev/null
+++ b/drivers/dma-buf/heaps/deferred-free-helper.h
@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef DEFERRED_FREE_HELPER_H
+#define DEFERRED_FREE_HELPER_H
+
+/**
+ * df_reason - enum for reason why item was freed
+ *
+ * This provides a reason for why the free function was called
+ * on the item. This is useful when deferred_free is used in
+ * combination with a pagepool, so under pressure the page can
+ * be immediately freed.
+ *
+ * DF_NORMAL: Normal deferred free
+ *
+ * DF_UNDER_PRESSURE: Free was called because the system
+ * is under memory pressure. Usually
+ * from a shrinker. Avoid allocating
+ * memory in the free call, as it may
+ * fail.
+ */
+enum df_reason {
+ DF_NORMAL,
+ DF_UNDER_PRESSURE,
+};
+
+/**
+ * deferred_freelist_item - item structure for deferred freelist
+ *
+ * This is to be added to the structure for whatever you want to
+ * defer freeing on.
+ *
+ * @nr_pages: number of pages used by item to be freed
+ * @free: function pointer to be called when freeing the item
+ * @list: list entry for the deferred list
+ */
+struct deferred_freelist_item {
+ size_t nr_pages;
+ void (*free)(struct deferred_freelist_item *i,
+ enum df_reason reason);
+ struct list_head list;
+};
+
+/**
+ * deferred_free - call to add item to the deferred free list
+ *
+ * @item: Pointer to deferred_freelist_item field of a structure
+ * @free: Function pointer to the free call
+ * @nr_pages: number of pages to be freed
+ */
+void deferred_free(struct deferred_freelist_item *item,
+ void (*free)(struct deferred_freelist_item *i,
+ enum df_reason reason),
+ size_t nr_pages);
+#endif
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v8 3/5] dma-buf: heaps: Add deferred-free-helper library code
@ 2021-03-04 23:20 ` John Stultz
0 siblings, 0 replies; 24+ messages in thread
From: John Stultz @ 2021-03-04 23:20 UTC (permalink / raw)
To: lkml
Cc: dri-devel, Sandeep Patil, Ezequiel Garcia, Robin Murphy,
James Jones, Liam Mark, Laura Abbott, Chris Goldsworthy,
Hridya Valsaraju, Ørjan Eide, linux-media,
Suren Baghdasaryan, Christian Koenig, Daniel Mentz
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.
As not all heaps will find this useful, its implemented as
a optional helper library that heaps can utilize.
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Christian Koenig <christian.koenig@amd.com>
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>
---
v2:
* Fix sleep in atomic issue from using a mutex, by switching
to a spinlock as Reported-by: kernel test robot <oliver.sang@intel.com>
* Cleanup API to use a reason enum for clarity and add some documentation
comments as suggested by Suren Baghdasaryan.
v3:
* Minor tweaks so it can be built as a module
* A few small fixups suggested by Daniel Mentz
v4:
* Tweak from Daniel Mentz to make sure the shrinker
count/freed values are tracked in pages not bytes
v5:
* Fix up page count tracking as suggested by Suren Baghdasaryan
v7:
* Rework accounting to use nr_pages rather then size, as suggested
by Suren Baghdasaryan
---
drivers/dma-buf/heaps/Kconfig | 3 +
drivers/dma-buf/heaps/Makefile | 1 +
drivers/dma-buf/heaps/deferred-free-helper.c | 138 +++++++++++++++++++
drivers/dma-buf/heaps/deferred-free-helper.h | 55 ++++++++
4 files changed, 197 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..f7aef8bc7119 100644
--- a/drivers/dma-buf/heaps/Kconfig
+++ b/drivers/dma-buf/heaps/Kconfig
@@ -1,3 +1,6 @@
+config DMABUF_HEAPS_DEFERRED_FREE
+ tristate
+
config 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..e19c8b68dfeb
--- /dev/null
+++ b/drivers/dma-buf/heaps/deferred-free-helper.c
@@ -0,0 +1,138 @@
+// 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_nr_pages;
+wait_queue_head_t freelist_waitqueue;
+struct task_struct *freelist_task;
+static DEFINE_SPINLOCK(free_list_lock);
+
+void deferred_free(struct deferred_freelist_item *item,
+ void (*free)(struct deferred_freelist_item*,
+ enum df_reason),
+ size_t nr_pages)
+{
+ unsigned long flags;
+
+ INIT_LIST_HEAD(&item->list);
+ item->nr_pages = nr_pages;
+ item->free = free;
+
+ spin_lock_irqsave(&free_list_lock, flags);
+ list_add(&item->list, &free_list);
+ list_nr_pages += nr_pages;
+ spin_unlock_irqrestore(&free_list_lock, flags);
+ wake_up(&freelist_waitqueue);
+}
+EXPORT_SYMBOL_GPL(deferred_free);
+
+static size_t free_one_item(enum df_reason reason)
+{
+ unsigned long flags;
+ size_t nr_pages;
+ struct deferred_freelist_item *item;
+
+ spin_lock_irqsave(&free_list_lock, flags);
+ if (list_empty(&free_list)) {
+ spin_unlock_irqrestore(&free_list_lock, flags);
+ return 0;
+ }
+ item = list_first_entry(&free_list, struct deferred_freelist_item, list);
+ list_del(&item->list);
+ nr_pages = item->nr_pages;
+ list_nr_pages -= nr_pages;
+ spin_unlock_irqrestore(&free_list_lock, flags);
+
+ item->free(item, reason);
+ return nr_pages;
+}
+
+static unsigned long get_freelist_nr_pages(void)
+{
+ unsigned long nr_pages;
+ unsigned long flags;
+
+ spin_lock_irqsave(&free_list_lock, flags);
+ nr_pages = list_nr_pages;
+ spin_unlock_irqrestore(&free_list_lock, flags);
+ return nr_pages;
+}
+
+static unsigned long freelist_shrink_count(struct shrinker *shrinker,
+ struct shrink_control *sc)
+{
+ return get_freelist_nr_pages();
+}
+
+static unsigned long freelist_shrink_scan(struct shrinker *shrinker,
+ struct shrink_control *sc)
+{
+ unsigned long total_freed = 0;
+
+ if (sc->nr_to_scan == 0)
+ return 0;
+
+ while (total_freed < sc->nr_to_scan) {
+ size_t pages_freed = free_one_item(DF_UNDER_PRESSURE);
+
+ if (!pages_freed)
+ break;
+
+ total_freed += pages_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_nr_pages() > 0);
+
+ free_one_item(DF_NORMAL);
+ }
+
+ return 0;
+}
+
+static int deferred_freelist_init(void)
+{
+ list_nr_pages = 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("Creating thread for deferred free failed\n");
+ return -1;
+ }
+ sched_set_normal(freelist_task, 19);
+
+ return register_shrinker(&freelist_shrinker);
+}
+module_init(deferred_freelist_init);
+MODULE_LICENSE("GPL v2");
+
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..11940328ce3f
--- /dev/null
+++ b/drivers/dma-buf/heaps/deferred-free-helper.h
@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef DEFERRED_FREE_HELPER_H
+#define DEFERRED_FREE_HELPER_H
+
+/**
+ * df_reason - enum for reason why item was freed
+ *
+ * This provides a reason for why the free function was called
+ * on the item. This is useful when deferred_free is used in
+ * combination with a pagepool, so under pressure the page can
+ * be immediately freed.
+ *
+ * DF_NORMAL: Normal deferred free
+ *
+ * DF_UNDER_PRESSURE: Free was called because the system
+ * is under memory pressure. Usually
+ * from a shrinker. Avoid allocating
+ * memory in the free call, as it may
+ * fail.
+ */
+enum df_reason {
+ DF_NORMAL,
+ DF_UNDER_PRESSURE,
+};
+
+/**
+ * deferred_freelist_item - item structure for deferred freelist
+ *
+ * This is to be added to the structure for whatever you want to
+ * defer freeing on.
+ *
+ * @nr_pages: number of pages used by item to be freed
+ * @free: function pointer to be called when freeing the item
+ * @list: list entry for the deferred list
+ */
+struct deferred_freelist_item {
+ size_t nr_pages;
+ void (*free)(struct deferred_freelist_item *i,
+ enum df_reason reason);
+ struct list_head list;
+};
+
+/**
+ * deferred_free - call to add item to the deferred free list
+ *
+ * @item: Pointer to deferred_freelist_item field of a structure
+ * @free: Function pointer to the free call
+ * @nr_pages: number of pages to be freed
+ */
+void deferred_free(struct deferred_freelist_item *item,
+ void (*free)(struct deferred_freelist_item *i,
+ enum df_reason reason),
+ size_t nr_pages);
+#endif
--
2.25.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v8 3/5] dma-buf: heaps: Add deferred-free-helper library code
2021-03-04 23:20 ` John Stultz
@ 2021-03-05 10:50 ` Christian König
-1 siblings, 0 replies; 24+ messages in thread
From: Christian König @ 2021-03-05 10:50 UTC (permalink / raw)
To: John Stultz, lkml
Cc: Daniel Vetter, Sumit Semwal, Liam Mark, Chris Goldsworthy,
Laura Abbott, Brian Starkey, Hridya Valsaraju,
Suren Baghdasaryan, Sandeep Patil, Daniel Mentz, Ørjan Eide,
Robin Murphy, Ezequiel Garcia, Simon Ser, James Jones,
linux-media, dri-devel
Am 05.03.21 um 00:20 schrieb John Stultz:
> 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.
In general that's a nice idea, but to be honest this implementation
looks broken and rather inefficient.
You should probably rather integrate that into the DRM pool core
functionality which is currently clearing all freed pages anyway.
I would also use a work item per pool instead of a kthread, that would
help with data locality.
Regards,
Christian.
>
> As not all heaps will find this useful, its implemented as
> a optional helper library that heaps can utilize.
>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Christian Koenig <christian.koenig@amd.com>
> 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>
> ---
> v2:
> * Fix sleep in atomic issue from using a mutex, by switching
> to a spinlock as Reported-by: kernel test robot <oliver.sang@intel.com>
> * Cleanup API to use a reason enum for clarity and add some documentation
> comments as suggested by Suren Baghdasaryan.
> v3:
> * Minor tweaks so it can be built as a module
> * A few small fixups suggested by Daniel Mentz
> v4:
> * Tweak from Daniel Mentz to make sure the shrinker
> count/freed values are tracked in pages not bytes
> v5:
> * Fix up page count tracking as suggested by Suren Baghdasaryan
> v7:
> * Rework accounting to use nr_pages rather then size, as suggested
> by Suren Baghdasaryan
> ---
> drivers/dma-buf/heaps/Kconfig | 3 +
> drivers/dma-buf/heaps/Makefile | 1 +
> drivers/dma-buf/heaps/deferred-free-helper.c | 138 +++++++++++++++++++
> drivers/dma-buf/heaps/deferred-free-helper.h | 55 ++++++++
> 4 files changed, 197 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..f7aef8bc7119 100644
> --- a/drivers/dma-buf/heaps/Kconfig
> +++ b/drivers/dma-buf/heaps/Kconfig
> @@ -1,3 +1,6 @@
> +config DMABUF_HEAPS_DEFERRED_FREE
> + tristate
> +
> config 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..e19c8b68dfeb
> --- /dev/null
> +++ b/drivers/dma-buf/heaps/deferred-free-helper.c
> @@ -0,0 +1,138 @@
> +// 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_nr_pages;
> +wait_queue_head_t freelist_waitqueue;
> +struct task_struct *freelist_task;
> +static DEFINE_SPINLOCK(free_list_lock);
> +
> +void deferred_free(struct deferred_freelist_item *item,
> + void (*free)(struct deferred_freelist_item*,
> + enum df_reason),
> + size_t nr_pages)
> +{
> + unsigned long flags;
> +
> + INIT_LIST_HEAD(&item->list);
> + item->nr_pages = nr_pages;
> + item->free = free;
> +
> + spin_lock_irqsave(&free_list_lock, flags);
> + list_add(&item->list, &free_list);
> + list_nr_pages += nr_pages;
> + spin_unlock_irqrestore(&free_list_lock, flags);
> + wake_up(&freelist_waitqueue);
> +}
> +EXPORT_SYMBOL_GPL(deferred_free);
> +
> +static size_t free_one_item(enum df_reason reason)
> +{
> + unsigned long flags;
> + size_t nr_pages;
> + struct deferred_freelist_item *item;
> +
> + spin_lock_irqsave(&free_list_lock, flags);
> + if (list_empty(&free_list)) {
> + spin_unlock_irqrestore(&free_list_lock, flags);
> + return 0;
> + }
> + item = list_first_entry(&free_list, struct deferred_freelist_item, list);
> + list_del(&item->list);
> + nr_pages = item->nr_pages;
> + list_nr_pages -= nr_pages;
> + spin_unlock_irqrestore(&free_list_lock, flags);
> +
> + item->free(item, reason);
> + return nr_pages;
> +}
> +
> +static unsigned long get_freelist_nr_pages(void)
> +{
> + unsigned long nr_pages;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&free_list_lock, flags);
> + nr_pages = list_nr_pages;
> + spin_unlock_irqrestore(&free_list_lock, flags);
> + return nr_pages;
> +}
> +
> +static unsigned long freelist_shrink_count(struct shrinker *shrinker,
> + struct shrink_control *sc)
> +{
> + return get_freelist_nr_pages();
> +}
> +
> +static unsigned long freelist_shrink_scan(struct shrinker *shrinker,
> + struct shrink_control *sc)
> +{
> + unsigned long total_freed = 0;
> +
> + if (sc->nr_to_scan == 0)
> + return 0;
> +
> + while (total_freed < sc->nr_to_scan) {
> + size_t pages_freed = free_one_item(DF_UNDER_PRESSURE);
> +
> + if (!pages_freed)
> + break;
> +
> + total_freed += pages_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_nr_pages() > 0);
> +
> + free_one_item(DF_NORMAL);
> + }
> +
> + return 0;
> +}
> +
> +static int deferred_freelist_init(void)
> +{
> + list_nr_pages = 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("Creating thread for deferred free failed\n");
> + return -1;
> + }
> + sched_set_normal(freelist_task, 19);
> +
> + return register_shrinker(&freelist_shrinker);
> +}
> +module_init(deferred_freelist_init);
> +MODULE_LICENSE("GPL v2");
> +
> 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..11940328ce3f
> --- /dev/null
> +++ b/drivers/dma-buf/heaps/deferred-free-helper.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef DEFERRED_FREE_HELPER_H
> +#define DEFERRED_FREE_HELPER_H
> +
> +/**
> + * df_reason - enum for reason why item was freed
> + *
> + * This provides a reason for why the free function was called
> + * on the item. This is useful when deferred_free is used in
> + * combination with a pagepool, so under pressure the page can
> + * be immediately freed.
> + *
> + * DF_NORMAL: Normal deferred free
> + *
> + * DF_UNDER_PRESSURE: Free was called because the system
> + * is under memory pressure. Usually
> + * from a shrinker. Avoid allocating
> + * memory in the free call, as it may
> + * fail.
> + */
> +enum df_reason {
> + DF_NORMAL,
> + DF_UNDER_PRESSURE,
> +};
> +
> +/**
> + * deferred_freelist_item - item structure for deferred freelist
> + *
> + * This is to be added to the structure for whatever you want to
> + * defer freeing on.
> + *
> + * @nr_pages: number of pages used by item to be freed
> + * @free: function pointer to be called when freeing the item
> + * @list: list entry for the deferred list
> + */
> +struct deferred_freelist_item {
> + size_t nr_pages;
> + void (*free)(struct deferred_freelist_item *i,
> + enum df_reason reason);
> + struct list_head list;
> +};
> +
> +/**
> + * deferred_free - call to add item to the deferred free list
> + *
> + * @item: Pointer to deferred_freelist_item field of a structure
> + * @free: Function pointer to the free call
> + * @nr_pages: number of pages to be freed
> + */
> +void deferred_free(struct deferred_freelist_item *item,
> + void (*free)(struct deferred_freelist_item *i,
> + enum df_reason reason),
> + size_t nr_pages);
> +#endif
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v8 3/5] dma-buf: heaps: Add deferred-free-helper library code
@ 2021-03-05 10:50 ` Christian König
0 siblings, 0 replies; 24+ messages in thread
From: Christian König @ 2021-03-05 10:50 UTC (permalink / raw)
To: John Stultz, lkml
Cc: dri-devel, Sandeep Patil, Chris Goldsworthy, Ezequiel Garcia,
Robin Murphy, James Jones, Liam Mark, Laura Abbott,
Hridya Valsaraju, Ørjan Eide, linux-media,
Suren Baghdasaryan, Daniel Mentz
Am 05.03.21 um 00:20 schrieb John Stultz:
> 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.
In general that's a nice idea, but to be honest this implementation
looks broken and rather inefficient.
You should probably rather integrate that into the DRM pool core
functionality which is currently clearing all freed pages anyway.
I would also use a work item per pool instead of a kthread, that would
help with data locality.
Regards,
Christian.
>
> As not all heaps will find this useful, its implemented as
> a optional helper library that heaps can utilize.
>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Christian Koenig <christian.koenig@amd.com>
> 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>
> ---
> v2:
> * Fix sleep in atomic issue from using a mutex, by switching
> to a spinlock as Reported-by: kernel test robot <oliver.sang@intel.com>
> * Cleanup API to use a reason enum for clarity and add some documentation
> comments as suggested by Suren Baghdasaryan.
> v3:
> * Minor tweaks so it can be built as a module
> * A few small fixups suggested by Daniel Mentz
> v4:
> * Tweak from Daniel Mentz to make sure the shrinker
> count/freed values are tracked in pages not bytes
> v5:
> * Fix up page count tracking as suggested by Suren Baghdasaryan
> v7:
> * Rework accounting to use nr_pages rather then size, as suggested
> by Suren Baghdasaryan
> ---
> drivers/dma-buf/heaps/Kconfig | 3 +
> drivers/dma-buf/heaps/Makefile | 1 +
> drivers/dma-buf/heaps/deferred-free-helper.c | 138 +++++++++++++++++++
> drivers/dma-buf/heaps/deferred-free-helper.h | 55 ++++++++
> 4 files changed, 197 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..f7aef8bc7119 100644
> --- a/drivers/dma-buf/heaps/Kconfig
> +++ b/drivers/dma-buf/heaps/Kconfig
> @@ -1,3 +1,6 @@
> +config DMABUF_HEAPS_DEFERRED_FREE
> + tristate
> +
> config 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..e19c8b68dfeb
> --- /dev/null
> +++ b/drivers/dma-buf/heaps/deferred-free-helper.c
> @@ -0,0 +1,138 @@
> +// 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_nr_pages;
> +wait_queue_head_t freelist_waitqueue;
> +struct task_struct *freelist_task;
> +static DEFINE_SPINLOCK(free_list_lock);
> +
> +void deferred_free(struct deferred_freelist_item *item,
> + void (*free)(struct deferred_freelist_item*,
> + enum df_reason),
> + size_t nr_pages)
> +{
> + unsigned long flags;
> +
> + INIT_LIST_HEAD(&item->list);
> + item->nr_pages = nr_pages;
> + item->free = free;
> +
> + spin_lock_irqsave(&free_list_lock, flags);
> + list_add(&item->list, &free_list);
> + list_nr_pages += nr_pages;
> + spin_unlock_irqrestore(&free_list_lock, flags);
> + wake_up(&freelist_waitqueue);
> +}
> +EXPORT_SYMBOL_GPL(deferred_free);
> +
> +static size_t free_one_item(enum df_reason reason)
> +{
> + unsigned long flags;
> + size_t nr_pages;
> + struct deferred_freelist_item *item;
> +
> + spin_lock_irqsave(&free_list_lock, flags);
> + if (list_empty(&free_list)) {
> + spin_unlock_irqrestore(&free_list_lock, flags);
> + return 0;
> + }
> + item = list_first_entry(&free_list, struct deferred_freelist_item, list);
> + list_del(&item->list);
> + nr_pages = item->nr_pages;
> + list_nr_pages -= nr_pages;
> + spin_unlock_irqrestore(&free_list_lock, flags);
> +
> + item->free(item, reason);
> + return nr_pages;
> +}
> +
> +static unsigned long get_freelist_nr_pages(void)
> +{
> + unsigned long nr_pages;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&free_list_lock, flags);
> + nr_pages = list_nr_pages;
> + spin_unlock_irqrestore(&free_list_lock, flags);
> + return nr_pages;
> +}
> +
> +static unsigned long freelist_shrink_count(struct shrinker *shrinker,
> + struct shrink_control *sc)
> +{
> + return get_freelist_nr_pages();
> +}
> +
> +static unsigned long freelist_shrink_scan(struct shrinker *shrinker,
> + struct shrink_control *sc)
> +{
> + unsigned long total_freed = 0;
> +
> + if (sc->nr_to_scan == 0)
> + return 0;
> +
> + while (total_freed < sc->nr_to_scan) {
> + size_t pages_freed = free_one_item(DF_UNDER_PRESSURE);
> +
> + if (!pages_freed)
> + break;
> +
> + total_freed += pages_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_nr_pages() > 0);
> +
> + free_one_item(DF_NORMAL);
> + }
> +
> + return 0;
> +}
> +
> +static int deferred_freelist_init(void)
> +{
> + list_nr_pages = 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("Creating thread for deferred free failed\n");
> + return -1;
> + }
> + sched_set_normal(freelist_task, 19);
> +
> + return register_shrinker(&freelist_shrinker);
> +}
> +module_init(deferred_freelist_init);
> +MODULE_LICENSE("GPL v2");
> +
> 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..11940328ce3f
> --- /dev/null
> +++ b/drivers/dma-buf/heaps/deferred-free-helper.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef DEFERRED_FREE_HELPER_H
> +#define DEFERRED_FREE_HELPER_H
> +
> +/**
> + * df_reason - enum for reason why item was freed
> + *
> + * This provides a reason for why the free function was called
> + * on the item. This is useful when deferred_free is used in
> + * combination with a pagepool, so under pressure the page can
> + * be immediately freed.
> + *
> + * DF_NORMAL: Normal deferred free
> + *
> + * DF_UNDER_PRESSURE: Free was called because the system
> + * is under memory pressure. Usually
> + * from a shrinker. Avoid allocating
> + * memory in the free call, as it may
> + * fail.
> + */
> +enum df_reason {
> + DF_NORMAL,
> + DF_UNDER_PRESSURE,
> +};
> +
> +/**
> + * deferred_freelist_item - item structure for deferred freelist
> + *
> + * This is to be added to the structure for whatever you want to
> + * defer freeing on.
> + *
> + * @nr_pages: number of pages used by item to be freed
> + * @free: function pointer to be called when freeing the item
> + * @list: list entry for the deferred list
> + */
> +struct deferred_freelist_item {
> + size_t nr_pages;
> + void (*free)(struct deferred_freelist_item *i,
> + enum df_reason reason);
> + struct list_head list;
> +};
> +
> +/**
> + * deferred_free - call to add item to the deferred free list
> + *
> + * @item: Pointer to deferred_freelist_item field of a structure
> + * @free: Function pointer to the free call
> + * @nr_pages: number of pages to be freed
> + */
> +void deferred_free(struct deferred_freelist_item *item,
> + void (*free)(struct deferred_freelist_item *i,
> + enum df_reason reason),
> + size_t nr_pages);
> +#endif
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v8 4/5] dma-buf: system_heap: Add drm pagepool support to system heap
2021-03-04 23:20 ` John Stultz
@ 2021-03-04 23:20 ` John Stultz
-1 siblings, 0 replies; 24+ messages in thread
From: John Stultz @ 2021-03-04 23:20 UTC (permalink / raw)
To: lkml
Cc: John Stultz, Daniel Vetter, Christian Koenig, Sumit Semwal,
Liam Mark, Chris Goldsworthy, Laura Abbott, Brian Starkey,
Hridya Valsaraju, Suren Baghdasaryan, Sandeep Patil,
Daniel Mentz, Ørjan Eide, Robin Murphy, Ezequiel Garcia,
Simon Ser, James Jones, linux-media, dri-devel
Utilize the drm pagepool code to speed up allocation
performance.
This is similar to the ION pagepool usage, but tries to
utilize generic code instead of a custom implementation.
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Christian Koenig <christian.koenig@amd.com>
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>
---
v2:
* Fix build issue caused by selecting PAGE_POOL w/o NET
as Reported-by: kernel test robot <lkp@intel.com>
v3:
* Simplify the page zeroing logic a bit by using kmap_atomic
instead of vmap as suggested by Daniel Mentz
v5:
* Shift away from networking page pool completely to
dmabuf page pool implementation
v6:
* Switch again to using the drm_page_pool code shared w/
ttm_pool
v7:
* Slight rework for drm_page_pool changes
v8:
* Rework to use the rewritten drm_page_pool logic
* Drop explicit buffer zeroing, as the drm_page_pool handles that
---
drivers/dma-buf/heaps/Kconfig | 1 +
drivers/dma-buf/heaps/system_heap.c | 27 ++++++++++++++++++++++++---
2 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
index f7aef8bc7119..7e28934e0def 100644
--- a/drivers/dma-buf/heaps/Kconfig
+++ b/drivers/dma-buf/heaps/Kconfig
@@ -4,6 +4,7 @@ config DMABUF_HEAPS_DEFERRED_FREE
config DMABUF_HEAPS_SYSTEM
bool "DMA-BUF System Heap"
depends on DMABUF_HEAPS
+ select DRM_PAGE_POOL
help
Choose this option to enable the system dmabuf heap. The system heap
is backed by pages from the buddy allocator. If in doubt, say Y.
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
index 29e49ac17251..006271881d85 100644
--- a/drivers/dma-buf/heaps/system_heap.c
+++ b/drivers/dma-buf/heaps/system_heap.c
@@ -21,6 +21,8 @@
#include <linux/slab.h>
#include <linux/vmalloc.h>
+#include <drm/page_pool.h>
+
static struct dma_heap *sys_heap;
struct system_heap_buffer {
@@ -53,6 +55,7 @@ static gfp_t order_flags[] = {HIGH_ORDER_GFP, LOW_ORDER_GFP, LOW_ORDER_GFP};
*/
static const unsigned int orders[] = {8, 4, 0};
#define NUM_ORDERS ARRAY_SIZE(orders)
+struct drm_page_pool pools[NUM_ORDERS];
static struct sg_table *dup_sg_table(struct sg_table *table)
{
@@ -281,18 +284,28 @@ static void system_heap_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map)
dma_buf_map_clear(map);
}
+static unsigned long system_heap_free_pages(struct drm_page_pool *pool, struct page *p)
+{
+ __free_pages(p, pool->order);
+ return 1 << pool->order;
+}
+
static void system_heap_dma_buf_release(struct dma_buf *dmabuf)
{
struct system_heap_buffer *buffer = dmabuf->priv;
struct sg_table *table;
struct scatterlist *sg;
- int i;
+ int i, j;
table = &buffer->sg_table;
for_each_sg(table->sgl, sg, table->nents, i) {
struct page *page = sg_page(sg);
- __free_pages(page, compound_order(page));
+ for (j = 0; j < NUM_ORDERS; j++) {
+ if (compound_order(page) == orders[j])
+ break;
+ }
+ drm_page_pool_add(&pools[j], page);
}
sg_free_table(table);
kfree(buffer);
@@ -323,7 +336,9 @@ static struct page *alloc_largest_available(unsigned long size,
if (max_order < orders[i])
continue;
- page = alloc_pages(order_flags[i], orders[i]);
+ page = drm_page_pool_remove(&pools[i]);
+ if (!page)
+ page = alloc_pages(order_flags[i], orders[i]);
if (!page)
continue;
return page;
@@ -423,6 +438,12 @@ static const struct dma_heap_ops system_heap_ops = {
static int system_heap_create(void)
{
struct dma_heap_export_info exp_info;
+ int i;
+
+ for (i = 0; i < NUM_ORDERS; i++) {
+ drm_page_pool_init(&pools[i], orders[i],
+ system_heap_free_pages);
+ }
exp_info.name = "system";
exp_info.ops = &system_heap_ops;
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v8 4/5] dma-buf: system_heap: Add drm pagepool support to system heap
@ 2021-03-04 23:20 ` John Stultz
0 siblings, 0 replies; 24+ messages in thread
From: John Stultz @ 2021-03-04 23:20 UTC (permalink / raw)
To: lkml
Cc: dri-devel, Sandeep Patil, Ezequiel Garcia, Robin Murphy,
James Jones, Liam Mark, Laura Abbott, Chris Goldsworthy,
Hridya Valsaraju, Ørjan Eide, linux-media,
Suren Baghdasaryan, Christian Koenig, Daniel Mentz
Utilize the drm pagepool code to speed up allocation
performance.
This is similar to the ION pagepool usage, but tries to
utilize generic code instead of a custom implementation.
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Christian Koenig <christian.koenig@amd.com>
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>
---
v2:
* Fix build issue caused by selecting PAGE_POOL w/o NET
as Reported-by: kernel test robot <lkp@intel.com>
v3:
* Simplify the page zeroing logic a bit by using kmap_atomic
instead of vmap as suggested by Daniel Mentz
v5:
* Shift away from networking page pool completely to
dmabuf page pool implementation
v6:
* Switch again to using the drm_page_pool code shared w/
ttm_pool
v7:
* Slight rework for drm_page_pool changes
v8:
* Rework to use the rewritten drm_page_pool logic
* Drop explicit buffer zeroing, as the drm_page_pool handles that
---
drivers/dma-buf/heaps/Kconfig | 1 +
drivers/dma-buf/heaps/system_heap.c | 27 ++++++++++++++++++++++++---
2 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
index f7aef8bc7119..7e28934e0def 100644
--- a/drivers/dma-buf/heaps/Kconfig
+++ b/drivers/dma-buf/heaps/Kconfig
@@ -4,6 +4,7 @@ config DMABUF_HEAPS_DEFERRED_FREE
config DMABUF_HEAPS_SYSTEM
bool "DMA-BUF System Heap"
depends on DMABUF_HEAPS
+ select DRM_PAGE_POOL
help
Choose this option to enable the system dmabuf heap. The system heap
is backed by pages from the buddy allocator. If in doubt, say Y.
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
index 29e49ac17251..006271881d85 100644
--- a/drivers/dma-buf/heaps/system_heap.c
+++ b/drivers/dma-buf/heaps/system_heap.c
@@ -21,6 +21,8 @@
#include <linux/slab.h>
#include <linux/vmalloc.h>
+#include <drm/page_pool.h>
+
static struct dma_heap *sys_heap;
struct system_heap_buffer {
@@ -53,6 +55,7 @@ static gfp_t order_flags[] = {HIGH_ORDER_GFP, LOW_ORDER_GFP, LOW_ORDER_GFP};
*/
static const unsigned int orders[] = {8, 4, 0};
#define NUM_ORDERS ARRAY_SIZE(orders)
+struct drm_page_pool pools[NUM_ORDERS];
static struct sg_table *dup_sg_table(struct sg_table *table)
{
@@ -281,18 +284,28 @@ static void system_heap_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map)
dma_buf_map_clear(map);
}
+static unsigned long system_heap_free_pages(struct drm_page_pool *pool, struct page *p)
+{
+ __free_pages(p, pool->order);
+ return 1 << pool->order;
+}
+
static void system_heap_dma_buf_release(struct dma_buf *dmabuf)
{
struct system_heap_buffer *buffer = dmabuf->priv;
struct sg_table *table;
struct scatterlist *sg;
- int i;
+ int i, j;
table = &buffer->sg_table;
for_each_sg(table->sgl, sg, table->nents, i) {
struct page *page = sg_page(sg);
- __free_pages(page, compound_order(page));
+ for (j = 0; j < NUM_ORDERS; j++) {
+ if (compound_order(page) == orders[j])
+ break;
+ }
+ drm_page_pool_add(&pools[j], page);
}
sg_free_table(table);
kfree(buffer);
@@ -323,7 +336,9 @@ static struct page *alloc_largest_available(unsigned long size,
if (max_order < orders[i])
continue;
- page = alloc_pages(order_flags[i], orders[i]);
+ page = drm_page_pool_remove(&pools[i]);
+ if (!page)
+ page = alloc_pages(order_flags[i], orders[i]);
if (!page)
continue;
return page;
@@ -423,6 +438,12 @@ static const struct dma_heap_ops system_heap_ops = {
static int system_heap_create(void)
{
struct dma_heap_export_info exp_info;
+ int i;
+
+ for (i = 0; i < NUM_ORDERS; i++) {
+ drm_page_pool_init(&pools[i], orders[i],
+ system_heap_free_pages);
+ }
exp_info.name = "system";
exp_info.ops = &system_heap_ops;
--
2.25.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v8 4/5] dma-buf: system_heap: Add drm pagepool support to system heap
2021-03-04 23:20 ` John Stultz
(?)
@ 2021-03-05 0:55 ` kernel test robot
-1 siblings, 0 replies; 24+ messages in thread
From: kernel test robot @ 2021-03-05 0:55 UTC (permalink / raw)
To: John Stultz, lkml
Cc: John Stultz, Daniel Vetter, Christian Koenig, Sumit Semwal,
Liam Mark, Chris Goldsworthy, Laura Abbott, Brian Starkey,
Hridya Valsaraju
[-- Attachment #1: Type: text/plain, Size: 3713 bytes --]
Hi John,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.12-rc1]
[cannot apply to linux/master drm-intel/for-linux-next drm-tip/drm-tip]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/John-Stultz/Generic-page-pool-deferred-freeing-for-system-dmabuf-heap/20210305-072137
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 3cb60ee6323968b694208c4cbd56a7176396e931
config: openrisc-randconfig-p001-20210304 (attached as .config)
compiler: or1k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/6a9bf19a9ed9e5058a11a3e3217530fdf2675e0c
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review John-Stultz/Generic-page-pool-deferred-freeing-for-system-dmabuf-heap/20210305-072137
git checkout 6a9bf19a9ed9e5058a11a3e3217530fdf2675e0c
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=openrisc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
or1k-linux-ld: arch/openrisc/kernel/entry.o: in function `_external_irq_handler':
(.text+0x804): undefined reference to `printk'
(.text+0x804): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `printk'
or1k-linux-ld: drivers/dma-buf/heaps/system_heap.o: in function `system_heap_create':
>> system_heap.c:(.text+0x15c): undefined reference to `drm_page_pool_init'
system_heap.c:(.text+0x15c): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `drm_page_pool_init'
>> or1k-linux-ld: system_heap.c:(.text+0x16c): undefined reference to `drm_page_pool_init'
system_heap.c:(.text+0x16c): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `drm_page_pool_init'
or1k-linux-ld: system_heap.c:(.text+0x17c): undefined reference to `drm_page_pool_init'
system_heap.c:(.text+0x17c): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `drm_page_pool_init'
or1k-linux-ld: drivers/dma-buf/heaps/system_heap.o: in function `system_heap_dma_buf_release':
>> system_heap.c:(.text+0xf78): undefined reference to `drm_page_pool_add'
system_heap.c:(.text+0xf78): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `drm_page_pool_add'
or1k-linux-ld: drivers/dma-buf/heaps/system_heap.o: in function `system_heap_allocate':
>> system_heap.c:(.text+0x11f8): undefined reference to `drm_page_pool_remove'
system_heap.c:(.text+0x11f8): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `drm_page_pool_remove'
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for DRM_PAGE_POOL
Depends on HAS_IOMEM && DRM
Selected by
- DMABUF_HEAPS_SYSTEM && DMABUF_HEAPS
"cppcheck warnings: (new ones prefixed by >>)"
>> drivers/dma-buf/heaps/system_heap.c:290:2: warning: int result is returned as long value. If the return value is long to avoid loss of information, then you have loss of information. [truncLongCastReturn]
return 1 << pool->order;
^
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 17496 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v8 4/5] dma-buf: system_heap: Add drm pagepool support to system heap
2021-03-04 23:20 ` John Stultz
@ 2021-03-05 5:00 ` kernel test robot
-1 siblings, 0 replies; 24+ messages in thread
From: kernel test robot @ 2021-03-05 5:00 UTC (permalink / raw)
To: John Stultz, lkml
Cc: kbuild-all, John Stultz, Daniel Vetter, Christian Koenig,
Sumit Semwal, Liam Mark, Chris Goldsworthy, Laura Abbott,
Brian Starkey, Hridya Valsaraju
[-- Attachment #1: Type: text/plain, Size: 2652 bytes --]
Hi John,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.12-rc1 next-20210304]
[cannot apply to linux/master drm-intel/for-linux-next drm-tip/drm-tip]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/John-Stultz/Generic-page-pool-deferred-freeing-for-system-dmabuf-heap/20210305-072137
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 3cb60ee6323968b694208c4cbd56a7176396e931
config: ia64-randconfig-c003-20210305 (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/6a9bf19a9ed9e5058a11a3e3217530fdf2675e0c
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review John-Stultz/Generic-page-pool-deferred-freeing-for-system-dmabuf-heap/20210305-072137
git checkout 6a9bf19a9ed9e5058a11a3e3217530fdf2675e0c
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=ia64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
ia64-linux-ld: drivers/dma-buf/heaps/system_heap.o: in function `system_heap_create':
system_heap.c:(.text+0x1b2): undefined reference to `drm_page_pool_init'
ia64-linux-ld: system_heap.c:(.text+0x1e2): undefined reference to `drm_page_pool_init'
ia64-linux-ld: system_heap.c:(.text+0x212): undefined reference to `drm_page_pool_init'
ia64-linux-ld: drivers/dma-buf/heaps/system_heap.o: in function `system_heap_dma_buf_release':
system_heap.c:(.text+0x1312): undefined reference to `drm_page_pool_add'
>> ia64-linux-ld: system_heap.c:(.text+0x1432): undefined reference to `drm_page_pool_add'
ia64-linux-ld: drivers/dma-buf/heaps/system_heap.o: in function `system_heap_allocate':
system_heap.c:(.text+0x16e2): undefined reference to `drm_page_pool_remove'
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for DRM_PAGE_POOL
Depends on HAS_IOMEM && DRM
Selected by
- DMABUF_HEAPS_SYSTEM && DMABUF_HEAPS
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30075 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v8 5/5] dma-buf: system_heap: Add deferred freeing to the system heap
2021-03-04 23:20 ` John Stultz
@ 2021-03-04 23:20 ` John Stultz
-1 siblings, 0 replies; 24+ messages in thread
From: John Stultz @ 2021-03-04 23:20 UTC (permalink / raw)
To: lkml
Cc: John Stultz, Daniel Vetter, Christian Koenig, Sumit Semwal,
Liam Mark, Chris Goldsworthy, Laura Abbott, Brian Starkey,
Hridya Valsaraju, Suren Baghdasaryan, Sandeep Patil,
Daniel Mentz, Ørjan Eide, Robin Murphy, Ezequiel Garcia,
Simon Ser, James Jones, linux-media, dri-devel
Utilize the deferred free helper library in the system heap.
This provides a nice performance bump and puts the
system heap performance on par with ION.
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Christian Koenig <christian.koenig@amd.com>
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>
---
v2:
* Rework deferred-free api to use reason enum as suggested by
Suren Baghdasaryan
* Rework for deferred-free api change to use nr_pages rather
than size as suggsted by Suren Baghdasaryan
v8:
* Reworked to drop buffer zeroing logic, as the drm_page_pool now
handles that.
---
drivers/dma-buf/heaps/Kconfig | 1 +
drivers/dma-buf/heaps/system_heap.c | 28 ++++++++++++++++++++++------
2 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
index 7e28934e0def..10632ccfb4a5 100644
--- a/drivers/dma-buf/heaps/Kconfig
+++ b/drivers/dma-buf/heaps/Kconfig
@@ -5,6 +5,7 @@ config DMABUF_HEAPS_SYSTEM
bool "DMA-BUF System Heap"
depends on DMABUF_HEAPS
select DRM_PAGE_POOL
+ select DMABUF_HEAPS_DEFERRED_FREE
help
Choose this option to enable the system dmabuf heap. The system heap
is backed by pages from the buddy allocator. If in doubt, say Y.
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
index 006271881d85..c753c82fd9f1 100644
--- a/drivers/dma-buf/heaps/system_heap.c
+++ b/drivers/dma-buf/heaps/system_heap.c
@@ -22,6 +22,7 @@
#include <linux/vmalloc.h>
#include <drm/page_pool.h>
+#include "deferred-free-helper.h"
static struct dma_heap *sys_heap;
@@ -33,6 +34,7 @@ struct system_heap_buffer {
struct sg_table sg_table;
int vmap_cnt;
void *vaddr;
+ struct deferred_freelist_item deferred_free;
};
struct dma_heap_attachment {
@@ -290,27 +292,41 @@ static unsigned long system_heap_free_pages(struct drm_page_pool *pool, struct p
return 1 << pool->order;
}
-static void system_heap_dma_buf_release(struct dma_buf *dmabuf)
+static void system_heap_buf_free(struct deferred_freelist_item *item,
+ enum df_reason reason)
{
- struct system_heap_buffer *buffer = dmabuf->priv;
+ struct system_heap_buffer *buffer;
struct sg_table *table;
struct scatterlist *sg;
int i, j;
+ buffer = container_of(item, struct system_heap_buffer, deferred_free);
table = &buffer->sg_table;
for_each_sg(table->sgl, sg, table->nents, i) {
struct page *page = sg_page(sg);
- for (j = 0; j < NUM_ORDERS; j++) {
- if (compound_order(page) == orders[j])
- break;
+ if (reason == DF_UNDER_PRESSURE) {
+ __free_pages(page, compound_order(page));
+ } else {
+ for (j = 0; j < NUM_ORDERS; j++) {
+ if (compound_order(page) == orders[j])
+ break;
+ }
+ drm_page_pool_add(&pools[j], page);
}
- drm_page_pool_add(&pools[j], page);
}
sg_free_table(table);
kfree(buffer);
}
+static void system_heap_dma_buf_release(struct dma_buf *dmabuf)
+{
+ struct system_heap_buffer *buffer = dmabuf->priv;
+ int npages = PAGE_ALIGN(buffer->len) / PAGE_SIZE;
+
+ deferred_free(&buffer->deferred_free, system_heap_buf_free, npages);
+}
+
static const struct dma_buf_ops system_heap_buf_ops = {
.attach = system_heap_attach,
.detach = system_heap_detach,
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v8 5/5] dma-buf: system_heap: Add deferred freeing to the system heap
@ 2021-03-04 23:20 ` John Stultz
0 siblings, 0 replies; 24+ messages in thread
From: John Stultz @ 2021-03-04 23:20 UTC (permalink / raw)
To: lkml
Cc: dri-devel, Sandeep Patil, Ezequiel Garcia, Robin Murphy,
James Jones, Liam Mark, Laura Abbott, Chris Goldsworthy,
Hridya Valsaraju, Ørjan Eide, linux-media,
Suren Baghdasaryan, Christian Koenig, Daniel Mentz
Utilize the deferred free helper library in the system heap.
This provides a nice performance bump and puts the
system heap performance on par with ION.
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Christian Koenig <christian.koenig@amd.com>
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>
---
v2:
* Rework deferred-free api to use reason enum as suggested by
Suren Baghdasaryan
* Rework for deferred-free api change to use nr_pages rather
than size as suggsted by Suren Baghdasaryan
v8:
* Reworked to drop buffer zeroing logic, as the drm_page_pool now
handles that.
---
drivers/dma-buf/heaps/Kconfig | 1 +
drivers/dma-buf/heaps/system_heap.c | 28 ++++++++++++++++++++++------
2 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
index 7e28934e0def..10632ccfb4a5 100644
--- a/drivers/dma-buf/heaps/Kconfig
+++ b/drivers/dma-buf/heaps/Kconfig
@@ -5,6 +5,7 @@ config DMABUF_HEAPS_SYSTEM
bool "DMA-BUF System Heap"
depends on DMABUF_HEAPS
select DRM_PAGE_POOL
+ select DMABUF_HEAPS_DEFERRED_FREE
help
Choose this option to enable the system dmabuf heap. The system heap
is backed by pages from the buddy allocator. If in doubt, say Y.
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
index 006271881d85..c753c82fd9f1 100644
--- a/drivers/dma-buf/heaps/system_heap.c
+++ b/drivers/dma-buf/heaps/system_heap.c
@@ -22,6 +22,7 @@
#include <linux/vmalloc.h>
#include <drm/page_pool.h>
+#include "deferred-free-helper.h"
static struct dma_heap *sys_heap;
@@ -33,6 +34,7 @@ struct system_heap_buffer {
struct sg_table sg_table;
int vmap_cnt;
void *vaddr;
+ struct deferred_freelist_item deferred_free;
};
struct dma_heap_attachment {
@@ -290,27 +292,41 @@ static unsigned long system_heap_free_pages(struct drm_page_pool *pool, struct p
return 1 << pool->order;
}
-static void system_heap_dma_buf_release(struct dma_buf *dmabuf)
+static void system_heap_buf_free(struct deferred_freelist_item *item,
+ enum df_reason reason)
{
- struct system_heap_buffer *buffer = dmabuf->priv;
+ struct system_heap_buffer *buffer;
struct sg_table *table;
struct scatterlist *sg;
int i, j;
+ buffer = container_of(item, struct system_heap_buffer, deferred_free);
table = &buffer->sg_table;
for_each_sg(table->sgl, sg, table->nents, i) {
struct page *page = sg_page(sg);
- for (j = 0; j < NUM_ORDERS; j++) {
- if (compound_order(page) == orders[j])
- break;
+ if (reason == DF_UNDER_PRESSURE) {
+ __free_pages(page, compound_order(page));
+ } else {
+ for (j = 0; j < NUM_ORDERS; j++) {
+ if (compound_order(page) == orders[j])
+ break;
+ }
+ drm_page_pool_add(&pools[j], page);
}
- drm_page_pool_add(&pools[j], page);
}
sg_free_table(table);
kfree(buffer);
}
+static void system_heap_dma_buf_release(struct dma_buf *dmabuf)
+{
+ struct system_heap_buffer *buffer = dmabuf->priv;
+ int npages = PAGE_ALIGN(buffer->len) / PAGE_SIZE;
+
+ deferred_free(&buffer->deferred_free, system_heap_buf_free, npages);
+}
+
static const struct dma_buf_ops system_heap_buf_ops = {
.attach = system_heap_attach,
.detach = system_heap_detach,
--
2.25.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 24+ messages in thread