Am 19.08.22 um 15:11 schrieb Jason Gunthorpe:
On Thu, Aug 18, 2022 at 03:37:01PM +0200, Christian König wrote:
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.
The gem approach is backwards to what I did here.
As I said, what GEM does is not necessary the best approach either.
GEM holds a singleton pointer to the dmabuf and holds a reference on
it as long as it has the pointer. This means the dmabuf can not be
freed until the GEM object is freed.
For this I held a "weak reference" on the dmabuf in a list, and we
convert the weak reference to a strong reference in the usual way
using a try_get.
The reason it is different is because the VFIO interface allows
creating a DMABUF with unique parameters on every user request. Eg the
user can select a BAR index and a slice of the MMIO space unique to
each each request and this results in a unique DMABUF.
Due to this we have to store a list of DMABUFs and we need the
DMABUF's to clean up their memory when the user closes the file.
Yeah, that makes sense.
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.
IMHO, it is an anti-pattern. The caller should hold a strong reference
on an object before invoking any API surface. Upgrading a weak
reference to a strong reference requires the standard "try get" API.
But if you feel strongly I don't mind dropping the try_get around move.
Well I see it as well that both approaches are not ideal, but my gut
feeling tells me that just documenting that dma_buf_move_notify() can
still be called as long as the release callback wasn't called yet is
probably the better approach.
On the other hand this is really just a gut feeling without strong
arguments backing it. So if somebody has an argument which makes try_get
necessary I'm happy to hear it.
Regards,
Christian.
Jason
