Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Returning false for non-blockable invalidations incorrectly prevents the OOM reaper from unmapping memory. - [Medium] Redundant VMA splitting for partial overlaps when the VMA already has default attributes. --
commit fe0b22bc527cb1c9caa55068f36be5982435fd31 Author: Arvind Yadav <[email protected]> drm/xe/vm: Add madvise autoreset notifier worker This commit adds a worker to reset madvise-managed VMA attributes when userspace unmaps CPU-only mirror ranges. It uses an MMU notifier callback to record pending unmap events and widens the range if multiple unmaps arrive before the worker runs. The worker then resets attributes for ranges not covered by a CPU VMA. > diff --git a/drivers/gpu/drm/xe/xe_vm_madvise.c > b/drivers/gpu/drm/xe/xe_vm_madvise.c > index c4fb290041956..c2abe712598a1 100644 > --- a/drivers/gpu/drm/xe/xe_vm_madvise.c > +++ b/drivers/gpu/drm/xe/xe_vm_madvise.c [ ... ] > @@ -732,3 +738,501 @@ int xe_vm_madvise_ioctl(struct drm_device *dev, void > *data, struct drm_file *fil [ ... ] > +static int xe_vm_madvise_process_unmap(struct xe_vm *vm, u64 start, u64 end) > +{ [ ... ] > + has_default_attr = xe_vma_has_default_mem_attrs(vma); > + seg_start = max(addr, xe_vma_start(vma)); > + seg_end = min(end, xe_vma_end(vma)); > + > + /* Merge adjacent default-attr VMAs when possible. */ > + if (has_default_attr && > + xe_vma_start(vma) >= start && > + xe_vma_end(vma) <= end) { > + seg_start = xe_vma_start(vma); > + seg_end = xe_vma_end(vma); > + xe_vm_find_cpu_addr_mirror_vma_range(vm, &seg_start, > &seg_end); > + if (xe_vma_start(vma) == seg_start && xe_vma_end(vma) > == seg_end) { > + /* Nothing to merge. */ > + addr = seg_end; > + continue; > + } > + } else if (xe_vma_start(vma) == seg_start && xe_vma_end(vma) == > seg_end) { > + /* Exact VMA match, reset in place. */ > + xe_vma_reset_to_default_attrs(vma); > + addr = seg_end; > + continue; > + } > + > + err = xe_vm_alloc_cpu_addr_mirror_vma(vm, seg_start, seg_end - > seg_start); [Severity: Medium] When a CPU hole partially overlaps a GPU VMA that already has default attributes, will this fall through to xe_vm_alloc_cpu_addr_mirror_vma() and cause redundant VMA splitting? If the overlap is strictly partial, both the if and else if conditions above fail. The fall-through maps the overlapping portion with default attributes, forcing the GPUVM infrastructure to split the existing VMA. Since the existing VMA already possessed default attributes, this split appears to be redundant and might needlessly fragment a single large VMA into smaller identical pieces. Repeated partial unmaps from userspace could artificially fragment the VMA tree and leak xe_vma objects. [ ... ] > +static bool xe_madvise_notifier_callback(struct mmu_interval_notifier *mni, > + const struct mmu_notifier_range *range, > + unsigned long cur_seq) > +{ > + struct xe_madvise_notifier *notifier = > + container_of(mni, struct xe_madvise_notifier, mmu_notifier); > + struct xe_vm *vm = notifier->vm; > + u64 adj_start, adj_end; > + > + if (range->event != MMU_NOTIFY_UNMAP) > + return true; > + > + if (!mmu_notifier_range_blockable(range)) > + return false; [Severity: High] Does returning false for non-blockable invalidations incorrectly prevent the OOM reaper from unmapping memory? Because the callback implementation is entirely non-blocking (it relies on down_read_trylock(), spin_lock(), and queue_work()), there seems to be no technical limitation requiring it to block. Returning false from an MMU interval notifier's invalidate callback signals that the invalidation could not be completed without blocking, causing __mmu_notifier_invalidate_range_start() to return -EAGAIN. Since the OOM reaper operates in a non-blockable context, receiving -EAGAIN means it cannot retry indefinitely and will skip the VMA, failing to reclaim the memory and defeating the purpose of the OOM killer. Could this safely process the event and unconditionally return true for unmap events instead? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
