Re: [PATCH 0/4] Allow MMIO regions to be exported through dma-buf

["Christian König" <christian.koenig@amd.com>]

Am 18.08.22 um 15:16 schrieb Jason Gunthorpe:
> On Thu, Aug 18, 2022 at 02:58:10PM +0200, Christian König wrote:
>
>>>> The only thing I'm not 100% convinced of is dma_buf_try_get(), I've seen
>>>> this incorrectly used so many times that I can't count them any more.
>>>>
>>>> Would that be somehow avoidable? Or could you at least explain the use case
>>>> a bit better.
>>> I didn't see a way, maybe you know of one
>> For GEM objects we usually don't use the reference count of the DMA-buf, but
>> rather that of the GEM object for this. But that's not an ideal solution
>> either.
> You can't really ignore the dmabuf refcount. At some point you have to
> deal with the dmabuf being asynchronously released by userspace.

Yeah, but in this case the dma-buf is just a reference to the 
real/private object which holds the backing store.

When the dma-buf is released you drop the real object reference and from 
your driver internals you only try_get only the real object.

The advantage is that only your driver can use the try_get function and 
not some importing driver which doesn't know about the internals of the 
exporter.

We just had to many cases where developers weren't sure if a pointer is 
still valid and by using try_get it just "magically" got working (well I 
have to admit it made the crashing less likely....).

>>> 	down_write(&vdev->memory_lock);
>>> 	list_for_each_entry_safe(priv, tmp, &vdev->dmabufs, dmabufs_elm) {
>>> 		if (!dma_buf_try_get(priv->dmabuf))
>>> 			continue;
>> What would happen if you don't skip destroyed dma-bufs here? In other words
>> why do you maintain that list in the first place?
> The list is to keep track of the dmabufs that were created, it is not
> optional.
>
> The only question is what do you do about invoking
> dma_buf_move_notify() on a dmabuf that is already undergoing
> destruction.

Ah, yes. Really good point.

>
> For instance undergoing destruction means the dmabuf core has already
> done this:
>
> 	mutex_lock(&db_list.lock);
> 	list_del(&dmabuf->list_node);
> 	mutex_unlock(&db_list.lock);
> 	dma_buf_stats_teardown(dmabuf);
>
> So it seems non-ideal to continue to use it.
>
> However, dma_buf_move_notify() specifically has no issue with that level of
> destruction since it only does
>
> 	list_for_each_entry(attach, &dmabuf->attachments, node)
>
> And attachments must be empty if the file refcount is zero.
>
> So we could delete the try_buf and just rely on move being safe on
> partially destroyed dma_buf's as part of the API design.

I think that might be the more defensive approach. A comment on the 
dma_buf_move_notify() function should probably be a good idea.

Thanks,
Christian.

>
> Jason