Re: [PATCH 0/2] iommu: Remove iommu_sva_ops::mm_exit()

From: Jason Gunthorpe <jgg@ziepe.ca>
To: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>,
	arnd@arndb.de, gregkh@linuxfoundation.org,
	iommu@lists.linux-foundation.org, zhangfei.gao@linaro.org,
	linux-accelerators@lists.ozlabs.org
Subject: Re: [PATCH 0/2] iommu: Remove iommu_sva_ops::mm_exit()
Date: Wed, 8 Apr 2020 16:02:26 -0300	[thread overview]
Message-ID: <20200408190226.GA11886@ziepe.ca> (raw)
In-Reply-To: <20200408113552.7888bfee@jacob-builder>

On Wed, Apr 08, 2020 at 11:35:52AM -0700, Jacob Pan wrote:
> Hi Jean,
> 
> On Wed,  8 Apr 2020 16:04:25 +0200
> Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:
> 
> > The IOMMU SVA API currently requires device drivers to implement an
> > mm_exit() callback, which stops device jobs that do DMA. This function
> > is called in the release() MMU notifier, when an address space that is
> > shared with a device exits.
> > 
> > It has been noted several time during discussions about SVA that
> > cancelling DMA jobs can be slow and complex, and doing it in the
> > release() notifier might cause synchronization issues (patch 2 has
> > more background). Device drivers must in any case call unbind() to
> > remove their bond, after stopping DMA from a more favorable context
> > (release of a file descriptor).
> > 
> > So after mm exits, rather than notifying device drivers, we can hold
> > on to the PASID until unbind(), ask IOMMU drivers to silently abort
> > DMA and Page Requests in the meantime. This change should relieve the
> > mmput() path.
>
> I assume mm is destroyed after all the FDs are closed

FDs do not hold a mmget(), but they may hold a mmgrab(), ie anything
using mmu_notifiers has to hold a grab until the notifier is
destroyed, which is often triggered by FD close.

So the exit_mmap() -> release() may happen before the FDs are
destroyed, but the final mmdrop() will be during some FD release when
the final mmdrop() happens.

But, in all the drivers I've looked at the PASID and the mmu_notifier
must have identical lifetimes.

> In VT-d, because of enqcmd and lazy PASID free we plan to hold on to the
> PASID until mmdrop.
> https://lore.kernel.org/patchwork/patch/1217762/

Why? The bind already gets a mmu_notifier which has refcounts and the
right lifetime for PASID.. This code could already be simplified by
using the mmu_notifier_get()/put() stuff.

A reason to store the PASID in the mm_struct would be if some code
needs fast access to it, but then I'm not sure how that works with
SVM_FLAG_PRIVATE_PASID ..

Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu