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