diff mbox series

[2/2] drm: test: Fix 32-bit issue in drm_buddy_test

Message ID 20230329065532.2122295-2-davidgow@google.com
State New
Headers
Series [1/2] drm: buddy_allocator: Fix buddy allocator init on 32-bit systems |

Commit Message

David Gow March 29, 2023, 6:55 a.m. UTC 
  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 <davidgow@google.com>
---

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(-)

Comments

Jani Nikula March 29, 2023, 9:39 a.m. UTC | #1 
On Wed, 29 Mar 2023, David Gow <davidgow@google.com> 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 <davidgow@google.com>
> ---
>
> 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 March 29, 2023, 9:42 a.m. UTC | #2 
On Wed, 29 Mar 2023, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 29 Mar 2023, David Gow <davidgow@google.com> 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 <davidgow@google.com>
>> ---
>>
>> 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;
>>  	}
Maíra Canal March 29, 2023, 10:54 a.m. UTC | #3 
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 <davidgow@google.com>

As we still didn't consolidate an implementation of is_power_of_2_u64(),

Reviewed-by: Maíra Canal <mcanal@igalia.com>

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 March 29, 2023, 11:28 a.m. UTC | #4 
On Wed, 29 Mar 2023, Maíra Canal <mcanal@igalia.com> 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 <davidgow@google.com>
>
> 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 <mcanal@igalia.com>
>
> 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;
>>   	}
Christian König March 29, 2023, 1:14 p.m. UTC | #5 
Am 29.03.23 um 13:28 schrieb Jani Nikula:
> On Wed, 29 Mar 2023, Maíra Canal <mcanal@igalia.com> 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 <davidgow@google.com>
>> 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 <mcanal@igalia.com>
>>
>> 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;
>>>    	}
diff mbox series

Patch

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;
 	}