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
