Re: RFC: dma_resv_unlock() and ww_acquire_ctx scope

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux