Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Incorrect boundary and alignment checks in `svm_range_split_head` cause missing GPU page table remaps for shattered huge pages, and unnecessary remaps for perfectly aligned ranges. --
commit 91761d4de2f6b8a545a87df1ae810dd4280f1f59 Author: Gerhard Schwanzer <[email protected]> drm/amdkfd: Use last + 1 of vm range to check 2MB huge page alignment This patch updates the split-remap logic to use the exclusive upper bound when computing the last 2MB-aligned boundary of the original range. It also modifies the split head check to compare the inclusive head->last directly against the exclusive aligned boundary. Link: https://lore.kernel.org/stable/ia1pr12mb85172f7fe9157c092eda46a0e3...@ia1pr12mb8517.namprd12.prod.outlook.com/ > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c > b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c > index 72cfb4a6ab3e1..30693ed767942 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c [ ... ] > @@ -1181,8 +1181,8 @@ svm_range_split_head(struct svm_range *prange, uint64_t > new_start, > > list_add(&head->list, insert_list); > > - if (huge_page_mapping && head->last + 1 > start_align && > - head->last + 1 < last_align_down && (!IS_ALIGNED(head->last, 512))) > + if (huge_page_mapping && head->last > start_align && > + head->last < last_align_down && (!IS_ALIGNED(head->last, 512))) > list_add(&head->update_list, remap_list); [Severity: High] Does removing the + 1 offsets from head->last in these boundary checks introduce unnecessary remaps for aligned ranges, while failing to fix the shattered huge pages? Because head->last is an inclusive page index, its aligned boundary values are 511, 1023, etc. If a split head perfectly aligns with a 2MB page (for example, ending at 511), head->last is 511. The !IS_ALIGNED(511, 512) check is true. Before this patch, head->last + 1 < last_align_down (512 < 512) evaluated to false, safely preventing a remap. By removing the + 1, this evaluates 511 < 512 as true, causing perfectly aligned ranges to be unnecessarily remapped. Additionally, if a split head shatters a 2MB page by ending exactly one page into the next block (for example, ending at 512), !IS_ALIGNED(512, 512) is false. This incorrectly skips adding the shattered range to the remap list, leaving stale 2MB mappings in the hardware. Should the code use the exclusive bound head->last + 1 for all of these comparisons, including the IS_ALIGNED check? > return 0; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
