From: "Sakac, Pavol" <sakacpav@amazon.de>
To: Robin Murphy <robin.murphy@arm.com>,
Joerg Roedel <joro@8bytes.org>, "Will Deacon" <will@kernel.org>
Cc: "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Bjorn Helgaas <bhelgaas@google.com>,
"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
"nh-open-source@amazon.com" <nh-open-source@amazon.com>
Subject: Re: [RFC PATCH 0/3] iommu: Reduce iommu_probe_device_lock contention
Date: Thu, 17 Sep 2026 20:21:59 +0000 [thread overview]
Message-ID: <d59e7e89-2ab4-441b-851c-417d0177b9a1@amazon.de> (raw)
In-Reply-To: <1785d4ce-03f5-4226-9987-d267a10a360d@arm.com>
On 9/11/26 19:45, Robin Murphy wrote:
> On 11/09/2026 1:58 pm, Pavol Sakac wrote:
>> iommu_probe_device_lock is a file-scope mutex held across
>> __iommu_probe_device(): every device's IOMMU probe serializes against
>> every other. With "PCI/IOV: Initialize virtual functions in
>> parallel" [1] fanning an SR-IOV enable across CPUs, it collects 94.7%
>> of all lock wait. Patch 1 gathers per-member sysfs publication into one
>> all-or-nothing helper (no functional change); patches 2-3 move that
>> publication and first-device default-domain setup into per-group
>> finalisation after the global unlock, under group->mutex, mirroring the
>> removal path and bus_iommu_probe().
>>
>> Lock statistics and SR-IOV init time for 4x PF (NVMe, 255 VFs each), on
>> the reproducer from the parallel VF initialization cover letter [1]:
>>
>> lock_stat:
>> Lock wait: Before After contentions: Before After
>> iommu_probe_device_lock 25507 ms 10471 ms 1143 841
>> &root->kernfs_rwsem 942 ms 1834 ms 93208 116316
>> &vfio.group_lock 425 ms 3614 ms 497 736
>>
>> avg wait per acquisition 6.3 ms -> 2.6 ms (4080 acq., both arms)
>>
>> Stage SR-IOV init time:
>> S0 (baseline) 3027 ms
>> S1 999 ms
>> S2 (this series) 995 ms
>>
>> Reproducer disclaimer:
>> I lean primarily on lock_stat numbers to defend the improvements. In
>> the reproducer, this lock's residual hold dominates the window and
>> masks the later series' wall-time gains, more in [1].
>>
>> This is relief, not removal: the lock still has the highest wait
>> time in the profile after this series. Wait per acquisition drops
>> from 6.3 ms to 2.6 ms, which is what makes the smaller
>> serialization points behind it measurable for the later series.
>>
>> RFC on the direction: The comment in __iommu_probe_device() expects
>> the lock to narrow to device_lock() once the ACPI/OF replay calls
>> are cleaned up. I could not make that work for the whole section:
>> group formation in ops->device_group() is a cross-device decision a
>> per-device lock cannot order. This series instead moves the work
>> that needs no global ordering out of the section. Is that an
>> acceptable step, or is there a scoping or removal plan this should
>> wait for?
>
> The point of probe_device_lock is to prevent multiple threads trying to
> probe the *same* device concurrently; it protects the per-device state
> of dev->iommu and dev->iommu_group until the latter is assigned or the
> former is cleaned up (depending on how the probe goes). The replay calls
> are mostly gone, but the main reason device_lock() still won't work is
> that the same problem exists for driver-model-based IOMMU drivers
> themselves, since we don't have a good way to avoid bus_iommu_probe()
> deadlocking on IOMMU devices that are in the middle of registering
> during their own driver bind (not least the caller itself).
>
Thanks, that makes sense.
>> A second question: I have measured where 90% of the residual hold goes:
>> get_pci_alias_group() walks every PCI device in the system to find
>> same-bus DMA aliases. It runs once per device probed, so once per VF,
>> under this lock, and the VFs keep growing the list it walks. A prototype
>> that skips the walk when no device has a dma_alias_mask and this device
>> has no pci_real_dma_dev() override cuts this lock's hold time by about
>> 90%, for the same acquisitions and the same groups. I am not proposing
>> it here, as I do not have the time to get it right this cycle. How can
>> we optimize this preferably in O(1) time?
>
> TBH that makes it sound like optimising pci_device_group() is the better
> thing to do. We were never really meant to have a global lock here - it
> was just an acceptable compromise for simplicity at the time - so I'm
> still not keen on adding yet more complexity to the probe flow to work
> around it as if global serialisation was necessary when it isn't.
>
> Heck, even if you do just want a quick bodge to ease contention then I'd
> still lean more towards something more self-contained like this
> hometime-on-a-Friday fun I couldn't resist sketching out...
>
> Thanks,
> Robin.
>
I appreciate the sketch :)
I've tested it with table size of 16, but it's not enough and needs also
get_pci_alias_group() walking scope reduction fix so it does not iterate
all devices but just those on the bus - as it itself claims to only need.
I left get_pci_function_alias_group() as is - VFs skip the walk there.
When both are combined (in below table as arm C), they work the best and
finally put kernfs_rwsem to the top. Here are numbers from the reproducer
running with all of my patches from S1-S5 series:
Arm:
- A the 3 iommu patches as posted
- B purely your lock table with 16 entries replacing my 3
- C B + the alias-walk fix
- D just alias-walk fix alone
A B C D
iommu_probe_device_lock wait 10391 ms 19538 ms 1198 ms 8237 ms
iommu_probe_device_lock hold 3338 ms 15074 ms 1002 ms 754 ms
iommu_probe_device_lock cont 836 731 384 1134
&root->kernfs_rwsem wait 660 ms 949 ms 2129 ms 835 ms
gdp_mutex wait 162 ms 189 ms 1139 ms 154 ms
&k->k_lock acquisitions 1195808 1201016 86824 87242
&k->k_lock contentions 2378 997420 4567 2261
all classes, wait 11249 ms 26730 ms 4497 ms 9237 ms
SR-IOV init time 1152 ms 1600 ms 923 ms 1012 ms
runs (min over) 5 3 3 3
Also ran different table sizes sweep of arm C:
4x 16x 64x
iommu_probe_device_lock wait 4638 ms 1198 ms 447 ms
iommu_probe_device_lock hold 854 ms 1002 ms 1423 ms
iommu_probe_device_lock cont 957 384 121
&root->kernfs_rwsem wait 1764 ms 2129 ms 3522 ms
gdp_mutex wait 665 ms 1139 ms 1713 ms
&k->k_lock acquisitions 86975 86824 86876
all classes, wait 7081 ms 4497 ms 5709 ms
SR-IOV init time 1000 ms 923 ms 1007 ms
runs (min over) 3 3 3
Would you like to take the first one? I can post v2 with just the alias
walk fix.
> ----->8-----
>
> From: Robin Murphy <robin.murphy@arm.com>
> Subject: [PATCH] UNTESTED: iommu: Reduce iommu_probe_device_lock contention
>
> The purpose of iommu_probe_device_lock was to prevent multiple threads
> trying to probe the same device concurrently, it's only global for the
> sake of simplicity, as there are still reasons why we can't use
> device_lock(), and adding a whole other lock to struct device itself
> just for this would be unreasonable.
>
> However, we're now getting sufficiently large systems with enough
> devices to start seeing significant contention on this lock, so let's
> scale it to a lock table to reduce contention between unrelated devices.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> drivers/acpi/scan.c | 6 +++---
> drivers/iommu/iommu.c | 37 +++++++++++++++++++++++++------------
> drivers/iommu/of_iommu.c | 6 +++---
> include/linux/iommu.h | 2 +-
> 4 files changed, 32 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index f48715ed827c..33d6a758042a 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1620,10 +1620,10 @@ static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
> int err;
>
> /* Serialise to make dev->iommu stable under our potential fwspec */
> - mutex_lock(&iommu_probe_device_lock);
> + mutex_lock(iommu_probe_device_lock(dev));
> /* If we already translated the fwspec there is nothing left to do */
> if (dev_iommu_fwspec_get(dev)) {
> - mutex_unlock(&iommu_probe_device_lock);
> + mutex_unlock(iommu_probe_device_lock(dev));
> return 0;
> }
>
> @@ -1633,7 +1633,7 @@ static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
> if (err && err != -EPROBE_DEFER)
> err = viot_iommu_configure(dev);
>
> - mutex_unlock(&iommu_probe_device_lock);
> + mutex_unlock(iommu_probe_device_lock(dev));
>
> return err;
> }
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index cd1bca7ede9a..11519e195aaf 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -381,9 +381,9 @@ int iommu_mock_device_add(struct device *dev, struct iommu_device *iommu)
> {
> int rc;
>
> - mutex_lock(&iommu_probe_device_lock);
> + mutex_lock(iommu_probe_device_lock(dev));
> rc = iommu_fwspec_init(dev, iommu->fwnode);
> - mutex_unlock(&iommu_probe_device_lock);
> + mutex_unlock(iommu_probe_device_lock(dev));
>
> if (rc)
> return rc;
> @@ -400,7 +400,7 @@ static struct dev_iommu *dev_iommu_get(struct device *dev)
> {
> struct dev_iommu *param = dev->iommu;
>
> - lockdep_assert_held(&iommu_probe_device_lock);
> + lockdep_assert_held(iommu_probe_device_lock(dev));
>
> if (param)
> return param;
> @@ -457,7 +457,7 @@ void dev_iommu_priv_set(struct device *dev, void *priv)
> {
> /* FSL_PAMU does something weird */
> if (!IS_ENABLED(CONFIG_FSL_PAMU))
> - lockdep_assert_held(&iommu_probe_device_lock);
> + lockdep_assert_held(iommu_probe_device_lock(dev));
> dev->iommu->priv = priv;
> }
> EXPORT_SYMBOL_GPL(dev_iommu_priv_set);
> @@ -483,9 +483,9 @@ static int iommu_init_device(struct device *dev)
> * found no IOMMU to wait for, so there's no point calling it again.
> */
> if (!dev->iommu->fwspec && !dev->driver && dev->bus->dma_configure) {
> - mutex_unlock(&iommu_probe_device_lock);
> + mutex_unlock(iommu_probe_device_lock(dev));
> dev->bus->dma_configure(dev);
> - mutex_lock(&iommu_probe_device_lock);
> + mutex_lock(iommu_probe_device_lock(dev));
> /* If another instance finished the job for us, skip it */
> if (!dev->iommu || dev->iommu_group)
> return -ENODEV;
> @@ -622,7 +622,20 @@ static struct iommu_domain *pasid_array_entry_to_domain(void *entry)
> return ((struct iommu_attach_handle *)xa_untag_pointer(entry))->domain;
> }
>
> -DEFINE_MUTEX(iommu_probe_device_lock);
> +static struct mutex __iommu_probe_device_lock[4] = {
> + __MUTEX_INITIALIZER(iommu_probe_device_lock),
> + __MUTEX_INITIALIZER(iommu_probe_device_lock),
> + __MUTEX_INITIALIZER(iommu_probe_device_lock),
> + __MUTEX_INITIALIZER(iommu_probe_device_lock),
> +};
> +
> +struct mutex *iommu_probe_device_lock(const struct device *dev)
> +{
> + int hash = ((uintptr_t)dev / roundup_pow_of_two(sizeof(*dev))) %
> + ARRAY_SIZE(__iommu_probe_device_lock);
> +
> + return __iommu_probe_device_lock + hash;
> +}
>
> static int __iommu_probe_device(struct device *dev, struct list_head *group_list)
> {
> @@ -637,7 +650,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
> * probably be able to use device_lock() here to minimise the scope,
> * but for now enforcing a simple global ordering is fine.
> */
> - lockdep_assert_held(&iommu_probe_device_lock);
> + lockdep_assert_held(iommu_probe_device_lock(dev));
>
> /* Device is probed already if in a group */
> if (dev->iommu_group)
> @@ -711,9 +724,9 @@ int iommu_probe_device(struct device *dev)
> const struct iommu_ops *ops;
> int ret;
>
> - mutex_lock(&iommu_probe_device_lock);
> + mutex_lock(iommu_probe_device_lock(dev));
> ret = __iommu_probe_device(dev, NULL);
> - mutex_unlock(&iommu_probe_device_lock);
> + mutex_unlock(iommu_probe_device_lock(dev));
> if (ret)
> return ret;
>
> @@ -1803,9 +1816,9 @@ static int probe_iommu_group(struct device *dev, void *data)
> struct list_head *group_list = data;
> int ret;
>
> - mutex_lock(&iommu_probe_device_lock);
> + mutex_lock(iommu_probe_device_lock(dev));
> ret = __iommu_probe_device(dev, group_list);
> - mutex_unlock(&iommu_probe_device_lock);
> + mutex_unlock(iommu_probe_device_lock(dev));
> if (ret == -ENODEV)
> ret = 0;
>
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index a18bb60f6f3d..b5e3a425ca2d 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -121,9 +121,9 @@ int of_iommu_configure(struct device *dev, struct device_node *master_np,
> return -ENODEV;
>
> /* Serialise to make dev->iommu stable under our potential fwspec */
> - mutex_lock(&iommu_probe_device_lock);
> + mutex_lock(iommu_probe_device_lock(dev));
> if (dev_iommu_fwspec_get(dev)) {
> - mutex_unlock(&iommu_probe_device_lock);
> + mutex_unlock(iommu_probe_device_lock(dev));
> return 0;
> }
> dev_iommu_present = dev->iommu;
> @@ -151,7 +151,7 @@ int of_iommu_configure(struct device *dev, struct device_node *master_np,
> iommu_fwspec_free(dev);
> else if (err && dev->iommu)
> dev_iommu_free(dev);
> - mutex_unlock(&iommu_probe_device_lock);
> + mutex_unlock(iommu_probe_device_lock(dev));
>
> /*
> * If we're not on the iommu_probe_device() path (as indicated by the
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index ac43b8b93f14..3b876cb285e0 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -1201,7 +1201,7 @@ static inline void *dev_iommu_priv_get(struct device *dev)
>
> void dev_iommu_priv_set(struct device *dev, void *priv);
>
> -extern struct mutex iommu_probe_device_lock;
> +struct mutex *iommu_probe_device_lock(const struct device *dev);
> int iommu_probe_device(struct device *dev);
>
> int iommu_device_use_default_domain(struct device *dev);
> --
> 2.54.0.dirty
>
Here's the alias walk fix prototype (only tested on VFs though):
-----
From: Pavol Sakac <sakacpav@amazon.de>
Date: Thu, 17 Sep 2026 13:00:00 +0200
Subject: [PATCH] iommu: Find PCI DMA aliases on the device's own bus
get_pci_alias_group() walks every PCI device in the system with
for_each_pci_dev() to find the aliases of one device. It runs once per
probed device under iommu_probe_device_lock and takes the PCI bus
klist lock twice per step.
A DMA alias is a devfn on the same bus, so walk only that bus, with
pci_walk_bus(). The recursion walks the bus itself and cannot nest
inside pci_walk_bus(), so fetch one alias per pass and let the visited
bit the recursion sets move the next pass on; a visited alias is no
longer re-entered only to return NULL.
No functional change intended.
Assisted-by: LLM
Signed-off-by: Pavol Sakac <sakacpav@amazon.de>
---
drivers/iommu/iommu.c | 55 ++++++++++++++++++++++++++++--------------
1 file changed, 37 insertions(+), 18 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index cd1bca7..3bf1020 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1521,6 +1521,35 @@ static struct iommu_group *get_pci_function_alias_group(struct pci_dev *pdev,
return NULL;
}
+struct pci_alias_search {
+ struct pci_dev *pdev;
+ unsigned long *devfns;
+ struct pci_dev *alias;
+};
+
+static int find_pci_dma_alias(struct pci_dev *tmp, void *data)
+{
+ struct pci_alias_search *s = data;
+
+ /* pci_walk_bus() descends below the bus; aliases are same-bus only */
+ if (tmp->bus != s->pdev->bus ||
+ test_bit(tmp->devfn & 0xff, s->devfns) ||
+ !pci_devs_are_dma_aliases(s->pdev, tmp))
+ return 0;
+
+ s->alias = pci_dev_get(tmp);
+ return 1;
+}
+
+static struct pci_dev *get_pci_dma_alias(struct pci_dev *pdev,
+ unsigned long *devfns)
+{
+ struct pci_alias_search s = { .pdev = pdev, .devfns = devfns };
+
+ pci_walk_bus(pdev->bus, find_pci_dma_alias, &s);
+ return s.alias;
+}
+
/*
* Look for aliases to or from the given device for existing groups. DMA
* aliases are only supported on the same bus, therefore the search
@@ -1533,7 +1562,7 @@ static struct iommu_group *get_pci_function_alias_group(struct pci_dev *pdev,
static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
unsigned long *devfns)
{
- struct pci_dev *tmp = NULL;
+ struct pci_dev *tmp;
struct iommu_group *group;
if (test_and_set_bit(pdev->devfn & 0xff, devfns))
@@ -1543,24 +1572,14 @@ static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
if (group)
return group;
- for_each_pci_dev(tmp) {
- if (tmp == pdev || tmp->bus != pdev->bus)
- continue;
-
- /* We alias them or they alias us */
- if (pci_devs_are_dma_aliases(pdev, tmp)) {
- group = get_pci_alias_group(tmp, devfns);
- if (group) {
- pci_dev_put(tmp);
- return group;
- }
-
+ /* One alias per walk: the recursion cannot nest in pci_walk_bus() */
+ while ((tmp = get_pci_dma_alias(pdev, devfns))) {
+ group = get_pci_alias_group(tmp, devfns);
+ if (!group)
group = get_pci_function_alias_group(tmp, devfns);
- if (group) {
- pci_dev_put(tmp);
- return group;
- }
- }
+ pci_dev_put(tmp);
+ if (group)
+ return group;
}
return NULL;
--
2.47.1
prev parent reply other threads:[~2026-09-17 20:22 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-11 12:58 Pavol Sakac
2026-09-11 12:58 ` [RFC PATCH 1/3] iommu: split sysfs link publication out of iommu_group_alloc_device() Pavol Sakac
2026-09-11 12:58 ` [RFC PATCH 2/3] iommu: create device sysfs links outside iommu_probe_device_lock Pavol Sakac
2026-09-11 12:58 ` [RFC PATCH 3/3] iommu: set up the default domain " Pavol Sakac
2026-09-11 17:45 ` [RFC PATCH 0/3] iommu: Reduce iommu_probe_device_lock contention Robin Murphy
2026-09-17 20:21 ` Sakac, Pavol [this message]
Reply instructions:
You may reply publicly 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=d59e7e89-2ab4-441b-851c-417d0177b9a1@amazon.de \
--to=sakacpav@amazon.de \
--cc=bhelgaas@google.com \
--cc=iommu@lists.linux.dev \
--cc=joro@8bytes.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=nh-open-source@amazon.com \
--cc=robin.murphy@arm.com \
--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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®