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