Re: RFC: dma_resv_unlock() and ww_acquire_ctx scope

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

On 2023-01-19 11:24, Paul Cercueil wrote:
> Hi,
>
> Just a reflexion I have after an intensive (and intense) debugging
> session.
>
> I had the following code:
>
>
> int my_dma_resv_lock(struct dma_buf *dmabuf)
> {
> 	struct ww_acquire_ctx ctx;
> 	int ret;
>
> 	ww_acquire_init(&ctx, &reservation_ww_class);
>
> 	ret = dma_resv_lock_interruptible(dmabuf->resv, &ctx);
> 	if (ret) {
> 		if (ret != -EDEADLK)
> 			return ret;
>
> 		ret = dma_resv_lock_slow_interruptible(dmabuf->resv,
> 						       &ctx);
> 	}
>
> 	return ret;
> }
>
>
> Then I would eventually unlock the dma_resv object in the caller
> function. What made me think this was okay, was that the API itself
> suggests it's okay - as dma_resv_unlock() does not take the
> "ww_acquire_ctx" object as argument, my assumption was that after the
> dma_resv was locked, the variable could go out of scope.
>
> I wonder if it would be possible to change the API a little bit, so
> that it is less prone to errors like this. Maybe by doing a struct copy
> of the initialized ctx into the dma_resv object (if at all possible),
> so that the original can actually go out of scope, or maybe having
> dma_resv_unlock() take the ww_acquire_ctx as argument, even if it is
> not actually used in the function body - just to make it obvious that
> it is needed all the way to the point where the dma_resv is unlocked.
>
> This email doesn't have to result in anything, I just thought I'd share
> one point where I feel the API could be made less error-prone.

Hey,

This example code will fail eventually. If you have DEBUG_LOCK_ALLOC 
enabled, a fake lock is inited in ww_acquire_init. If you don't free it 
using ww_acquire_fini(), lockdep will see that you free a live lock that 
was never released. PROVE_LOCKING will also complain that you never 
unlocked the ctx fake lock.

If you do call ww_acquire_fini, you will get a loud complain if you 
never released all locks, because ctx->acquired is non-zero.

Try with PROVE_LOCKING, your example will receive a lockdep splat. :)

~Maarten