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