[Patch v2] drm/ttm: Allow direct reclaim to allocate local memory

[Christian König]

Am 12.07.24 um 15:40 schrieb Vlastimil Babka:
> On 7/8/24 6:06 PM, Rajneesh Bhardwaj wrote:
>> Limiting the allocation of higher order pages to the closest NUMA node
>> and enabling direct memory reclaim provides not only failsafe against
>> situations when memory becomes too much fragmented and the allocator is
>> not able to satisfy the request from the local node but falls back to
>> remote pages (HUGEPAGE) but also offers performance improvement.
>> Accessing remote pages suffers due to bandwidth limitations and could be
>> avoided if memory becomes defragmented and in most cases without using
>> manual compaction. (/proc/sys/vm/compact_memory)
>>
>> Note: On certain distros such as RHEL, the proactive compaction is
>> disabled. (https://tinyurl.com/4f32f7rs)
>>
>> Cc: Dave Airlie <airlied at redhat.com>
>> Cc: Vlastimil Babka <vbabka at suse.cz>
>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Reviewed-by: Christian König <christian.koenig at amd.com>
>> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj at amd.com>
>> ---
>>   drivers/gpu/drm/ttm/ttm_pool.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
>> index 6e1fd6985ffc..cc27d5c7afe8 100644
>> --- a/drivers/gpu/drm/ttm/ttm_pool.c
>> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
>> @@ -91,7 +91,7 @@ static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t gfp_flags,
>>   	 */
>>   	if (order)
>>   		gfp_flags |= __GFP_NOMEMALLOC | __GFP_NORETRY | __GFP_NOWARN |
>> -			__GFP_KSWAPD_RECLAIM;
>> +			__GFP_RECLAIM | __GFP_THISNODE;
> __GFP_RECLAIM includes ___GFP_DIRECT_RECLAIM, what if the caller is a
> context that can't sleep?

That's unproblematic, all callers should be able to sleep.

>   Then it would be a bugy to add that.
> OTOH the only caller I see is ttm_pool_alloc() which seems to start with
> GFP_USER and that already includes __GFP_RECLAIM, so either way I see no
> reason to be adding it here, other than it might be a potential
> bug in case other callers are added later and have more restricted context.

Ah! Good point, I was already wondering why changing that didn't had any 
effect.

In this case we should probably just drop __GFP_KSWAPD_RECLAIM as well 
or stop using GFP_USER as base.

> As for __GFP_THISNODE addition that should be fine as this seems to be an
> opportunistic allocation with a fallback that's decreasing the attempted order.

That's what we wanted to hear, thanks!

>
> Also I note that the caller might be adding __GFP_RETRY_MAYFAIL
>
>          if (ctx->gfp_retry_mayfail)
>                  gfp_flags |= __GFP_RETRY_MAYFAIL;
>
> But here adding __GFP_NORETRY makes __GFP_RETRY_MAYFAIL non-effective as
> __alloc_pages_slowpath() evaluates __GFP_NORETRY earlier to decide to give
> up, than evaluating __GFP_RETRY_MAYFAIL to decide to try a bit harder.

That's expected, __GFP_RETRY_MAYFAIL is just relevant for the order==0 
case when we don't add any additional flags.

We could drop that as well since I don't think anybody uses that any more.

Thanks,
Christian.

>
> That's not introduced by this patch but maybe something to look into, if
> __GFP_RETRY_MAYFAIL is expected to be useful here for trying harder. If it's
> instead intended only for the final fallback order-0 case where the
> gfp_flags adjustment above doesn't apply, then __GFP_RETRY_MAYFAIL will
> cause the allocation to fail instead of applying the infamous implicit "too
> small to fail" for order-0 allocation and invoking OOM. If that's the reason
> for it to be used here, all is fine and great.
>
> Vlastimil
>
>>   
>>   	if (!pool->use_dma_alloc) {
>>   		p = alloc_pages_node(pool->nid, gfp_flags, order);