Re: [PATCH] drm: remove min_order BUG_ON check

[Arunpravin <arunpravin.paneerselvam@amd.com>]

On 07/03/22 10:11 pm, Matthew Auld wrote:
> On 07/03/2022 14:37, Arunpravin wrote:
>> place BUG_ON(order < min_order) outside do..while
>> loop as it fails Unigine Heaven benchmark.
>>
>> Unigine Heaven has buffer allocation requests for
>> example required pages are 161 and alignment request
>> is 128. To allocate the remaining 33 pages, continues
>> the iteration to find the order value which is 5 and
>> when it compares with min_order = 7, enables the
>> BUG_ON(). To avoid this problem, placed the BUG_ON
>> check outside of do..while loop.
>>
>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
>> ---
>>   drivers/gpu/drm/drm_buddy.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>> index 72f52f293249..ed94c56b720f 100644
>> --- a/drivers/gpu/drm/drm_buddy.c
>> +++ b/drivers/gpu/drm/drm_buddy.c
>> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>   	order = fls(pages) - 1;
>>   	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>>   
>> +	BUG_ON(order < min_order);
> 
> Isn't the issue that we are allowing a size that is not aligned to the 
> requested min_page_size? Should we not fix the caller(and throw a normal 
> error here), or perhaps add the round_up() here instead?
> 
CASE 1:
when size is not aligned to the requested min_page_size, for instance,
required size = 161 pages, min_page_size = 128 pages, here we have 3
possible options,
a. AFAIK,This kind of situation is common in any workload,the first
allocation (i.e) 128 pages is aligned to min_page_size, Should we just
allocate the left over 33 pages (2 pow 5, 2 pow 0) since the caller does
know the left over pages are not in min_page_size alignment?

b. There are many such instances in unigine heaven workload (there would
be many such workloads), throwing a normal error would lower the FPS? is
it possible to fix at caller application?

c. adding the round_up() is possible, but in every such instances we end
up allocating extra unused memory. For example, if required pages = 1028
and min_page_size = 1024 pages, we end up round up of left over 4 pages
to the min_page_size, so the total size would be 2048 pages.

> i.e if someone does:
> 
> alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags);
CASE 2:
I think this case should be detected (i.e) when min_page_size > size,
should we return -EINVAL?
> 
> This will still trigger the BUG_ON() even if we move it out of the loop, 
> AFAICT.
> 

Should we just allow the CASE 1 proceed for the allocation and return
-EINVAL for the CASE 2?

>> +
>>   	do {
>>   		order = min(order, (unsigned int)fls(pages) - 1);
>>   		BUG_ON(order > mm->max_order);
>> -		BUG_ON(order < min_order);
>>   
>>   		do {
>>   			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>>
>> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b