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

["Christian König" <christian.koenig@amd.com>]

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