On 16/03/22 12:28 pm, Paul Menzel wrote: > Dear Arunprivin, > > > Am 16.03.22 um 07:49 schrieb Arunpravin Paneer Selvam: > >> On 15/03/22 9:14 pm, Paul Menzel wrote: > >>> Am 15.03.22 um 16:42 schrieb Arunpravin: >>> >>>> On 15/03/22 2:35 pm, Paul Menzel wrote: >>> >>>>> Am 15.03.22 um 10:01 schrieb Arunpravin: >>>>> >>>>>> On 15/03/22 1:49 pm, Paul Menzel wrote: >>>>> >>>>>>> Am 14.03.22 um 20:40 schrieb Arunpravin: >>>>>>>> handle a situation in the condition order-- == min_order, >>>>>>>> when order = 0, leading to order = -1, it now won't exit >>>>>>>> the loop. To avoid this problem, added a order check in >>>>>>>> the same condition, (i.e) when order is 0, we return >>>>>>>> -ENOSPC >>>>>>>> >>>>>>>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@xxxxxxx> >>>>>>> >>>>>>> Please use your full name. >>>>>> okay >>>>> >>>>> You might also configure that in your email program. >>>> yes >>> >>> Not done yet though. ;-) >>> >> done in v2 :) >>>>>>>> --- >>>>>>>> drivers/gpu/drm/drm_buddy.c | 2 +- >>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c >>>>>>>> index 72f52f293249..5ab66aaf2bbd 100644 >>>>>>>> --- a/drivers/gpu/drm/drm_buddy.c >>>>>>>> +++ b/drivers/gpu/drm/drm_buddy.c >>>>>>> >>>>>>> In what tree is that file? >>>>>>> >>>>>> drm-tip - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgit.freedesktop.org%2Fdrm-tip%2Ftree%2F&data=04%7C01%7Carunpravin.paneerselvam%40amd.com%7C439b31d360ef495ab13408da071a6e1f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637830107357395422%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Z8KNmbUXmhk0xA8z7yHJN2j%2BRJ5VwpuMXww21mrC8x8%3D&reserved=0 >>>>>> drm-misc-next - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgit.freedesktop.org%2Fdrm%2Fdrm-misc%2Ftree%2F&data=04%7C01%7Carunpravin.paneerselvam%40amd.com%7C439b31d360ef495ab13408da071a6e1f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637830107357395422%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Mwqy6NVTiR%2FoHFpLvXnQdE95kHoJJUEiig0Juz37ATQ%3D&reserved=0 >>> >>> Thank Outlook. Now everybody feels safe. >>> >>>>>>>> @@ -685,7 +685,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, >>>>>>>> if (!IS_ERR(block)) >>>>>>>> break; >>>>>>>> >>>>>>>> - if (order-- == min_order) { >>>>>>>> + if (!order || order-- == min_order) { >>>>>>>> err = -ENOSPC; >>>>>>>> goto err_free; >>>>>>>> } >>>>> >>>>> Thank you for the hint. So the whole function is: >>>>> >>>>> 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) >>>>> /* Allocate traversing within the range */ >>>>> block = alloc_range_bias(mm, start, end, order); >>>>> else >>>>> /* Allocate from freelist */ >>>>> block = alloc_from_freelist(mm, order, flags); >>>>> >>>>> if (!IS_ERR(block)) >>>>> break; >>>>> >>>>> if (order-- == min_order) { >>>>> err = -ENOSPC; >>>>> goto err_free; >>>>> } >>>>> } while (1); >>>>> >>>>> mark_allocated(block); >>>>> mm->avail -= drm_buddy_block_size(mm, block); >>>>> kmemleak_update_trace(block); >>>>> list_add_tail(&block->link, &allocated); >>>>> >>>>> pages -= BIT(order); >>>>> >>>>> if (!pages) >>>>> break; >>>>> } while (1); >>>>> >>>>> Was the BUG_ON triggered for your case? >>>>> >>>>> BUG_ON(order < min_order); >>>> no, this BUG_ON is not triggered for this bug >>>>> >>>>> Please give more details. >>>> >>>> there is a chance when there is no space to allocate, order value >>>> decrements and reaches to 0 at one point, here we should exit the loop, >>>> otherwise, further order value decrements to -1 and do..while loop >>>> doesn't exit. Hence added a check to exit the loop if order value becomes 0. >>> >>> Sorry, I do not see it. How can that be with order ≥ min_order and the >>> check `order-- == min_order`? Is min_order 0? Please explain that in the >>> next commit message. >>> >> please check v2, yes when min_order is 0, the above said situation may >> occur.And, since the order is unsigned int, I think it will not trigger >> the BUG_ON(order < min_order) when order becomes -1. Hence I think we >> needed a check !order to exit the loop. > > Thank you for clarifying this. I still do not understand it though. With > > order = fls(pages) - 1; > min_order = ilog2(min_page_size) - ilog2(mm->chunk_size); > > is zorder` always non-negative? Let’s assume it is. Also, can min_order > get “negative” (wraps around)? > > I would add BUG_ON statements for these cases? > > BUG_ON(fls(pages) - 1 < 1); > BUG_ON(ilog2(min_page_size) - ilog2(mm->chunk_size) < 1); > > Assuming “negative” is not possible, your case can only happen if > `order` and `min_order` are 0, right? If `order` is greater than 0, and > `min_order` is 0, the first BUG_ON in the while loop would be hit. If > `order` is 0 and `min_order` is greater than 0, everything should work > as the condition in `if (order-- == min_order)` is going to be true > eventually. > > Could you please analyze this more. The current patch looks more like > papering over something, or I am missing something. > Thanks for the analysis, Matthew suggested to add a simple check, I have sent the patch for the review. Regards, Arun > > Kind regards, > > Paul > > > PS: The commit message summary of your v2 should also be updated. >
