Re: [PATCH v9 1/5] drm: Add a sharable drm page-pool implementation

["Christian König" <christian.koenig@amd.com>]

Am 01.07.21 um 00:24 schrieb John Stultz:
> On Wed, Jun 30, 2021 at 2:10 AM Christian König
> <christian.koenig@amd.com> wrote:
>> Am 30.06.21 um 03:34 schrieb John Stultz:
>>> +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.
> So, I guess I'm not quite clear on what you'd like to see...
>
> Part of what I'm balancing here is the TTM subsystem normally sets a
> global max size, whereas the old ION pool didn't have caps (instead
> just relying on the shrinker when needed).
> So I'm trying to come up with a solution that can serve both uses. So
> I've got this drm_page_pool_set_max() function to optionally set the
> maximum value, which is called in the TTM initialization path or set
> the boot argument. But for systems that use the dmabuf system heap,
> but don't use TTM, no global limit is enforced.

Yeah, exactly that's what I'm trying to prevent.

See if we have the same functionality used by different use cases we 
should not have different behavior depending on what drivers are loaded.

Is it a problem if we restrict the ION pool to 50% of system memory as 
well? If yes than I would rather drop the limit from TTM and only rely 
on the shrinker there as well.

> Your earlier suggestion to have it as an argument to the
> drm_page_pool_shrinker_init() didn't make much sense to me, as then we
> may have multiple subsystems trying to initialize the pool shrinker,
> which doesn't seem ideal. So I'm not sure what would be preferred.
>
> I guess we could have subsystems allocate their own pool managers with
> their own shrinkers and per-manager-size-limits? But that also feels
> like a fair increase in complexity, for I'm not sure how much benefit.
>
>>> +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.
> Ok, initially my thought was it seemed like wasteful overhead to add
> the page to the pool and then shrink the pool, so just freeing the
> given page directly if the pool was full seemed more straightforward.
> But on consideration here I do realize having most-recently-used hot
> pages in the pool would be good for performance, so I'll switch that
> back. Thanks for pointing this out!

The even bigger problem is that you then always drop pages from the 
active pools.

E.g. a pool which just allocated and then freed 2MiB during driver load 
for some firmware upload will never see pressure if you do it this way.

So those 2MiB would never be recycled while they could be well used in 
one of the active pools.

Regards,
Christian.

>
> Thanks again so much for the review and feedback!
> -john