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

From: Jason Gunthorpe <jgg@nvidia.com>
To: "Christian König" <christian.koenig@amd.com>
Cc: Alex Williamson <alex.williamson@redhat.com>,
	Cornelia Huck <cohuck@redhat.com>,
	dri-devel@lists.freedesktop.org, kvm@vger.kernel.org,
	linaro-mm-sig@lists.linaro.org, linux-media@vger.kernel.org,
	Sumit Semwal <sumit.semwal@linaro.org>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Leon Romanovsky <leon@kernel.org>,
	linux-rdma@vger.kernel.org, Maor Gottlieb <maorg@nvidia.com>,
	Oded Gabbay <ogabbay@kernel.org>
Subject: Re: [PATCH 0/4] Allow MMIO regions to be exported through dma-buf
Date: Thu, 18 Aug 2022 10:16:06 -0300	[thread overview]
Message-ID: <Yv47lkz7FG8vhqA5@nvidia.com> (raw)
In-Reply-To: <d12fdf94-fbef-b981-2eff-660470ceca22@amd.com>

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.

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

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.

Jason