Re: [PATCH V5 1/5] iommu/amd: Remove unnecessary locking from AMD iommu driver

[Tom Murphy <murphyt7@tcd.ie>]

>I have to admit I don't fully understand the concurrency issues here, but neither do I understand what the mutex you removed might have helped to start with.

Each range in the page tables is protected by the IO virtual address
allocator. The iommu driver allocates an IOVA range using locks before
it writes to a page table range. The IOVA allocator acts like a lock
on a specific range of the page tables. So we can handle most of the
concurrency issues in the IOVA allocator and avoid locking while
writing to a range in the page tables.

However because we have multiple levels of pages we might have to
allocate a middle page (a PMD) which covers more than the IOVA range
we have allocated.
To solve this we could use locks:

//pseudo code
lock_page_table()
if (we need to allocate middle pages) {
 //allocate the page
 //set the PMD value
}
unlock_page_table()

but we can actually avoid having any locking by doing the following:

//pseudo code
if (we need to allocate middle pages) {
 //allocate the page
 //cmpxchg64 to set the PMD if it wasn't already set since we last checked
 if (the PMD was set while since we last checked)
   //free the page we just allocated
}

In this case we can end up doing a pointless page allocate and free
but it's worth it to avoid using locks

You can see this in the intel iommu code here:
https://github.com/torvalds/linux/blob/9140d8bdd4c5a04abe181bb300378355d56990a4/drivers/iommu/intel-iommu.c#L904

>what the mutex you removed might have helped to start with.
The mutex I removed is arguably completely useless.

In the dma ops path we handle the IOVA allocations in the driver so we
can be sure a certain range is protected by the IOVA allocator.

Because the iommu ops path doesn't handle the IOVA allocations it
seems reasonable to lock the page tables to avoid two writers writing
to the same range at the same time. Without the lock it's complete
chaos and all writers can be writing to the same range at the same
time resulting in complete garbage.
BUT the locking doesn't actually make any real difference. Even with
locking we still have a race condition if two writers want to write to
the same range at the same time, the race is just whoever gets the
lock first, we still can't be sure what the result will be. So the
result is still garbage, just slightly more usable garbage because at
least the range is correct for one writer.
It just makes no sense to ever have two writers writing to the same
range and adding a lock doesn't fix that.
Already the Intel iommu ops path doesn't use locks for it's page table
so this isn't a new idea I'm just doing the same for the AMD iommu
driver

Does all that make sense?

On Tue, 20 Aug 2019 at 10:41, Christoph Hellwig <hch@infradead.org> wrote:
>
> On Thu, Aug 15, 2019 at 12:09:39PM +0100, Tom Murphy wrote:
> > We can remove the mutex lock from amd_iommu_map and amd_iommu_unmap.
> > iommu_map doesn’t lock while mapping and so no two calls should touch
> > the same iova range. The AMD driver already handles the page table page
> > allocations without locks so we can safely remove the locks.
>
> I've been looking over the code and trying to understand how the
> synchronization works.  I gues we the cmpxchg64 in free_clear_pte
> is the important point here?  I have to admit I don't fully understand
> the concurrency issues here, but neither do I understand what the
> mutex you removed might have helped to start with.