Message-ID: <ab35ed32-ead4-3dc4-550d-55f288810220@amd.com>
Date:   Wed, 30 Jun 2021 11:10:31 +0200
From:   Christian König <christian.koenig@....com>
To:     John Stultz <john.stultz@...aro.org>,
        lkml <linux-kernel@...r.kernel.org>
Cc:     Daniel Vetter <daniel@...ll.ch>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Liam Mark <lmark@...eaurora.org>,
        Chris Goldsworthy <cgoldswo@...eaurora.org>,
        Laura Abbott <labbott@...nel.org>,
        Brian Starkey <Brian.Starkey@....com>,
        Hridya Valsaraju <hridya@...gle.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Sandeep Patil <sspatil@...gle.com>,
        Daniel Mentz <danielmentz@...gle.com>,
        Ørjan Eide <orjan.eide@....com>,
        Robin Murphy <robin.murphy@....com>,
        Ezequiel Garcia <ezequiel@...labora.com>,
        Simon Ser <contact@...rsion.fr>,
        James Jones <jajones@...dia.com>, linux-media@...r.kernel.org,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v9 1/5] drm: Add a sharable drm page-pool implementation

Am 30.06.21 um 03:34 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.
>
> Cc: Daniel Vetter <daniel@...ll.ch>
> Cc: Christian Koenig <christian.koenig@....com>
> Cc: Sumit Semwal <sumit.semwal@...aro.org>
> Cc: Liam Mark <lmark@...eaurora.org>
> Cc: Chris Goldsworthy <cgoldswo@...eaurora.org>
> Cc: Laura Abbott <labbott@...nel.org>
> Cc: Brian Starkey <Brian.Starkey@....com>
> Cc: Hridya Valsaraju <hridya@...gle.com>
> Cc: Suren Baghdasaryan <surenb@...gle.com>
> Cc: Sandeep Patil <sspatil@...gle.com>
> Cc: Daniel Mentz <danielmentz@...gle.com>
> Cc: Ørjan Eide <orjan.eide@....com>
> Cc: Robin Murphy <robin.murphy@....com>
> Cc: Ezequiel Garcia <ezequiel@...labora.com>
> Cc: Simon Ser <contact@...rsion.fr>
> Cc: James Jones <jajones@...dia.com>
> Cc: linux-media@...r.kernel.org
> Cc: dri-devel@...ts.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@...aro.org>
> ---
> v8:
> * Completely rewritten from scratch, using only the
>    ttm_pool logic so it can be dual licensed.
> v9:
> * Add Kerneldoc comments similar to tmm implementation
>    as suggested by ChristianK
> * Mark some functions static as suggested by ChristianK
> * Fix locking issue ChristianK pointed out
> * Add methods to block the shrinker so users can
>    make atomic calculations across multiple pools, as
>    suggested by ChristianK
> * Fix up Kconfig dependency issue as Reported-by:
>    kernel test robot <lkp@...el.com>
> ---
>   drivers/gpu/drm/Kconfig     |   3 +
>   drivers/gpu/drm/Makefile    |   2 +
>   drivers/gpu/drm/page_pool.c | 297 ++++++++++++++++++++++++++++++++++++
>   include/drm/page_pool.h     |  68 +++++++++
>   4 files changed, 370 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 3c16bd1afd87..52d9ba92b35e 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -177,6 +177,9 @@ 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
> +
>   config DRM_TTM
>   	tristate
>   	depends on DRM && MMU
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 5279db4392df..affa4ca3a08e 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..c07bbe3afc32
> --- /dev/null
> +++ b/drivers/gpu/drm/page_pool.c
> @@ -0,0 +1,297 @@
> +// 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; /* max size of the pool */
> +
> +MODULE_PARM_DESC(page_pool_size, "Number of pages in the drm page pool");
> +module_param(page_pool_size, ulong, 0644);
> +
> +static atomic_long_t nr_managed_pages;
> +
> +static struct mutex shrinker_lock;
> +static struct list_head shrinker_list;
> +static struct shrinker mm_shrinker;
> +
> +/**
> + * drm_page_pool_set_max - Sets maximum size of all pools
> + *
> + * Sets the maximum number of pages allows in all pools.
> + * This can only be set once, and the first caller wins.
> + */
> +void drm_page_pool_set_max(unsigned long max)
> +{
> +	if (!page_pool_size)
> +		page_pool_size = max;
> +}
> +
> +/**
> + * drm_page_pool_get_max - Maximum size of all pools
> + *
> + * Return the maximum number of pages allows in all pools
> + */
> +unsigned long drm_page_pool_get_max(void)
> +{
> +	return page_pool_size;
> +}

Well in general I don't think it is a good idea to have getters/setters 
for one line functionality, similar applies to locking/unlocking the 
mutex below.

Then in this specific case what those functions do is to aid 
initializing the general pool manager and that in turn should absolutely 
not be exposed.

The TTM pool manager exposes this as function because initializing the 
pool manager is done in one part of the module and calculating the 
default value for the pages in another one. But that is not something I 
would like to see here.

> +
> +/**
> + * drm_page_pool_get_total - Current size of all pools
> + *
> + * Return the number of pages in all managed pools
> + */
> +unsigned long drm_page_pool_get_total(void)
> +{
> +	return atomic_long_read(&nr_managed_pages);
> +}
> +
> +/**
> + * drm_page_pool_get_size - Get the number of pages in the pool
> + *
> + * @pool: Pool to calculate the size of
> + *
> + * Return the number of pages in the specified pool
> + */
> +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;
> +}
> +
> +/**
> + * drm_page_pool_add - Add a page to a pool
> + *
> + * @pool: Pool to add page to
> + * @page: Page to be added to the pool
> + *
> + * Gives specified page 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;
> +
> +	/* Make sure we won't grow larger then the max pool size */
> +	if (page_pool_size &&
> +	       ((drm_page_pool_get_total()) + num_pages > page_pool_size)) {
> +		pool->free(pool, p);
> +		return;
> +	}

That is not a good idea. See how ttm_pool_free() does this.

First the page is given back to the pool, then all pools are shrinked if 
they are above the global limit.

Regards,
Christian.

> +
> +	/* Be sure to zero pages before adding them to the pool */
> +	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, &nr_managed_pages);
> +
> +}
> +
> +/**
> + * drm_page_pool_remove - Remove page from pool
> + *
> + * @pool: Pool to pull the page from
> + *
> + * 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, &nr_managed_pages);
> +		pool->page_count -= 1 << pool->order;
> +		list_del(&p->lru);
> +	}
> +	spin_unlock(&pool->lock);
> +
> +	return p;
> +}
> +
> +/**
> + * drm_page_pool_init - Initialize a pool
> + *
> + * @pool: the pool to initialize
> + * @order: page allocation order
> + * @free_page: function pointer to free the pool's pages
> + *
> + * Initialize and add a pool type to the global shrinker list
> + */
> +void drm_page_pool_init(struct drm_page_pool *pool, unsigned int order,
> +			void (*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);
> +}
> +
> +/**
> + * drm_page_pool_fini - Cleanup a pool
> + *
> + * @pool: the pool to clean up
> + *
> + * 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;
> +
> +	mutex_lock(&shrinker_lock);
> +	list_del(&pool->shrinker_list);
> +	mutex_unlock(&shrinker_lock);
> +
> +	while ((p = drm_page_pool_remove(pool)))
> +		pool->free(pool, p);
> +}
> +
> +/**
> + * drm_page_pool_shrink - Shrink the drm page pool
> + *
> + * Free pages using the global shrinker list. Returns
> + * the number of pages free
> + */
> +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);
> +	if (p) {
> +		pool->free(pool, p);
> +		num_freed = 1 << pool->order;
> +	} else {
> +		num_freed = 0;
> +	}
> +
> +	list_move_tail(&pool->shrinker_list, &shrinker_list);
> +	mutex_unlock(&shrinker_lock);
> +
> +	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(&nr_managed_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(&nr_managed_pages);
> +
> +	return num_pages ? num_pages : SHRINK_EMPTY;
> +}
> +
> +/**
> + * dma_page_pool_lock_shrinker - Take the shrinker lock
> + *
> + * Takes the shrinker lock, preventing the shrinker from making
> + * changes to the pools
> + */
> +void dma_page_pool_lock_shrinker(void)
> +{
> +	mutex_lock(&shrinker_lock);
> +}
> +
> +/**
> + * dma_page_pool_unlock_shrinker - Release the shrinker lock
> + *
> + * Releases the shrinker lock, allowing the shrinker to free
> + * pages
> + */
> +void dma_page_pool_unlock_shrinker(void)
> +{
> +	mutex_unlock(&shrinker_lock);
> +}
> +
> +/**
> + * drm_page_pool_shrinker_init - Initialize globals
> + *
> + * Initialize the global locks and lists for the shrinker.
> + */
> +static int drm_page_pool_shrinker_init(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_fini - Finalize globals
> + *
> + * Unregister the shrinker.
> + */
> +static void drm_page_pool_shrinker_fini(void)
> +{
> +	unregister_shrinker(&mm_shrinker);
> +	WARN_ON(!list_empty(&shrinker_list));
> +}
> +
> +module_init(drm_page_pool_shrinker_init);
> +module_exit(drm_page_pool_shrinker_fini);
> +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..860cf6c92aab
> --- /dev/null
> +++ b/include/drm/page_pool.h
> @@ -0,0 +1,68 @@
> +/* 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;
> +	void (*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 int drm_page_pool_shrink(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 dma_page_pool_lock_shrinker(void);
> +void dma_page_pool_unlock_shrinker(void);
> +void drm_page_pool_init(struct drm_page_pool *pool, unsigned int order,
> +			void (*free_page)(struct drm_page_pool *pool, struct page *p));
> +void drm_page_pool_fini(struct drm_page_pool *pool);
> +
> +#endif

Powered by blists - more mailing lists