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
