Re: [Linaro-mm-sig] Re: [PATCH v4 01/14] dma-buf: introduce initial file I/O infrastructure

[Pavel Begunkov]

On 8/6/26 02:37, Matthew Brost wrote:
> On Wed, Aug 05, 2026 at 10:27:49AM +0200, Christian König wrote:
> > On 7/28/26 23:29, Pavel Begunkov wrote:
...>>> +	/*
> > > +	 * There are no more requests using the map, we can signal the fence.
> > > +	 * It should be done before taking the resv lock as someone could be
> > > +	 * waiting for the fence while holding the lock.
> > > +	 */
> > > +	dma_fence_signal(&fence->base);
> >
> > Signaling fences has a whole bunch of very strict rules associated with it. E.g. you can't alocate memory for example.
>
> Yes, and the rules around signaling fences from worker threads become
> interesting as well. In practice, the entire workqueue (or any work item
> scheduled on that workqueue) effectively becomes part of the fence
> signaling and reclaim path.
>
> > Are you sure you actually need and want a dma_fence here?
> >
> > > +
> > > +	dma_resv_lock(dmabuf->resv, NULL);
>
> So this is illegal because code is allowed to hold dma-resv locks while
> waiting on dma-fences. See dma_resv_lockdep / __dma_fence_might_wait /
> dma_fence_begin_signalling.
>
> > > +	ctx->dev_ops->unmap(ctx, map);
> > > +	dma_resv_unlock(dmabuf->resv);
> > > +
> > > +	dma_fence_put(&fence->base);
> >
> > You should probably set map->fence to NULL after that.
> >
> > > +	percpu_ref_exit(&map->refs);
> > > +	kfree(map);
> > > +
> > > +	if (refcount_dec_and_test(&ctx->refs)) {
> > > +		/*
> > > +		 * Destruction needs to wait for I/O and dma fences. Defer it to
> > > +		 * simplify locking.
> > > +		 */
> > > +		INIT_WORK(&ctx->release_work, dma_buf_io_ctx_destroy_work);
> > > +		queue_work(system_wq, &ctx->release_work);
> > > +	}
> > > +}
> > > +
> > > +static void dma_buf_io_map_refs_release(struct percpu_ref *ref)
> > > +{
> > > +	struct dma_buf_io_map *map = container_of(ref, struct dma_buf_io_map, refs);
> > > +
> > > +	/* might sleep, use a worker */
> > > +	INIT_WORK(&map->release_work, dma_buf_io_map_release_work);
> > > +	queue_work(system_wq, &map->release_work);
>
> You can't guarantee that a GFP_KERNEL allocation won't be performed from
> a system worker thread, so you can't safely signal a fence from one. The
> pathological case is when all threads in system_wq are running work
> items that perform GFP_KERNEL allocations, enter reclaim, and then wait
> on a fence that is signaled by another work item queued on system_wq.
> Since all worker threads are occupied, the signaling work item cannot be
> scheduled, resulting in a deadlock. We actually hit this exact deadlock
> early in Xe.
>
> So, as Christian says, think carefully about whether you really need a
> dma-fence here. If you do, then you need to play by the rules.

I started with moving it out of wq, which was a good idea anyway,
but as mentioned in another email, in the end I just got rid of
fences as I believe Christian was suggesting / hinting on. I cc'ed
you on v6 if you'd be curious.

--
Pavel Begunkov