Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
From: Tom Murphy <murphyt7@tcd.ie>
To: Christoph Hellwig <hch@infradead.org>
Cc: iommu@lists.linux-foundation.org,
	Heiko Stuebner <heiko@sntech.de>,
	virtualization@lists.linux-foundation.org,
	linux-tegra@vger.kernel.org,
	Thierry Reding <thierry.reding@gmail.com>,
	Will Deacon <will@kernel.org>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	linux-samsung-soc@vger.kernel.org,
	Krzysztof Kozlowski <krzk@kernel.org>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	linux-rockchip@lists.infradead.org,
	Andy Gross <agross@kernel.org>,
	Gerald Schaefer <gerald.schaefer@de.ibm.com>,
	linux-s390@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-mediatek@lists.infradead.org,
	Matthias Brugger <matthias.bgg@gmail.com>,
	linux-arm-kernel@lists.infradead.org,
	David Woodhouse <dwmw2@infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Kukjin Kim <kgene@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>
Subject: Re: [PATCH V5 1/5] iommu/amd: Remove unnecessary locking from AMD iommu driver
Date: Sat, 24 Aug 2019 08:56:59 +0100
Message-ID: <CALQxJussiGDzWFT1xhko6no5jZNOezWCFuJQUCr4XwH4NHri3Q@mail.gmail.com> (raw)
In-Reply-To: <20190820094143.GA24154@infradead.org>

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

  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-15 11:09 [PATCH V5 0/5] iommu/amd: Convert the AMD iommu driver to the dma-iommu api Tom Murphy
2019-08-15 11:09 ` [PATCH V5 1/5] iommu/amd: Remove unnecessary locking from AMD iommu driver Tom Murphy
2019-08-20  9:41   ` Christoph Hellwig
2019-08-24  7:56     ` Tom Murphy [this message]
2019-08-24 22:41       ` Christoph Hellwig
2019-08-15 11:09 ` [PATCH V5 2/5] iommu: Add gfp parameter to iommu_ops::map Tom Murphy
2019-08-19 18:23   ` Robin Murphy
2019-08-20  9:41   ` Christoph Hellwig
2019-08-15 11:09 ` [PATCH V5 3/5] iommu/dma-iommu: Handle deferred devices Tom Murphy
2019-08-19 18:26   ` Robin Murphy
2019-08-20  9:43   ` Christoph Hellwig
2019-08-15 11:09 ` [PATCH V5 4/5] iommu/dma-iommu: Use the dev->coherent_dma_mask Tom Murphy
2019-08-19 18:39   ` Robin Murphy
2019-08-20  9:43   ` Christoph Hellwig
2019-08-15 11:09 ` [PATCH V5 5/5] iommu/amd: Convert AMD iommu driver to the dma-iommu api Tom Murphy
     [not found] ` <20190817033914.4812-1-hdanton@sina.com>
2019-08-17  7:19   ` [PATCH V5 3/5] iommu/dma-iommu: Handle deferred devices Tom Murphy
2019-09-05  6:18 ` [PATCH V5 0/5] iommu/amd: Convert the AMD iommu driver to the dma-iommu api Christoph Hellwig

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CALQxJussiGDzWFT1xhko6no5jZNOezWCFuJQUCr4XwH4NHri3Q@mail.gmail.com \
    --to=murphyt7@tcd.ie \
    --cc=agross@kernel.org \
    --cc=dwmw2@infradead.org \
    --cc=gerald.schaefer@de.ibm.com \
    --cc=hch@infradead.org \
    --cc=heiko@sntech.de \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jean-philippe@linaro.org \
    --cc=jonathanh@nvidia.com \
    --cc=kgene@kernel.org \
    --cc=krzk@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=matthias.bgg@gmail.com \
    --cc=robin.murphy@arm.com \
    --cc=thierry.reding@gmail.com \
    --cc=virtualization@lists.linux-foundation.org \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org linux-arm-msm@archiver.kernel.org
	public-inbox-index linux-arm-msm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox