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

[Jason Gunthorpe <jgg@nvidia.com>]

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.

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.

> > 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.

Jason