Re: [Intel-xe] [RFC PATCH] dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling()

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

Daniel,

On 4/28/23 14:52, Thomas Hellström wrote:
> Condsider the following call sequence:
>
> /* Upper layer */
> dma_fence_begin_signalling();
> lock(tainted_shared_lock);
> /* Driver callback */
> dma_fence_begin_signalling();
> ...
>
> The driver might here use a utility that is annotated as intended for the
> dma-fence signalling critical path. Now if the upper layer isn't correctly
> annotated yet for whatever reason, resulting in
>
> /* Upper layer */
> lock(tainted_shared_lock);
> /* Driver callback */
> dma_fence_begin_signalling();
>
> We will receive a false lockdep locking order violation notification from
> dma_fence_begin_signalling(). However entering a dma-fence signalling
> critical section itself doesn't block and could not cause a deadlock.
>
> So use a successful read_trylock() annotation instead for
> dma_fence_begin_signalling(). That will make sure that the locking order
> is correctly registered in the first case, and doesn't register any
> locking order in the second case.
>
> The alternative is of course to make sure that the "Upper layer" is always
> correctly annotated. But experience shows that's not easily achievable
> in all cases.
>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

Resurrecting the discussion on this one. I can't see a situation where 
we would miss *relevant* locking
order violation warnings with this patch. Ofc if we have a scheduler 
annotation patch that would work fine as well, but the lack of 
annotation in the scheduler callbacks is really starting to hurt us.

Thanks,

Thomas



> ---
>   drivers/dma-buf/dma-fence.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index f177c56269bb..17f632768ef9 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -308,8 +308,8 @@ bool dma_fence_begin_signalling(void)
>   	if (in_atomic())
>   		return true;
>   
> -	/* ... and non-recursive readlock */
> -	lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL, _RET_IP_);
> +	/* ... and non-recursive successful read_trylock */
> +	lock_acquire(&dma_fence_lockdep_map, 0, 1, 1, 1, NULL, _RET_IP_);
>   
>   	return false;
>   }
> @@ -340,7 +340,7 @@ void __dma_fence_might_wait(void)
>   	lock_map_acquire(&dma_fence_lockdep_map);
>   	lock_map_release(&dma_fence_lockdep_map);
>   	if (tmp)
> -		lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL, _THIS_IP_);
> +		lock_acquire(&dma_fence_lockdep_map, 0, 1, 1, 1, NULL, _THIS_IP_);
>   }
>   #endif
>