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/buddy: Track per-order used blocks with a scoreboard Date: Tue, 05 May 2026 08:19:37 +1000 Message-ID: In-Reply-To: <20260504135343.1797869-3-francois.dugast@intel.com> References: <20260504135343.1797869-1-francois.dugast@intel.com> <20260504135343.1797869-3-francois.dugast@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 **Correctness concern in `__gpu_buddy_free()` coalescing loop:** ```c if (gpu_buddy_block_is_allocated(block)) mm->used_scoreboard[gpu_buddy_block_order(block)]--; gpu_block_free(mm, block); gpu_block_free(mm, buddy); block =3D parent; ``` On the **first iteration** of the loop, `block` can indeed be allocated (th= e normal path from `gpu_buddy_free_block()` which asserts `gpu_buddy_block_= is_allocated(block)` at line 616). So the decrement is correct on the first= iteration. However, on **subsequent iterations** of the coalescing loop, `block =3D pa= rent` from the previous iteration. The parent was previously in `GPU_BUDDY_= SPLIT` state (that's how the buddy allocator tree works =E2=80=94 when chil= dren exist, the parent is split). A split block is not allocated, so `gpu_b= uddy_block_is_allocated(block)` will be false and the decrement won't fire.= This is actually correct behavior =E2=80=94 split blocks shouldn't affect = the used scoreboard. But there's a subtler problem: **when `__gpu_buddy_free()` is called from `= __force_merge()`** at line 356, the `block` passed in is **free** (it was j= ust pulled from the free rbtree and had `rbtree_remove()` called on it, and= its free_scoreboard already decremented). In this path, `block` is neither= allocated nor split =E2=80=94 it's free (though removed from the tree). So= the `gpu_buddy_block_is_allocated()` check will correctly be false, and no= spurious decrement happens. This path is safe. Similarly, the error-path callers at lines 768, 878, 999 pass free blocks, = so the guard is correct there too. **Net assessment for this guard:** The `gpu_buddy_block_is_allocated()` che= ck in `__gpu_buddy_free()` is **correct** =E2=80=94 it only fires on the fi= rst iteration when called from `gpu_buddy_free_block()`. But the commit mes= sage says: > "consumed directly by gpu_block_free() during coalescing" This is somewhat misleading. The block isn't "consumed by gpu_block_free() = during coalescing" in the sense of bypassing mark_free =E2=80=94 `gpu_block= _free()` just frees the slab memory. The used_scoreboard decrement is neede= d because the block is destroyed (merged into parent) without going through= `mark_free()` first. The commit message and the comment in `gpu_buddy.h` (= which says "decremented in gpu_buddy_free_block()") don't accurately descri= be this site =E2=80=94 they should mention `__gpu_buddy_free()`'s coalescin= g loop. **Observation on `mark_free()` used_scoreboard decrement:** ```c static void mark_free(struct gpu_buddy *mm, struct gpu_buddy_block *block) { if (gpu_buddy_block_is_allocated(block)) mm->used_scoreboard[gpu_buddy_block_order(block)]--; ``` `mark_free()` is called from: 1. `__gpu_buddy_free()` at line 298 =E2=80=94 at the end, after the coalesc= ing loop. At this point `block` could be the original allocated block (if n= o coalescing happened) or a parent that was split. Only in the "no coalesci= ng" case is the block still allocated, so the guard is correct. 2. `gpu_buddy_init()` at line 455 =E2=80=94 marking root blocks free during= init. These are freshly allocated slab objects, not in ALLOCATED state. Sa= fe. This is correct. **Naming nit:** `out_free_free_scoreboard` is an unfortunately stuttery lab= el name. Something like `out_free_scoreboards` or `out_free_sb` would read = better. **Comment accuracy in `gpu_buddy.h`:** ```c * Incremented in mark_allocated(), decremented in * gpu_buddy_free_block() which is the sole entry point for freeing * allocated blocks. ``` This comment says the decrement happens in `gpu_buddy_free_block()`, but th= e actual decrement happens in two places: `mark_free()` (which has the `gpu= _buddy_block_is_allocated()` guard) and inside `__gpu_buddy_free()`'s coale= scing loop. `gpu_buddy_free_block()` is the public entry point, but the dec= rements happen deeper. The comment should be updated to reflect the actual = sites, or simplified to just say "decremented when blocks leave the allocat= ed state." **Print format change:** The output format changes from `blocks:` to `free_= blocks:, used_blocks:` in both `gpu_buddy_print()` and `drm_buddy_print()`.= This is a user-visible debugfs format change. If any tooling parses this o= utput, it would break. This is probably fine for debugfs but worth noting. **Summary:** The patch is functionally correct =E2=80=94 the scoreboard acc= ounting is right at all transition points. The main feedback is: 1. The header comment for `used_scoreboard` doesn't accurately describe whe= re decrements happen. 2. The `out_free_free_scoreboard` label name could be improved. 3. The commit message's description of the decrement sites is slightly misl= eading. --- Generated by Claude Code Patch Reviewer