Thank you for your contribution! Sashiko AI review found 1 potential issue(s) 
to consider:
- [Medium] Tracepoint metric lock_ns incorrectly includes lock-dropped backoff 
sleep time due to WW mutex contention retries.
--

--- Patch [26]: [PATCH v3 26/33] drm/xe: Add tracepoint for xe_gem_create_ioctl 
---
commit 08fb65ed7bb6e79b15efc1ef9667995aaebd39fe
Author: Matthew Brost <[email protected]>

drm/xe: Add tracepoint for xe_gem_create_ioctl

This commit adds a tracepoint to profile GEM creation in the xe driver.
It records the BO size, placement, CPU caching, ASID, total IOCTL
duration, and the time spent under the dma-resv lock to help evaluate
out-of-lock preallocation performance.

> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index a646a72122d40..caf5aa0bab533 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c

[ ... ]

> @@ -4100,6 +4105,10 @@ int xe_gem_create_ioctl(struct drm_device *dev, void 
> *data,
>                       if (err)
>                               break;
>               }
> +
> +             if (lock_start == ktime_set(0, 0))
> +                     lock_start = ktime_get();
> +

[Severity: Medium]
Will this lock_start timestamp incorrectly include lock-dropped backoff
sleep time when a WW mutex contention retry occurs?

When contention happens, drm_exec_cleanup() drops all locks and the next
iteration might sleep in dma_resv_lock_slow_interruptible() without holding
any dma-resv locks. Because of the zero-check guard here, lock_start is
retained from the first iteration.

Could this cause the lock_ns metric to encapsulate the unbounded lock-dropped
sleep time and corrupt the intended profiling metric? If vm != NULL, removing
this zero-check would exclude the sleep (which happens early in
xe_vm_drm_exec_lock). However, if vm == NULL, the sleep occurs inside
xe_bo_create_user(), so does this require capturing the start time inside
the locking functions or accumulating the actual duration per iteration?

>               bo = xe_bo_create_user(xe, vm, args->size, args->cpu_caching,
>                                      bo_flags, have_prealloc ? &prealloc : 
> NULL,
>                                      &exec);

[ ... ]

-- 
Sashiko AI review ยท 
https://sashiko.dev/#/patchset/[email protected]?part=26

Reply via email to