The drm buddy allocator tests were broken on 32-bit systems, as
rounddown_pow_of_two() takes a long, and the buddy allocator handles
64-bit sizes even on 32-bit systems.
This can be reproduced with the drm_buddy_allocator KUnit tests on i386:
./tools/testing/kunit/kunit.py run --arch i386 \
--kunitconfig ./drivers/gpu/drm/tests drm_buddy
(It results in kernel BUG_ON() when too many blocks are created, due to
the block size being too small.)
This was independently uncovered (and fixed) by Luís Mendes, whose patch
added a new u64 variant of rounddown_pow_of_two(). This version instead
recalculates the size based on the order.
Reported-by: Luís Mendes <[email protected]>
Link: https://lore.kernel.org/lkml/CAEzXK1oghXAB_KpKpm=-CviDQbNaH0qfgYTSSjZgvvyj4U78AA@mail.gmail.com/T/
Signed-off-by: David Gow <[email protected]>
---
drivers/gpu/drm/drm_buddy.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 3d1f50f481cf..7098f125b54a 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -146,8 +146,8 @@ int drm_buddy_init(struct drm_buddy *mm, u64 size, u64 chunk_size)
unsigned int order;
u64 root_size;
- root_size = rounddown_pow_of_two(size);
- order = ilog2(root_size) - ilog2(chunk_size);
+ order = ilog2(size) - ilog2(chunk_size);
+ root_size = chunk_size << order;
root = drm_block_alloc(mm, NULL, order, offset);
if (!root)
--
2.40.0.348.gf938b09366-goog
The drm_buddy_test KUnit tests verify that returned blocks have sizes
which are powers of two using is_power_of_2(). However, is_power_of_2()
operations on a 'long', but the block size is a u64. So on systems where
long is 32-bit, this can sometimes fail even on correctly sized blocks.
This only reproduces randomly, as the parameters passed to the buddy
allocator in this test are random. The seed 0xb2e06022 reproduced it
fine here.
For now, just hardcode an is_power_of_2() implementation using
x & (x - 1).
Signed-off-by: David Gow <[email protected]>
---
There are actually a couple of is_power_of_2_u64() implementations
already around in:
- drivers/gpu/drm/i915/i915_utils.h
- fs/btrfs/misc.h (called is_power_of_two_u64)
So the ideal thing would be to consolidate these in one place.
---
drivers/gpu/drm/tests/drm_buddy_test.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c b/drivers/gpu/drm/tests/drm_buddy_test.c
index f8ee714df396..09ee6f6af896 100644
--- a/drivers/gpu/drm/tests/drm_buddy_test.c
+++ b/drivers/gpu/drm/tests/drm_buddy_test.c
@@ -89,7 +89,8 @@ static int check_block(struct kunit *test, struct drm_buddy *mm,
err = -EINVAL;
}
- if (!is_power_of_2(block_size)) {
+ /* We can't use is_power_of_2() for a u64 on 32-bit systems. */
+ if (block_size & (block_size - 1)) {
kunit_err(test, "block size not power of two\n");
err = -EINVAL;
}
--
2.40.0.348.gf938b09366-goog
Am 29.03.23 um 08:55 schrieb David Gow:
> The drm buddy allocator tests were broken on 32-bit systems, as
> rounddown_pow_of_two() takes a long, and the buddy allocator handles
> 64-bit sizes even on 32-bit systems.
>
> This can be reproduced with the drm_buddy_allocator KUnit tests on i386:
> ./tools/testing/kunit/kunit.py run --arch i386 \
> --kunitconfig ./drivers/gpu/drm/tests drm_buddy
>
> (It results in kernel BUG_ON() when too many blocks are created, due to
> the block size being too small.)
>
> This was independently uncovered (and fixed) by Luís Mendes, whose patch
> added a new u64 variant of rounddown_pow_of_two(). This version instead
> recalculates the size based on the order.
>
> Reported-by: Luís Mendes <[email protected]>
> Link: https://lore.kernel.org/lkml/CAEzXK1oghXAB_KpKpm=-CviDQbNaH0qfgYTSSjZgvvyj4U78AA@mail.gmail.com/T/
> Signed-off-by: David Gow <[email protected]>
Acked-by: Christian König <[email protected]> for the series.
> ---
> drivers/gpu/drm/drm_buddy.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
> index 3d1f50f481cf..7098f125b54a 100644
> --- a/drivers/gpu/drm/drm_buddy.c
> +++ b/drivers/gpu/drm/drm_buddy.c
> @@ -146,8 +146,8 @@ int drm_buddy_init(struct drm_buddy *mm, u64 size, u64 chunk_size)
> unsigned int order;
> u64 root_size;
>
> - root_size = rounddown_pow_of_two(size);
> - order = ilog2(root_size) - ilog2(chunk_size);
> + order = ilog2(size) - ilog2(chunk_size);
> + root_size = chunk_size << order;
>
> root = drm_block_alloc(mm, NULL, order, offset);
> if (!root)
On Wed, 29 Mar 2023, David Gow <[email protected]> wrote:
> The drm_buddy_test KUnit tests verify that returned blocks have sizes
> which are powers of two using is_power_of_2(). However, is_power_of_2()
> operations on a 'long', but the block size is a u64. So on systems where
> long is 32-bit, this can sometimes fail even on correctly sized blocks.
>
> This only reproduces randomly, as the parameters passed to the buddy
> allocator in this test are random. The seed 0xb2e06022 reproduced it
> fine here.
>
> For now, just hardcode an is_power_of_2() implementation using
> x & (x - 1).
>
> Signed-off-by: David Gow <[email protected]>
> ---
>
> There are actually a couple of is_power_of_2_u64() implementations
> already around in:
> - drivers/gpu/drm/i915/i915_utils.h
> - fs/btrfs/misc.h (called is_power_of_two_u64)
>
> So the ideal thing would be to consolidate these in one place.
>
>
> ---
> drivers/gpu/drm/tests/drm_buddy_test.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c b/drivers/gpu/drm/tests/drm_buddy_test.c
> index f8ee714df396..09ee6f6af896 100644
> --- a/drivers/gpu/drm/tests/drm_buddy_test.c
> +++ b/drivers/gpu/drm/tests/drm_buddy_test.c
> @@ -89,7 +89,8 @@ static int check_block(struct kunit *test, struct drm_buddy *mm,
> err = -EINVAL;
> }
>
> - if (!is_power_of_2(block_size)) {
> + /* We can't use is_power_of_2() for a u64 on 32-bit systems. */
> + if (block_size & (block_size - 1)) {
Then maybe use is_power_of_2_u64() instead?
BR,
Jani.
> kunit_err(test, "block size not power of two\n");
> err = -EINVAL;
> }
--
Jani Nikula, Intel Open Source Graphics Center
On Wed, 29 Mar 2023, Jani Nikula <[email protected]> wrote:
> On Wed, 29 Mar 2023, David Gow <[email protected]> wrote:
>> The drm_buddy_test KUnit tests verify that returned blocks have sizes
>> which are powers of two using is_power_of_2(). However, is_power_of_2()
>> operations on a 'long', but the block size is a u64. So on systems where
>> long is 32-bit, this can sometimes fail even on correctly sized blocks.
>>
>> This only reproduces randomly, as the parameters passed to the buddy
>> allocator in this test are random. The seed 0xb2e06022 reproduced it
>> fine here.
>>
>> For now, just hardcode an is_power_of_2() implementation using
>> x & (x - 1).
>>
>> Signed-off-by: David Gow <[email protected]>
>> ---
>>
>> There are actually a couple of is_power_of_2_u64() implementations
>> already around in:
>> - drivers/gpu/drm/i915/i915_utils.h
>> - fs/btrfs/misc.h (called is_power_of_two_u64)
>>
>> So the ideal thing would be to consolidate these in one place.
>>
>>
>> ---
>> drivers/gpu/drm/tests/drm_buddy_test.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c b/drivers/gpu/drm/tests/drm_buddy_test.c
>> index f8ee714df396..09ee6f6af896 100644
>> --- a/drivers/gpu/drm/tests/drm_buddy_test.c
>> +++ b/drivers/gpu/drm/tests/drm_buddy_test.c
>> @@ -89,7 +89,8 @@ static int check_block(struct kunit *test, struct drm_buddy *mm,
>> err = -EINVAL;
>> }
>>
>> - if (!is_power_of_2(block_size)) {
>> + /* We can't use is_power_of_2() for a u64 on 32-bit systems. */
>> + if (block_size & (block_size - 1)) {
>
> Then maybe use is_power_of_2_u64() instead?
*sigh* And as soon as I wrote that I realized it's a local thing in
btrfs. Never mind for now...
...but in the long run would be nice to either fix is_power_of_2() for
u64 or add that u64 version...
BR,
Jani.
>
> BR,
> Jani.
>
>> kunit_err(test, "block size not power of two\n");
>> err = -EINVAL;
>> }
--
Jani Nikula, Intel Open Source Graphics Center
On 3/29/23 03:55, David Gow wrote:
> The drm_buddy_test KUnit tests verify that returned blocks have sizes
> which are powers of two using is_power_of_2(). However, is_power_of_2()
> operations on a 'long', but the block size is a u64. So on systems where
> long is 32-bit, this can sometimes fail even on correctly sized blocks.
>
> This only reproduces randomly, as the parameters passed to the buddy
> allocator in this test are random. The seed 0xb2e06022 reproduced it
> fine here.
>
> For now, just hardcode an is_power_of_2() implementation using
> x & (x - 1).
>
> Signed-off-by: David Gow <[email protected]>
As we still didn't consolidate an implementation of is_power_of_2_u64(),
Reviewed-by: Maíra Canal <[email protected]>
Best Regards,
- Maíra Canal
> ---
>
> There are actually a couple of is_power_of_2_u64() implementations
> already around in:
> - drivers/gpu/drm/i915/i915_utils.h
> - fs/btrfs/misc.h (called is_power_of_two_u64)
>
> So the ideal thing would be to consolidate these in one place.
>
>
> ---
> drivers/gpu/drm/tests/drm_buddy_test.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c b/drivers/gpu/drm/tests/drm_buddy_test.c
> index f8ee714df396..09ee6f6af896 100644
> --- a/drivers/gpu/drm/tests/drm_buddy_test.c
> +++ b/drivers/gpu/drm/tests/drm_buddy_test.c
> @@ -89,7 +89,8 @@ static int check_block(struct kunit *test, struct drm_buddy *mm,
> err = -EINVAL;
> }
>
> - if (!is_power_of_2(block_size)) {
> + /* We can't use is_power_of_2() for a u64 on 32-bit systems. */
> + if (block_size & (block_size - 1)) {
> kunit_err(test, "block size not power of two\n");
> err = -EINVAL;
> }
On Wed, 29 Mar 2023, Maíra Canal <[email protected]> wrote:
> On 3/29/23 03:55, David Gow wrote:
>> The drm_buddy_test KUnit tests verify that returned blocks have sizes
>> which are powers of two using is_power_of_2(). However, is_power_of_2()
>> operations on a 'long', but the block size is a u64. So on systems where
>> long is 32-bit, this can sometimes fail even on correctly sized blocks.
>>
>> This only reproduces randomly, as the parameters passed to the buddy
>> allocator in this test are random. The seed 0xb2e06022 reproduced it
>> fine here.
>>
>> For now, just hardcode an is_power_of_2() implementation using
>> x & (x - 1).
>>
>> Signed-off-by: David Gow <[email protected]>
>
> As we still didn't consolidate an implementation of is_power_of_2_u64(),
I just cooked up some patches to try to make is_power_of_2() more
flexible. I only sent them to the "CI trybot" for a quick spin first,
will post to lkml later. [1]
BR,
Jani.
[1] https://patchwork.freedesktop.org/series/115785/
>
> Reviewed-by: Maíra Canal <[email protected]>
>
> Best Regards,
> - Maíra Canal
>
>> ---
>>
>> There are actually a couple of is_power_of_2_u64() implementations
>> already around in:
>> - drivers/gpu/drm/i915/i915_utils.h
>> - fs/btrfs/misc.h (called is_power_of_two_u64)
>>
>> So the ideal thing would be to consolidate these in one place.
>>
>>
>> ---
>> drivers/gpu/drm/tests/drm_buddy_test.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c b/drivers/gpu/drm/tests/drm_buddy_test.c
>> index f8ee714df396..09ee6f6af896 100644
>> --- a/drivers/gpu/drm/tests/drm_buddy_test.c
>> +++ b/drivers/gpu/drm/tests/drm_buddy_test.c
>> @@ -89,7 +89,8 @@ static int check_block(struct kunit *test, struct drm_buddy *mm,
>> err = -EINVAL;
>> }
>>
>> - if (!is_power_of_2(block_size)) {
>> + /* We can't use is_power_of_2() for a u64 on 32-bit systems. */
>> + if (block_size & (block_size - 1)) {
>> kunit_err(test, "block size not power of two\n");
>> err = -EINVAL;
>> }
--
Jani Nikula, Intel Open Source Graphics Center
Am 29.03.23 um 13:28 schrieb Jani Nikula:
> On Wed, 29 Mar 2023, Maíra Canal <[email protected]> wrote:
>> On 3/29/23 03:55, David Gow wrote:
>>> The drm_buddy_test KUnit tests verify that returned blocks have sizes
>>> which are powers of two using is_power_of_2(). However, is_power_of_2()
>>> operations on a 'long', but the block size is a u64. So on systems where
>>> long is 32-bit, this can sometimes fail even on correctly sized blocks.
>>>
>>> This only reproduces randomly, as the parameters passed to the buddy
>>> allocator in this test are random. The seed 0xb2e06022 reproduced it
>>> fine here.
>>>
>>> For now, just hardcode an is_power_of_2() implementation using
>>> x & (x - 1).
>>>
>>> Signed-off-by: David Gow <[email protected]>
>> As we still didn't consolidate an implementation of is_power_of_2_u64(),
> I just cooked up some patches to try to make is_power_of_2() more
> flexible. I only sent them to the "CI trybot" for a quick spin first,
> will post to lkml later. [1]
In the meantime I'm pushing this to drm-misc-fixes unless somebody has
some last second objections.
Christian.
>
> BR,
> Jani.
>
>
> [1] https://patchwork.freedesktop.org/series/115785/
>
>> Reviewed-by: Maíra Canal <[email protected]>
>>
>> Best Regards,
>> - Maíra Canal
>>
>>> ---
>>>
>>> There are actually a couple of is_power_of_2_u64() implementations
>>> already around in:
>>> - drivers/gpu/drm/i915/i915_utils.h
>>> - fs/btrfs/misc.h (called is_power_of_two_u64)
>>>
>>> So the ideal thing would be to consolidate these in one place.
>>>
>>>
>>> ---
>>> drivers/gpu/drm/tests/drm_buddy_test.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c b/drivers/gpu/drm/tests/drm_buddy_test.c
>>> index f8ee714df396..09ee6f6af896 100644
>>> --- a/drivers/gpu/drm/tests/drm_buddy_test.c
>>> +++ b/drivers/gpu/drm/tests/drm_buddy_test.c
>>> @@ -89,7 +89,8 @@ static int check_block(struct kunit *test, struct drm_buddy *mm,
>>> err = -EINVAL;
>>> }
>>>
>>> - if (!is_power_of_2(block_size)) {
>>> + /* We can't use is_power_of_2() for a u64 on 32-bit systems. */
>>> + if (block_size & (block_size - 1)) {
>>> kunit_err(test, "block size not power of two\n");
>>> err = -EINVAL;
>>> }
Am 30.03.23 um 12:53 schrieb Jani Nikula:
> On Wed, 29 Mar 2023, David Gow <[email protected]> wrote:
>> The drm buddy allocator tests were broken on 32-bit systems, as
>> rounddown_pow_of_two() takes a long, and the buddy allocator handles
>> 64-bit sizes even on 32-bit systems.
>>
>> This can be reproduced with the drm_buddy_allocator KUnit tests on i386:
>> ./tools/testing/kunit/kunit.py run --arch i386 \
>> --kunitconfig ./drivers/gpu/drm/tests drm_buddy
>>
>> (It results in kernel BUG_ON() when too many blocks are created, due to
>> the block size being too small.)
>>
>> This was independently uncovered (and fixed) by Luís Mendes, whose patch
>> added a new u64 variant of rounddown_pow_of_two(). This version instead
>> recalculates the size based on the order.
>>
>> Reported-by: Luís Mendes <[email protected]>
>> Link: https://lore.kernel.org/lkml/CAEzXK1oghXAB_KpKpm=-CviDQbNaH0qfgYTSSjZgvvyj4U78AA@mail.gmail.com/T/
>> Signed-off-by: David Gow <[email protected]>
>> ---
>> drivers/gpu/drm/drm_buddy.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>> index 3d1f50f481cf..7098f125b54a 100644
>> --- a/drivers/gpu/drm/drm_buddy.c
>> +++ b/drivers/gpu/drm/drm_buddy.c
>> @@ -146,8 +146,8 @@ int drm_buddy_init(struct drm_buddy *mm, u64 size, u64 chunk_size)
>> unsigned int order;
>> u64 root_size;
>>
>> - root_size = rounddown_pow_of_two(size);
>> - order = ilog2(root_size) - ilog2(chunk_size);
>> + order = ilog2(size) - ilog2(chunk_size);
>> + root_size = chunk_size << order;
> Just noticed near the beginning of the function there's also:
>
> if (!is_power_of_2(chunk_size))
> return -EINVAL;
>
> which is also wrong for 32-bit.
Yeah, but that isn't vital. We just use u64 for the chunk_size for
consistency.
In reality I wouldn't except more than 256K here.
Regards,
Christian.
>
>
> BR,
> Jani.
>
>
>>
>> root = drm_block_alloc(mm, NULL, order, offset);
>> if (!root)
On Wed, 29 Mar 2023, David Gow <[email protected]> wrote:
> The drm buddy allocator tests were broken on 32-bit systems, as
> rounddown_pow_of_two() takes a long, and the buddy allocator handles
> 64-bit sizes even on 32-bit systems.
>
> This can be reproduced with the drm_buddy_allocator KUnit tests on i386:
> ./tools/testing/kunit/kunit.py run --arch i386 \
> --kunitconfig ./drivers/gpu/drm/tests drm_buddy
>
> (It results in kernel BUG_ON() when too many blocks are created, due to
> the block size being too small.)
>
> This was independently uncovered (and fixed) by Luís Mendes, whose patch
> added a new u64 variant of rounddown_pow_of_two(). This version instead
> recalculates the size based on the order.
>
> Reported-by: Luís Mendes <[email protected]>
> Link: https://lore.kernel.org/lkml/CAEzXK1oghXAB_KpKpm=-CviDQbNaH0qfgYTSSjZgvvyj4U78AA@mail.gmail.com/T/
> Signed-off-by: David Gow <[email protected]>
> ---
> drivers/gpu/drm/drm_buddy.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
> index 3d1f50f481cf..7098f125b54a 100644
> --- a/drivers/gpu/drm/drm_buddy.c
> +++ b/drivers/gpu/drm/drm_buddy.c
> @@ -146,8 +146,8 @@ int drm_buddy_init(struct drm_buddy *mm, u64 size, u64 chunk_size)
> unsigned int order;
> u64 root_size;
>
> - root_size = rounddown_pow_of_two(size);
> - order = ilog2(root_size) - ilog2(chunk_size);
> + order = ilog2(size) - ilog2(chunk_size);
> + root_size = chunk_size << order;
Just noticed near the beginning of the function there's also:
if (!is_power_of_2(chunk_size))
return -EINVAL;
which is also wrong for 32-bit.
BR,
Jani.
>
> root = drm_block_alloc(mm, NULL, order, offset);
> if (!root)
--
Jani Nikula, Intel Open Source Graphics Center
On Thu, 30 Mar 2023, Christian König <[email protected]> wrote:
> Am 30.03.23 um 12:53 schrieb Jani Nikula:
>> On Wed, 29 Mar 2023, David Gow <[email protected]> wrote:
>>> The drm buddy allocator tests were broken on 32-bit systems, as
>>> rounddown_pow_of_two() takes a long, and the buddy allocator handles
>>> 64-bit sizes even on 32-bit systems.
>>>
>>> This can be reproduced with the drm_buddy_allocator KUnit tests on i386:
>>> ./tools/testing/kunit/kunit.py run --arch i386 \
>>> --kunitconfig ./drivers/gpu/drm/tests drm_buddy
>>>
>>> (It results in kernel BUG_ON() when too many blocks are created, due to
>>> the block size being too small.)
>>>
>>> This was independently uncovered (and fixed) by Luís Mendes, whose patch
>>> added a new u64 variant of rounddown_pow_of_two(). This version instead
>>> recalculates the size based on the order.
>>>
>>> Reported-by: Luís Mendes <[email protected]>
>>> Link: https://lore.kernel.org/lkml/CAEzXK1oghXAB_KpKpm=-CviDQbNaH0qfgYTSSjZgvvyj4U78AA@mail.gmail.com/T/
>>> Signed-off-by: David Gow <[email protected]>
>>> ---
>>> drivers/gpu/drm/drm_buddy.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>>> index 3d1f50f481cf..7098f125b54a 100644
>>> --- a/drivers/gpu/drm/drm_buddy.c
>>> +++ b/drivers/gpu/drm/drm_buddy.c
>>> @@ -146,8 +146,8 @@ int drm_buddy_init(struct drm_buddy *mm, u64 size, u64 chunk_size)
>>> unsigned int order;
>>> u64 root_size;
>>>
>>> - root_size = rounddown_pow_of_two(size);
>>> - order = ilog2(root_size) - ilog2(chunk_size);
>>> + order = ilog2(size) - ilog2(chunk_size);
>>> + root_size = chunk_size << order;
>> Just noticed near the beginning of the function there's also:
>>
>> if (!is_power_of_2(chunk_size))
>> return -EINVAL;
>>
>> which is also wrong for 32-bit.
>
> Yeah, but that isn't vital. We just use u64 for the chunk_size for
> consistency.
>
> In reality I wouldn't except more than 256K here.
Right. It's just not pedantically correct either. ;)
is_power_of_2() is pretty scary as it is, since it just truncates.
BR,
Jani.
>
> Regards,
> Christian.
>
>>
>>
>> BR,
>> Jani.
>>
>>
>>>
>>> root = drm_block_alloc(mm, NULL, order, offset);
>>> if (!root)
>
--
Jani Nikula, Intel Open Source Graphics Center