Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Medium] Concurrent exec queue additions and removals create a Time-of-Check to Time-of-Use (TOCTOU) race, leaving BOs with stale TTM priorities. - [Medium] The BO priority evaluation incorrectly prioritizes LOW priority exec queues over NORMAL priority ones, causing unintended priority downgrades. - [Medium] Unbound private BOs miss dynamic priority updates and permanently retain stale priorities. - [Medium] The patch omits the intended logic to downgrade completely unmapped BOs to `XE_BO_PRIORITY_LOW` in `xe_vma_destroy()`. --
--- Patch [2]: [PATCH v2 2/4] drm/xe: Re-prioritize VM's private BOs when adding an exec queue --- commit 45971462729146ee32e2c293b500a521f745e5fe Author: Matthew Brost <[email protected]> Subject: drm/xe: Re-prioritize VM's private BOs when adding an exec queue This commit updates the TTM LRU eviction priority of a VM's private BOs when exec queues are added or removed. It iterates over the VM's active VMAs to apply the new priority based on the highest priority band among the VM's attached exec queues. [ ... ] > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c > index 0b315d6120aae..0f2b8ffefb8ec 100644 > --- a/drivers/gpu/drm/xe/xe_vm.c > +++ b/drivers/gpu/drm/xe/xe_vm.c [ ... ] > @@ -4912,6 +4944,87 @@ int xe_vm_alloc_cpu_addr_mirror_vma(struct xe_vm *vm, > uint64_t start, uint64_t r > return xe_vm_alloc_vma(vm, &map_req, false); > } > > +/* > + * Determine the TTM LRU priority to use for BOs private to @vm, based on the > + * highest priority band among exec queues currently attached to @vm. > + * > + * A VM with a HIGH priority exec queue gets XE_BO_PRIORITY_HIGHEST, same as > + * BOs that are never subject to this heuristic at all (kernel or shared > + * BOs) -- userspace has indicated its most important work lives here, so > + * treat these BOs as equally important. Every other band is then shifted up > + * one level from its "intuitive" mapping: a NORMAL priority exec queue (or > no > + * exec queues at all yet) yields XE_BO_PRIORITY_HIGH, and a LOW priority > exec > + * queue yields XE_BO_PRIORITY_NORMAL. XE_BO_PRIORITY_LOW is intentionally > + * never returned here -- it is reserved for xe_vma_destroy() to apply once a > + * BO has no VMAs left, letting userspace signal "drop this from my working > + * set" via unbind while still retaining the backing store, without this > + * heuristic immediately promoting it back up on the next bind. [Severity: Medium] Is the logic to downgrade completely unmapped BOs to XE_BO_PRIORITY_LOW missing from this patch? The comment states it is reserved for xe_vma_destroy() to apply once a BO has no VMAs left, but the implementation to actually apply it does not seem to be included in this changeset. > + * > + * Must be called with @vm's exec_queues.lock held, in either read or write > + * mode. > + */ > +static int xe_vm_bo_priority_locked(struct xe_vm *vm) > +{ > + lockdep_assert_held(&vm->exec_queues.lock); > + > + if (vm->exec_queues.priority_count[XE_EXEC_QUEUE_PRIORITY_HIGH]) > + return XE_BO_PRIORITY_HIGHEST; > + if (vm->exec_queues.priority_count[XE_EXEC_QUEUE_PRIORITY_LOW]) > + return XE_BO_PRIORITY_NORMAL; > + > + return XE_BO_PRIORITY_HIGH; > +} [Severity: Medium] Does this logic incorrectly prioritize LOW priority exec queues over NORMAL priority ones? If a VM has both a NORMAL and a LOW priority queue, it appears this will return XE_BO_PRIORITY_NORMAL instead of checking for the presence of a NORMAL queue first, which might unfairly downgrade the VM's priority band. > + > +/** > + * xe_vm_bo_priority() - Determine TTM LRU priority for a VM's private BOs [ ... ] > +/* > + * Update the TTM LRU priority of all BOs private to @vm to @priority, moving > + * each to the tail of its new priority level's LRU list. BOs shared with > + * other VMs (extobjs) are left untouched. > + */ > +static void xe_vm_update_bo_priority(struct xe_vm *vm, int priority) > +{ > + struct drm_gpuva *gpuva; > + > + /* > + * vm->lock is the outer most lock and serializes against concurrent > + * VMA insertion/removal (bind/unbind), so it must be held to safely > + * walk vm->gpuvm here. xe_vm_lock() (the VM's dma-resv) is taken > + * inside it, as it protects each private BO's bo->ttm.priority. > + */ > + down_read(&vm->lock); > + xe_vm_lock(vm, false); > + drm_gpuvm_for_each_va(gpuva, &vm->gpuvm) { [Severity: Medium] Can unbound private BOs miss dynamic priority updates here? Since this loop iterates over mapped VMAs using drm_gpuvm_for_each_va(), any allocated but not yet bound BOs would be skipped and might permanently retain their stale creation priorities. > + struct xe_vma *vma = gpuva_to_vma(gpuva); > + struct xe_bo *bo = xe_vma_bo(vma); > + > + if (!bo || bo->vm != vm || bo->ttm.priority == priority) > + continue; > + > + xe_bo_update_ttm_priority(bo, priority); > + } > + xe_vm_unlock(vm); > + up_read(&vm->lock); > +} [ ... ] > @@ -4941,7 +5055,10 @@ void xe_vm_add_exec_queue(struct xe_vm *vm, struct > xe_exec_queue *q) > if (q->sched_props.priority >= XE_EXEC_QUEUE_PRIORITY_LOW && > q->sched_props.priority <= XE_EXEC_QUEUE_PRIORITY_HIGH) > ++vm->exec_queues.priority_count[q->sched_props.priority]; > + priority = xe_vm_bo_priority_locked(vm); > up_write(&vm->exec_queues.lock); > + > + xe_vm_update_bo_priority(vm, priority); > } [Severity: Medium] Is there a race condition between taking the priority snapshot and applying it? If multiple threads are concurrently adding and removing exec queues, their execution could interleave between dropping exec_queues.lock and acquiring vm->lock inside xe_vm_update_bo_priority(). Could the last thread to acquire vm->lock apply a stale priority snapshot that no longer reflects the current queue state? > > /** -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
