public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] gpu/buddy: Introduce gpu_buddy_assert() for kunit-aware assertions
@ 2026-02-24 13:25 Sanjay Yadav
  2026-02-25 11:19 ` Arunpravin Paneer Selvam
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Sanjay Yadav @ 2026-02-24 13:25 UTC (permalink / raw)
  To: dri-devel
  Cc: intel-xe, Christian König, Arunpravin Paneer Selvam,
	Matthew Auld

Introduce gpu_buddy_assert(), a small helper that wraps WARN_ON() and,
when CONFIG_KUNIT is enabled, also calls kunit_fail_current_test() so
that any active KUnit test is marked as failed. In non-KUnit builds
the macro reduces to WARN_ON(), preserving existing behaviour.

Stringify the asserted condition in the failure message to make it easy
to identify which assertion fired. Leave the WARN_ON() in
gpu_buddy_block_trim() unchanged, as it returns -EINVAL and the caller
already observes the failure via the return code.

Cc: Christian König <christian.koenig@amd.com>
Cc: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
Suggested-by: Matthew Auld <matthew.auld@intel.com>
Signed-off-by: Sanjay Yadav <sanjay.kumar.yadav@intel.com>
---
 drivers/gpu/buddy.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/buddy.c b/drivers/gpu/buddy.c
index b27761246d4b..da5a1222f46b 100644
--- a/drivers/gpu/buddy.c
+++ b/drivers/gpu/buddy.c
@@ -3,8 +3,7 @@
  * Copyright © 2021 Intel Corporation
  */
 
-#include <kunit/test-bug.h>
-
+#include <linux/bug.h>
 #include <linux/export.h>
 #include <linux/kmemleak.h>
 #include <linux/module.h>
@@ -12,6 +11,28 @@
 
 #include <linux/gpu_buddy.h>
 
+/**
+ * gpu_buddy_assert - assert a condition in the buddy allocator
+ * @condition: condition expected to be true
+ *
+ * When CONFIG_KUNIT is enabled, evaluates @condition and, if false, triggers
+ * a WARN_ON() and also calls kunit_fail_current_test() so that any running
+ * kunit test is properly marked as failed. The stringified condition is
+ * included in the failure message for easy identification.
+ *
+ * When CONFIG_KUNIT is not enabled, this reduces to WARN_ON() so production
+ * builds retain the same warning semantics as before.
+ */
+#if IS_ENABLED(CONFIG_KUNIT)
+#include <kunit/test-bug.h>
+#define gpu_buddy_assert(condition) do {						\
+	if (WARN_ON(!(condition)))						\
+		kunit_fail_current_test("gpu_buddy_assert(" #condition ")");	\
+} while (0)
+#else
+#define gpu_buddy_assert(condition) WARN_ON(!(condition))
+#endif
+
 static struct kmem_cache *slab_blocks;
 
 static unsigned int
@@ -268,8 +289,8 @@ static int __force_merge(struct gpu_buddy *mm,
 				if (!gpu_buddy_block_is_free(buddy))
 					continue;
 
-				WARN_ON(gpu_buddy_block_is_clear(block) ==
-					gpu_buddy_block_is_clear(buddy));
+				gpu_buddy_assert(gpu_buddy_block_is_clear(block) !=
+						 gpu_buddy_block_is_clear(buddy));
 
 				/*
 				 * Advance to the next node when the current node is the buddy,
@@ -415,8 +436,7 @@ void gpu_buddy_fini(struct gpu_buddy *mm)
 		start = gpu_buddy_block_offset(mm->roots[i]);
 		__force_merge(mm, start, start + size, order);
 
-		if (WARN_ON(!gpu_buddy_block_is_free(mm->roots[i])))
-			kunit_fail_current_test("buddy_fini() root");
+		gpu_buddy_assert(gpu_buddy_block_is_free(mm->roots[i]));
 
 		gpu_block_free(mm, mm->roots[i]);
 
@@ -424,7 +444,7 @@ void gpu_buddy_fini(struct gpu_buddy *mm)
 		size -= root_size;
 	}
 
-	WARN_ON(mm->avail != mm->size);
+	gpu_buddy_assert(mm->avail == mm->size);
 
 	for_each_free_tree(i)
 		kfree(mm->free_trees[i]);
@@ -541,7 +561,7 @@ static void __gpu_buddy_free_list(struct gpu_buddy *mm,
 {
 	struct gpu_buddy_block *block, *on;
 
-	WARN_ON(mark_dirty && mark_clear);
+	gpu_buddy_assert(!(mark_dirty && mark_clear));
 
 	list_for_each_entry_safe(block, on, objects, link) {
 		if (mark_clear)
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] gpu/buddy: Introduce gpu_buddy_assert() for kunit-aware assertions
  2026-02-24 13:25 [PATCH] gpu/buddy: Introduce gpu_buddy_assert() for kunit-aware assertions Sanjay Yadav
@ 2026-02-25 11:19 ` Arunpravin Paneer Selvam
  2026-02-25 13:20   ` Yadav, Sanjay Kumar
  2026-02-27  5:09 ` Claude review: " Claude Code Review Bot
  2026-02-27  5:09 ` Claude Code Review Bot
  2 siblings, 1 reply; 5+ messages in thread
From: Arunpravin Paneer Selvam @ 2026-02-25 11:19 UTC (permalink / raw)
  To: Sanjay Yadav, dri-devel; +Cc: intel-xe, Christian König, Matthew Auld

Reviewed-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>

On 2/24/2026 6:55 PM, Sanjay Yadav wrote:
> Introduce gpu_buddy_assert(), a small helper that wraps WARN_ON() and,
> when CONFIG_KUNIT is enabled, also calls kunit_fail_current_test() so
> that any active KUnit test is marked as failed. In non-KUnit builds
> the macro reduces to WARN_ON(), preserving existing behaviour.
>
> Stringify the asserted condition in the failure message to make it easy
> to identify which assertion fired. Leave the WARN_ON() in
> gpu_buddy_block_trim() unchanged, as it returns -EINVAL and the caller
> already observes the failure via the return code.
>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
> Suggested-by: Matthew Auld <matthew.auld@intel.com>
> Signed-off-by: Sanjay Yadav <sanjay.kumar.yadav@intel.com>
> ---
>   drivers/gpu/buddy.c | 36 ++++++++++++++++++++++++++++--------
>   1 file changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/buddy.c b/drivers/gpu/buddy.c
> index b27761246d4b..da5a1222f46b 100644
> --- a/drivers/gpu/buddy.c
> +++ b/drivers/gpu/buddy.c
> @@ -3,8 +3,7 @@
>    * Copyright © 2021 Intel Corporation
>    */
>   
> -#include <kunit/test-bug.h>
> -
> +#include <linux/bug.h>
>   #include <linux/export.h>
>   #include <linux/kmemleak.h>
>   #include <linux/module.h>
> @@ -12,6 +11,28 @@
>   
>   #include <linux/gpu_buddy.h>
>   
> +/**
> + * gpu_buddy_assert - assert a condition in the buddy allocator
> + * @condition: condition expected to be true
> + *
> + * When CONFIG_KUNIT is enabled, evaluates @condition and, if false, triggers
> + * a WARN_ON() and also calls kunit_fail_current_test() so that any running
> + * kunit test is properly marked as failed. The stringified condition is
> + * included in the failure message for easy identification.
> + *
> + * When CONFIG_KUNIT is not enabled, this reduces to WARN_ON() so production
> + * builds retain the same warning semantics as before.
> + */
> +#if IS_ENABLED(CONFIG_KUNIT)
> +#include <kunit/test-bug.h>
> +#define gpu_buddy_assert(condition) do {						\
> +	if (WARN_ON(!(condition)))						\
> +		kunit_fail_current_test("gpu_buddy_assert(" #condition ")");	\
> +} while (0)
> +#else
> +#define gpu_buddy_assert(condition) WARN_ON(!(condition))
> +#endif
> +
>   static struct kmem_cache *slab_blocks;
>   
>   static unsigned int
> @@ -268,8 +289,8 @@ static int __force_merge(struct gpu_buddy *mm,
>   				if (!gpu_buddy_block_is_free(buddy))
>   					continue;
>   
> -				WARN_ON(gpu_buddy_block_is_clear(block) ==
> -					gpu_buddy_block_is_clear(buddy));
> +				gpu_buddy_assert(gpu_buddy_block_is_clear(block) !=
> +						 gpu_buddy_block_is_clear(buddy));
>   
>   				/*
>   				 * Advance to the next node when the current node is the buddy,
> @@ -415,8 +436,7 @@ void gpu_buddy_fini(struct gpu_buddy *mm)
>   		start = gpu_buddy_block_offset(mm->roots[i]);
>   		__force_merge(mm, start, start + size, order);
>   
> -		if (WARN_ON(!gpu_buddy_block_is_free(mm->roots[i])))
> -			kunit_fail_current_test("buddy_fini() root");
> +		gpu_buddy_assert(gpu_buddy_block_is_free(mm->roots[i]));
>   
>   		gpu_block_free(mm, mm->roots[i]);
>   
> @@ -424,7 +444,7 @@ void gpu_buddy_fini(struct gpu_buddy *mm)
>   		size -= root_size;
>   	}
>   
> -	WARN_ON(mm->avail != mm->size);
> +	gpu_buddy_assert(mm->avail == mm->size);
>   
>   	for_each_free_tree(i)
>   		kfree(mm->free_trees[i]);
> @@ -541,7 +561,7 @@ static void __gpu_buddy_free_list(struct gpu_buddy *mm,
>   {
>   	struct gpu_buddy_block *block, *on;
>   
> -	WARN_ON(mark_dirty && mark_clear);
> +	gpu_buddy_assert(!(mark_dirty && mark_clear));
>   
>   	list_for_each_entry_safe(block, on, objects, link) {
>   		if (mark_clear)


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] gpu/buddy: Introduce gpu_buddy_assert() for kunit-aware assertions
  2026-02-25 11:19 ` Arunpravin Paneer Selvam
@ 2026-02-25 13:20   ` Yadav, Sanjay Kumar
  0 siblings, 0 replies; 5+ messages in thread
From: Yadav, Sanjay Kumar @ 2026-02-25 13:20 UTC (permalink / raw)
  To: Arunpravin Paneer Selvam, dri-devel
  Cc: intel-xe, Christian König, Matthew Auld


On 25-02-2026 16:49, Arunpravin Paneer Selvam wrote:
> Reviewed-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>

Thanks for your prompt review, Arun!

-Sanjay

>
> On 2/24/2026 6:55 PM, Sanjay Yadav wrote:
>> Introduce gpu_buddy_assert(), a small helper that wraps WARN_ON() and,
>> when CONFIG_KUNIT is enabled, also calls kunit_fail_current_test() so
>> that any active KUnit test is marked as failed. In non-KUnit builds
>> the macro reduces to WARN_ON(), preserving existing behaviour.
>>
>> Stringify the asserted condition in the failure message to make it easy
>> to identify which assertion fired. Leave the WARN_ON() in
>> gpu_buddy_block_trim() unchanged, as it returns -EINVAL and the caller
>> already observes the failure via the return code.
>>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
>> Suggested-by: Matthew Auld <matthew.auld@intel.com>
>> Signed-off-by: Sanjay Yadav <sanjay.kumar.yadav@intel.com>
>> ---
>>   drivers/gpu/buddy.c | 36 ++++++++++++++++++++++++++++--------
>>   1 file changed, 28 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/buddy.c b/drivers/gpu/buddy.c
>> index b27761246d4b..da5a1222f46b 100644
>> --- a/drivers/gpu/buddy.c
>> +++ b/drivers/gpu/buddy.c
>> @@ -3,8 +3,7 @@
>>    * Copyright © 2021 Intel Corporation
>>    */
>>   -#include <kunit/test-bug.h>
>> -
>> +#include <linux/bug.h>
>>   #include <linux/export.h>
>>   #include <linux/kmemleak.h>
>>   #include <linux/module.h>
>> @@ -12,6 +11,28 @@
>>     #include <linux/gpu_buddy.h>
>>   +/**
>> + * gpu_buddy_assert - assert a condition in the buddy allocator
>> + * @condition: condition expected to be true
>> + *
>> + * When CONFIG_KUNIT is enabled, evaluates @condition and, if false, 
>> triggers
>> + * a WARN_ON() and also calls kunit_fail_current_test() so that any 
>> running
>> + * kunit test is properly marked as failed. The stringified 
>> condition is
>> + * included in the failure message for easy identification.
>> + *
>> + * When CONFIG_KUNIT is not enabled, this reduces to WARN_ON() so 
>> production
>> + * builds retain the same warning semantics as before.
>> + */
>> +#if IS_ENABLED(CONFIG_KUNIT)
>> +#include <kunit/test-bug.h>
>> +#define gpu_buddy_assert(condition) do { \
>> +    if (WARN_ON(!(condition)))                        \
>> +        kunit_fail_current_test("gpu_buddy_assert(" #condition 
>> ")");    \
>> +} while (0)
>> +#else
>> +#define gpu_buddy_assert(condition) WARN_ON(!(condition))
>> +#endif
>> +
>>   static struct kmem_cache *slab_blocks;
>>     static unsigned int
>> @@ -268,8 +289,8 @@ static int __force_merge(struct gpu_buddy *mm,
>>                   if (!gpu_buddy_block_is_free(buddy))
>>                       continue;
>>   -                WARN_ON(gpu_buddy_block_is_clear(block) ==
>> -                    gpu_buddy_block_is_clear(buddy));
>> + gpu_buddy_assert(gpu_buddy_block_is_clear(block) !=
>> +                         gpu_buddy_block_is_clear(buddy));
>>                     /*
>>                    * Advance to the next node when the current node 
>> is the buddy,
>> @@ -415,8 +436,7 @@ void gpu_buddy_fini(struct gpu_buddy *mm)
>>           start = gpu_buddy_block_offset(mm->roots[i]);
>>           __force_merge(mm, start, start + size, order);
>>   -        if (WARN_ON(!gpu_buddy_block_is_free(mm->roots[i])))
>> -            kunit_fail_current_test("buddy_fini() root");
>> + gpu_buddy_assert(gpu_buddy_block_is_free(mm->roots[i]));
>>             gpu_block_free(mm, mm->roots[i]);
>>   @@ -424,7 +444,7 @@ void gpu_buddy_fini(struct gpu_buddy *mm)
>>           size -= root_size;
>>       }
>>   -    WARN_ON(mm->avail != mm->size);
>> +    gpu_buddy_assert(mm->avail == mm->size);
>>         for_each_free_tree(i)
>>           kfree(mm->free_trees[i]);
>> @@ -541,7 +561,7 @@ static void __gpu_buddy_free_list(struct 
>> gpu_buddy *mm,
>>   {
>>       struct gpu_buddy_block *block, *on;
>>   -    WARN_ON(mark_dirty && mark_clear);
>> +    gpu_buddy_assert(!(mark_dirty && mark_clear));
>>         list_for_each_entry_safe(block, on, objects, link) {
>>           if (mark_clear)
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Claude review: gpu/buddy: Introduce gpu_buddy_assert() for kunit-aware assertions
  2026-02-24 13:25 [PATCH] gpu/buddy: Introduce gpu_buddy_assert() for kunit-aware assertions Sanjay Yadav
  2026-02-25 11:19 ` Arunpravin Paneer Selvam
@ 2026-02-27  5:09 ` Claude Code Review Bot
  2026-02-27  5:09 ` Claude Code Review Bot
  2 siblings, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-02-27  5:09 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: gpu/buddy: Introduce gpu_buddy_assert() for kunit-aware assertions
Author: Sanjay Yadav <sanjay.kumar.yadav@intel.com>
Patches: 3
Reviewed: 2026-02-27T15:09:14.390252

---

This is a single-patch series introducing a `gpu_buddy_assert()` macro to the gpu buddy allocator (a renamed/refactored version of `drm_buddy`). The macro unifies the existing ad-hoc pattern of `WARN_ON()` + optional `kunit_fail_current_test()` into a consistent helper. The patch is clean, well-motivated, and the commit message clearly explains the rationale including why `gpu_buddy_block_trim()`'s WARN_ON is left untouched.

The condition inversions at each call site are all correct, the `#include` restructuring is appropriate, and the macro avoids double-evaluation. No functional issues found.

A few minor observations below, but nothing blocking.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Claude review: gpu/buddy: Introduce gpu_buddy_assert() for kunit-aware assertions
  2026-02-24 13:25 [PATCH] gpu/buddy: Introduce gpu_buddy_assert() for kunit-aware assertions Sanjay Yadav
  2026-02-25 11:19 ` Arunpravin Paneer Selvam
  2026-02-27  5:09 ` Claude review: " Claude Code Review Bot
@ 2026-02-27  5:09 ` Claude Code Review Bot
  2 siblings, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-02-27  5:09 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Positive aspects:**
- The macro is well-documented with a kernel-doc comment.
- Correctly avoids double-evaluation of the condition — it's evaluated once inside `WARN_ON(!(condition))`.
- The decision to leave the `WARN_ON` in `gpu_buddy_block_trim()` alone is sensible and explicitly called out in the commit message.
- The `#include <kunit/test-bug.h>` is now properly guarded behind `IS_ENABLED(CONFIG_KUNIT)`, which avoids pulling in kunit headers in production builds.

**Minor observations:**

1. **Return value of WARN_ON discarded in non-KUNIT path**: In the `#else` branch:
```c
#define gpu_buddy_assert(condition) WARN_ON(!(condition))
```
`WARN_ON()` returns a value (the condition), but all call sites discard it (the macro is used as a statement, not in an `if`). This is fine for the current usage, but if someone later tries to use `if (gpu_buddy_assert(...))`, the KUNIT path (wrapped in `do { } while(0)`) would not compile while the non-KUNIT path would. This asymmetry is minor since the macro name `_assert` doesn't suggest a return value, but for consistency you could wrap the `#else` case in `do { } while(0)` as well:
```c
#define gpu_buddy_assert(condition) do { WARN_ON(!(condition)); } while (0)
```

2. **`#include <linux/bug.h>` addition**: The old code included `<kunit/test-bug.h>` unconditionally, which transitively provided `WARN_ON` via its include chain. Now that `kunit/test-bug.h` is conditional, adding `<linux/bug.h>` explicitly is correct. However, it's worth noting that `WARN_ON` is likely already available through other transitive includes (e.g., `<linux/slab.h>` via `<linux/module.h>`), so the explicit include is defensive — which is good practice.

3. **Stringification readability for the negated case**: At the `__gpu_buddy_free_list` site:
```c
gpu_buddy_assert(!(mark_dirty && mark_clear));
```
The kunit failure message will read `"gpu_buddy_assert(!(mark_dirty && mark_clear))"`, which is clear enough, though slightly verbose. This is a trivial aesthetic point.

4. **Scope of the macro**: The macro is defined locally in `buddy.c` rather than in the `<linux/gpu_buddy.h>` header. This is appropriate since all current users are in this single file, following the principle of narrowest scope.

**Correctness of each hunk:**

- **`__force_merge`**: `WARN_ON(clear(block) == clear(buddy))` → `gpu_buddy_assert(clear(block) != clear(buddy))` — logically equivalent, correct.
- **`gpu_buddy_fini` root check**: `if (WARN_ON(!is_free(...))) kunit_fail_current_test(...)` → `gpu_buddy_assert(is_free(...))` — equivalent, cleaner.
- **`gpu_buddy_fini` avail check**: `WARN_ON(mm->avail != mm->size)` → `gpu_buddy_assert(mm->avail == mm->size)` — correct inversion, adds kunit reporting that was previously missing.
- **`__gpu_buddy_free_list`**: `WARN_ON(mark_dirty && mark_clear)` → `gpu_buddy_assert(!(mark_dirty && mark_clear))` — correct inversion, adds kunit reporting that was previously missing.

**Verdict:** The patch is straightforward, correct, and improves consistency. The only actionable suggestion is wrapping the `#else` path in `do { } while(0)` for macro hygiene symmetry. Otherwise:

Reviewed-by worthy with the minor `do { } while(0)` nit optionally addressed.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-02-27  5:09 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-02-24 13:25 [PATCH] gpu/buddy: Introduce gpu_buddy_assert() for kunit-aware assertions Sanjay Yadav
2026-02-25 11:19 ` Arunpravin Paneer Selvam
2026-02-25 13:20   ` Yadav, Sanjay Kumar
2026-02-27  5:09 ` Claude review: " Claude Code Review Bot
2026-02-27  5:09 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox