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@amd.com --- 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 @@ -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; }
Dear Arunpravin,
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@amd.com
Please use your full name.
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?
@@ -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; }
Kind regards,
Paul
On 15/03/22 1:49 pm, Paul Menzel wrote:
Dear Arunpravin,
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@amd.com
Please use your full name.
okay
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://cgit.freedesktop.org/drm-tip/tree/ drm-misc-next - https://cgit.freedesktop.org/drm/drm-misc/tree/
@@ -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; }Kind regards,
Paul
Dear Arunpravin,
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@amd.com
Please use your full name.
okay
You might also configure that in your email program.
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://cgit.freedesktop.org/drm-tip/tree/ drm-misc-next - https://cgit.freedesktop.org/drm/drm-misc/tree/
@@ -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);
Please give more details.
Kind regards,
Paul
On 15/03/22 2:35 pm, Paul Menzel wrote:
Dear Arunpravin,
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@amd.com
Please use your full name.
okay
You might also configure that in your email program.
yes
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.freed... drm-misc-next - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgit.freed...
@@ -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.
Thanks, Arun
Kind regards,
Paul
Dear Arunpravin,
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@amd.com
Please use your full name.
okay
You might also configure that in your email program.
yes
Not done yet though. ;-)
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.freed... drm-misc-next - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgit.freed...
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.
Kind regards,
Paul
On 15/03/22 9:14 pm, Paul Menzel wrote:
Dear Arunpravin,
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@amd.com
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.freed... drm-misc-next - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgit.freed...
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.
Regards, Arun
Kind regards,
Paul
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@amd.com
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.freed... drm-misc-next - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgit.freed...
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.
Kind regards,
Paul
PS: The commit message summary of your v2 should also be updated.
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@amd.com > > 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.freed... drm-misc-next - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgit.freed...
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.
On 14/03/2022 19:40, Arunpravin wrote:
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@amd.com
Hmm, it sounded like we were instead going to go with the round_up(size, min_page_size), or check and bail if the size is misaligned, in which case we don't need this, AFAICT.
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 @@ -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; }
dri-devel@lists.freedesktop.org
-
Arunpravin -
Arunpravin
-
Arunpravin Paneer Selvam
-
Matthew Auld -
Paul Menzel