From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: gpu/tests/gpu_buddy: Add gpu_test_buddy_alloc_range for exact-range allocation Date: Tue, 03 Mar 2026 13:14:20 +1000 Message-ID: In-Reply-To: <20260302150947.47535-2-sanjay.kumar.yadav@intel.com> References: <20260302150947.47535-2-sanjay.kumar.yadav@intel.com> <20260302150947.47535-2-sanjay.kumar.yadav@intel.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Positive aspects:** - Comprehensive coverage: basic full-mm, sub-range quarters, min-chunk at m= ultiple offsets, non-power-of-two mm sizes (multi-root), randomized slices,= negative overlap cases, checkerboard patterns, misalignment rejection, fre= e/re-alloc cycling, and mixed power-of-two sizes. - Negative tests correctly trigger the exact-range path (they satisfy `star= t + size =3D=3D end`) and verify that the appropriate validation catches th= em. - The randomized range generation maintains the invariant that `max_chunk >= =3D ps > 0` (since both `remaining` and `(N_RAND_RANGES - 1 - i) * ps` are = always multiples of `ps`), so there is no division-by-zero or underflow ris= k. I verified this carefully. **Observations and minor issues:** 1. **Leaked blocks on unexpected negative-test success.** In several negati= ve tests, the allocation result goes into `tmp`, but if the allocation unex= pectedly *succeeds* (indicating an allocator bug), those blocks are never f= reed before `gpu_buddy_fini()`: ```c ret =3D gpu_buddy_alloc_blocks(&mm, 0, mm_size, mm_size, ps, &tmp, 0); KUNIT_EXPECT_NE_MSG(test, ret, 0, "exact-range alloc should fail when range is partially used\n"); ``` This is consistent with how `gpu_test_buddy_alloc_range_bias` handles its n= egative cases, so it's an existing pattern. But it means a failing EXPECT w= on't trigger the WARN in `gpu_buddy_fini()` about unreleased blocks =E2=80= =94 consider adding `gpu_buddy_free_list(&mm, &tmp, 0)` after each negative= assertion, or at least before `gpu_buddy_fini()`, to make failures easier = to debug. 2. **Function length.** At 327 lines, the function is quite long. The sub-t= est blocks are already scoped with `{ }` for variable locality, which is go= od, but consider whether splitting into separate `kunit_case` functions (e.= g., `gpu_test_buddy_alloc_range_negative`, `gpu_test_buddy_alloc_range_rand= om`) would improve readability and give finer-grained pass/fail reporting f= rom KUnit. This is a stylistic suggestion, not a blocker. 3. **Test ordering in the case array.** The new test is inserted *before* `= gpu_test_buddy_alloc_range_bias`: ```c KUNIT_CASE(gpu_test_buddy_alloc_clear), KUNIT_CASE(gpu_test_buddy_alloc_range), KUNIT_CASE(gpu_test_buddy_alloc_range_bias), ``` This is a reasonable grouping. However, the name `gpu_test_buddy_alloc_rang= e` is very similar to `gpu_test_buddy_alloc_range_bias` and could cause con= fusion since the former tests the "exact-range" path (flags=3D0, start+size= =3D=3Dend) while the latter tests range-biased allocation (GPU_BUDDY_RANGE_= ALLOCATION flag). A more descriptive name like `gpu_test_buddy_alloc_range_= exact` would clarify intent and better distinguish the two. 4. **Negative misalignment tests.** The three misalignment sub-tests are go= od but it's worth noting they are all caught by the *general* alignment che= ck at `buddy.c:1117`: ```c if (!IS_ALIGNED(start | end | size, mm->chunk_size)) return -EINVAL; ``` rather than the exact-range-specific check at line 1128. The test still val= idates correct rejection, but a comment clarifying this would help future m= aintainers understand which validation layer is being exercised. If the int= ent is to specifically test the exact-range alignment check on line 1128, y= ou'd need start/end/size all aligned to `chunk_size` but start not aligned = to `min_block_size` (when `min_block_size > chunk_size`). 5. **Minor style:** The commit subject line prefix `gpu/tests/gpu_buddy:` d= iffers slightly from what you might see as a conventional `drm/buddy:` or `= drm/tests:` prefix. Check the existing convention for this test file in the= git log. **Correctness verification of key test logic:** - Quarter allocation: All four allocations go into the same `blocks` list; = `gpu_buddy_free_list` correctly frees them all. =E2=9C=93 - Checkerboard: Allocates every other page into `blocks`, tries exact-range= over a partially-occupied pair, correctly verifies failure, then allocates= the free page. The final `gpu_buddy_free_list(&mm, &blocks, 0)` frees both= the checkerboard and the single free-page allocation. =E2=9C=93 - Randomized: `round_up(prandom_u32_state(&prng) % max_chunk, ps)` =E2=80= =94 since `max_chunk` is always a multiple of `ps`, the rounded-up result n= ever exceeds `max_chunk`, preserving the budget invariant. =E2=9C=93 - Out-of-bounds negative test: caught by `end > mm->size` check at `buddy.c= :1120`. =E2=9C=93 **Verdict:** The patch is correct and provides valuable test coverage for t= he exact-range allocation path. The suggestions above (naming, tmp cleanup,= possible split) are minor improvements. No blocking issues. --- Generated by Claude Code Patch Reviewer