Received-SPF: pass (google.com: domain of linux-kernel+bounces-162501-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 29 Apr 2024 11:56:29 -0300
From: Jason Gunthorpe <jgg@ziepe.ca>
To: Pasha Tatashin <pasha.tatashin@soleen.com>
Cc: akpm@linux-foundation.org, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org, rientjes@google.com,
	dwmw2@infradead.org, baolu.lu@linux.intel.com, joro@8bytes.org,
	will@kernel.org, robin.murphy@arm.com, iommu@lists.linux.dev
Subject: Re: [RFC v2 2/3] iommu/intel: synchronize page table map and unmap
 operations
Message-ID: <20240429145629.GO231144@ziepe.ca>
References: <20240426034323.417219-1-pasha.tatashin@soleen.com>
 <20240426034323.417219-3-pasha.tatashin@soleen.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240426034323.417219-3-pasha.tatashin@soleen.com>

On Fri, Apr 26, 2024 at 03:43:22AM +0000, Pasha Tatashin wrote:
> Since, we are going to update  parent page table entries when lower
> level page tables become emtpy and we add them to the free list.
> We need a way to synchronize the operation.
> 
> Use domain->pgd_lock to protect all map and unmap operations.
> This is reader/writer lock. At the beginning everything is going to be
> read only mode, however, later, when free page table on unmap is added
> we will add a writer section as well.
> 
> Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
> ---
>  drivers/iommu/intel/iommu.c | 21 +++++++++++++++++++--
>  drivers/iommu/intel/iommu.h |  3 +++
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 1bfb6eccad05..8c7e596728b5 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -995,11 +995,13 @@ static void dma_pte_free_pagetable(struct dmar_domain *domain,
>  				   unsigned long last_pfn,
>  				   int retain_level)
>  {
> +	read_lock(&domain->pgd_lock);

I think no to this.

This is a very performance sensitive path for the DMA API, we really
do want to see a lockless RCU scheme to manage this overhead here.

This would be fine for a VFIO user, which I guess is your use case.

IMHO it is not a good idea to fiddle around the edges like this. We
need to get the iommu code to having shared algorithms for the radix
tree so we can actually implement something good here and share
it. Every driver has the same problem and needs the same complicated
fix.

I keep threatening to work on that but have yet to start..

Jason