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

On 8/5/26 09:27, Christian König wrote:
...>> +struct dma_buf_io_fence {
>> +	struct dma_fence base;
>> +	spinlock_t lock;
>> +};
> 
> Upstream has change to allow embedding the spinlock into the dma_fence, so this structure here is most likely not necessary any more.

ok

>> +static const char *dma_buf_io_fence_drv_name(struct dma_fence *fence)
>> +{
>> +	/* default fence release kfree's the base pointer */
>> +	BUILD_BUG_ON(offsetof(struct dma_buf_io_fence, base));
>> +
>> +	return "dma-buf-io-ctx";
>> +}
...>> +static void dma_buf_io_map_release_work(struct work_struct *work)
>> +{
>> +	struct dma_buf_io_map *map = container_of(work, struct dma_buf_io_map,
>> +						  release_work);
>> +	struct dma_buf_io_fence *fence = map->fence;
>> +	struct dma_buf_io_ctx *ctx = map->ctx;
>> +	struct dma_buf *dmabuf = ctx->dmabuf;
>> +
>> +	/* the release path must wait for fences */
>> +	if (WARN_ON_ONCE(refcount_read(&ctx->refs) == 0))
>> +		return;
> 
> Stuff like that is usually illegal.

Should be fine, it's just a warning. The map holds a ctx
reference so can't be 0. IIRC, it was synchronised a bit
differently before. I can kill it, refcount_inc() has the
same warning anyway.

> And why are you using refcount directly instead of kref?

Not sure it'd make much difference here.

> 
>> +
>> +	/* Prevent from destoying the ctx while unmapping */
>> +	refcount_inc(&ctx->refs);
> 
>> +
>> +	/*
>> +	 * 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.
> 
> Are you sure you actually need and want a dma_fence here?

Waiting for potentially a ton of IO synchronously on invalidate
sounds like a bad idea though. Hmm.

>> +
>> +	dma_resv_lock(dmabuf->resv, NULL);
>> +	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.

The map is freed two lines below, but I can add it as
a defensive measure.

>> +	percpu_ref_exit(&map->refs);
>> +	kfree(map);

...>> +struct dma_buf_io_map *dma_buf_io_create_map(struct dma_buf_io_ctx *ctx)
>> +{
>> +	struct dma_buf *dmabuf = ctx->dmabuf;
>> +	struct dma_buf_io_map *map;
>> +	long ret;
>> +
>> +retry:
>> +	/*
>> +	 * ->dmabuf_map() will be calling dma_buf_map_attachment(), for which
>> +	 * we'll need to wait for fences. Do a bit nicer and try to wait
>> +	 * without the resv lock first.
>> +	 */
> 
> Clear NAK to that. Always wait while holding the resv lock if you can!
> 
> It is absolutely not beneficial to do this outside of the lock and usually just hides problems instead and prevent fixing them.

Ok

...
>> +	ret = dma_resv_reserve_fences(dmabuf->resv, 1);
>> +	if (WARN_ON_ONCE(ret)) {
>> +		struct dma_fence *fence = &map->fence->base;
>> +
>> +		dma_fence_get(fence);
>> +		percpu_ref_kill(&map->refs);
>> +		dma_fence_wait(fence, false);
>> +		dma_fence_put(fence);
>> +		return;
>> +	}
>> +
>> +	dma_resv_add_fence(dmabuf->resv, &map->fence->base,
>> +			   DMA_RESV_USAGE_KERNEL);
> 
> That sequence is clearly incorrect!
> 
> The fence must be created after dma_resv_reserve_fences(), otherwise you definately have an illegal memory operation here.

I'm not sure what you mean, can you elaborate? I only cared about
pre-allocating it to avoid allocations here. We add / signal the fence
only once, no reuse. The map is going to be killed here, and if we
create a new map, it'll have its own fence.

I can move the dma_fence_init() call here if that makes a difference?

-- 
Pavel Begunkov