On 10/26/26 19:31, Felix Kuehling wrote: > On 2026-10-26 10:15, Philipp Stanner wrote: >> amdgpu's KFD eviction fence's dma_fence ops->enable_signaling() contains >> a dma_fence_is_signaled() check. >> >> ops->enable_signaling() is currently consistently being called with the >> dma_fence lock held. dma_fence_is_signaled() might try to take this lock >> in its dma_fence_signal() call. >> >> This deadlock can currently not occur because the amdkfd_fence_ops do >> not implement ops->signaled. Should that ever be added, however, >> deadlocks might occur occasionally once the fastpath check in >> dma_fence_is_signaled() is skipped. >> >> Replace dma_fence_is_signaled() with dma_fence_is_signaled_locked(). >> >> Signed-off-by: Philipp Stanner <[email protected]> >> --- >> As detailed in my recent discussions with Christian [1], I also think >> that drivers should be very tightly constrained as to what they do in >> the fence_ops. I think at the very least they shouldn't use any >> dma_fence functions there. >> >> So we could also remove this check here completely; >> __dma_fence_enable_signaling() already checks the signaled-flag, after >> all, but I'm not sure whether that might break amdgpu. > > Looking at the code, I think it should be safe to remove the check. There is > another one in kgd2kfd_schedule_evict_and_restore_process, which is called > from amdkfd_fence_enable_signaling, which is also redundant. > > We don't implement ops->signaled, and we explicitly signal the fence from SW > with dma_fence_signal or dma_fence_check_and_signal. So the signaled flag > should be the only thing that needs to be checked. The flag is only set while > holding the fence lock, and checked while holding the fence lock. So in > theory, relying on the check in __dma_fence_enable_signaling should be safe.
I actually had a patch to remove that one send out month ago already, it was just never merged. The call absolutely doesn't make sense since the ops->signaled callback isn't implemented and the bit tested before the enable signaling callback called. I can take care and push the removal to amd-staging-drm-next when I find time. Regards, Christian. > > Regards, > Felix > > >> >> >> P. >> >> [1] >> https://lore.kernel.org/dri-devel/[email protected]/ >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c >> index 9b10d015671c..61621eebb2b0 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c >> @@ -123,7 +123,8 @@ static bool amdkfd_fence_enable_signaling(struct >> dma_fence *f) >> if (!fence) >> return false; >> - if (dma_fence_is_signaled(f)) >> + /* The enable_signaling() ops is called under lock protection. */ >> + if (dma_fence_is_signaled_locked(f)) >> return true; >> if (!kgd2kfd_schedule_evict_and_restore_process(fence->mm, >> fence->context_id, f))
