On Thu, 09 Feb 2023 13:23:23 +0000,
Johan Hovold <johan+linaro@xxxxxxxxxx> wrote:
>
> The IRQ domain structures are currently protected by the global
> irq_domain_mutex. Switch to using more fine-grained per-domain locking,
> which can speed up parallel probing by reducing lock contention.
>
> On a recent arm64 laptop, the total time spent waiting for the locks
> during boot drops from 160 to 40 ms on average, while the maximum
> aggregate wait time drops from 550 to 90 ms over ten runs for example.
>
> Note that the domain lock of the root domain (innermost domain) must be
> used for hierarchical domains. For non-hierarchical domains (as for root
> domains), the new root pointer is set to the domain itself so that
> domain->root->mutex can be used in shared code paths.
>
> Also note that hierarchical domains should be constructed using
> irq_domain_create_hierarchy() (or irq_domain_add_hierarchy()) to avoid
> poking at irqdomain internals. As a safeguard, the lockdep assertion in
> irq_domain_set_mapping() will catch any offenders that fail to set the
> root domain pointer.
>
> Tested-by: Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx>
> Tested-by: Mark-PK Tsai <mark-pk.tsai@xxxxxxxxxxxx>
> Signed-off-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
> ---
> include/linux/irqdomain.h | 4 +++
> kernel/irq/irqdomain.c | 61 +++++++++++++++++++++++++--------------
> 2 files changed, 44 insertions(+), 21 deletions(-)
>
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index 16399de00b48..cad47737a052 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -125,6 +125,8 @@ struct irq_domain_chip_generic;
> * core code.
> * @flags: Per irq_domain flags
> * @mapcount: The number of mapped interrupts
> + * @mutex: Domain lock, hierarhical domains use root domain's lock
nit: hierarchical
> + * @root: Pointer to root domain, or containing structure if non-hierarchical
> *
> * Optional elements:
> * @fwnode: Pointer to firmware node associated with the irq_domain. Pretty easy
> @@ -152,6 +154,8 @@ struct irq_domain {
> void *host_data;
> unsigned int flags;
> unsigned int mapcount;
> + struct mutex mutex;
> + struct irq_domain *root;
>
> /* Optional data */
> struct fwnode_handle *fwnode;
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 804d316329c8..c96aa5e5a94b 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -226,6 +226,17 @@ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, unsigned int s
>
> domain->revmap_size = size;
>
> + /*
> + * Hierarchical domains use the domain lock of the root domain
> + * (innermost domain).
> + *
> + * For non-hierarchical domains (as for root domains), the root
> + * pointer is set to the domain itself so that domain->root->mutex
> + * can be used in shared code paths.
> + */
> + mutex_init(&domain->mutex);
> + domain->root = domain;
> +
> irq_domain_check_hierarchy(domain);
>
> mutex_lock(&irq_domain_mutex);
> @@ -503,7 +514,7 @@ static bool irq_domain_is_nomap(struct irq_domain *domain)
> static void irq_domain_clear_mapping(struct irq_domain *domain,
> irq_hw_number_t hwirq)
> {
> - lockdep_assert_held(&irq_domain_mutex);
> + lockdep_assert_held(&domain->root->mutex);
>
> if (irq_domain_is_nomap(domain))
> return;
> @@ -518,7 +529,11 @@ static void irq_domain_set_mapping(struct irq_domain *domain,
> irq_hw_number_t hwirq,
> struct irq_data *irq_data)
> {
> - lockdep_assert_held(&irq_domain_mutex);
> + /*
> + * This also makes sure that all domains point to the same root when
> + * called from irq_domain_insert_irq() for each domain in a hierarchy.
> + */
> + lockdep_assert_held(&domain->root->mutex);
>
> if (irq_domain_is_nomap(domain))
> return;
> @@ -540,7 +555,7 @@ static void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq)
>
> hwirq = irq_data->hwirq;
>
> - mutex_lock(&irq_domain_mutex);
> + mutex_lock(&domain->mutex);
So you made that point about being able to uniformly using root>mutex,
which I think is a good invariant. Yet you hardly make use of it. Why?
>
> irq_set_status_flags(irq, IRQ_NOREQUEST);
>
> @@ -562,7 +577,7 @@ static void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq)
> /* Clear reverse map for this hwirq */
> irq_domain_clear_mapping(domain, hwirq);
>
> - mutex_unlock(&irq_domain_mutex);
> + mutex_unlock(&domain->mutex);
> }
>
> static int irq_domain_associate_locked(struct irq_domain *domain, unsigned int virq,
> @@ -612,9 +627,9 @@ int irq_domain_associate(struct irq_domain *domain, unsigned int virq,
> {
> int ret;
>
> - mutex_lock(&irq_domain_mutex);
> + mutex_lock(&domain->mutex);
> ret = irq_domain_associate_locked(domain, virq, hwirq);
> - mutex_unlock(&irq_domain_mutex);
> + mutex_unlock(&domain->mutex);
>
> return ret;
> }
> @@ -731,7 +746,7 @@ unsigned int irq_create_mapping_affinity(struct irq_domain *domain,
> return 0;
> }
>
> - mutex_lock(&irq_domain_mutex);
> + mutex_lock(&domain->mutex);
>
> /* Check if mapping already exists */
> virq = irq_find_mapping(domain, hwirq);
> @@ -742,7 +757,7 @@ unsigned int irq_create_mapping_affinity(struct irq_domain *domain,
>
> virq = irq_create_mapping_affinity_locked(domain, hwirq, affinity);
> out:
> - mutex_unlock(&irq_domain_mutex);
> + mutex_unlock(&domain->mutex);
>
> return virq;
> }
> @@ -811,7 +826,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> if (WARN_ON(type & ~IRQ_TYPE_SENSE_MASK))
> type &= IRQ_TYPE_SENSE_MASK;
>
> - mutex_lock(&irq_domain_mutex);
> + mutex_lock(&domain->root->mutex);
>
> /*
> * If we've already configured this interrupt,
> @@ -864,11 +879,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> /* Store trigger type */
> irqd_set_trigger_type(irq_data, type);
> out:
> - mutex_unlock(&irq_domain_mutex);
> + mutex_unlock(&domain->root->mutex);
>
> return virq;
> err:
> - mutex_unlock(&irq_domain_mutex);
> + mutex_unlock(&domain->root->mutex);
>
> return 0;
> }
> @@ -1132,6 +1147,7 @@ struct irq_domain *irq_domain_create_hierarchy(struct irq_domain *parent,
> else
> domain = irq_domain_create_tree(fwnode, ops, host_data);
> if (domain) {
> + domain->root = parent->root;
> domain->parent = parent;
> domain->flags |= flags;
So we still have a bug here, as we have published a domain that we
keep updating. A parallel probing could find it in the interval and do
something completely wrong.
Splitting the work would help, as per the following patch.
Thanks,
M.
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index c96aa5e5a94b..6c675ef672e9 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -126,23 +126,12 @@ void irq_domain_free_fwnode(struct fwnode_handle *fwnode)
}
EXPORT_SYMBOL_GPL(irq_domain_free_fwnode);
-/**
- * __irq_domain_add() - Allocate a new irq_domain data structure
- * @fwnode: firmware node for the interrupt controller
- * @size: Size of linear map; 0 for radix mapping only
- * @hwirq_max: Maximum number of interrupts supported by controller
- * @direct_max: Maximum value of direct maps; Use ~0 for no limit; 0 for no
- * direct mapping
- * @ops: domain callbacks
- * @host_data: Controller private data pointer
- *
- * Allocates and initializes an irq_domain structure.
- * Returns pointer to IRQ domain, or NULL on failure.
- */
-struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, unsigned int size,
- irq_hw_number_t hwirq_max, int direct_max,
- const struct irq_domain_ops *ops,
- void *host_data)
+static struct irq_domain *__irq_domain_create(struct fwnode_handle *fwnode,
+ unsigned int size,
+ irq_hw_number_t hwirq_max,
+ int direct_max,
+ const struct irq_domain_ops *ops,
+ void *host_data)
{
struct irqchip_fwid *fwid;
struct irq_domain *domain;
@@ -239,12 +228,44 @@ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, unsigned int s
irq_domain_check_hierarchy(domain);
+ return domain;
+}
+
+static void __irq_domain_publish(struct irq_domain *domain)
+{
mutex_lock(&irq_domain_mutex);
debugfs_add_domain_dir(domain);
list_add(&domain->link, &irq_domain_list);
mutex_unlock(&irq_domain_mutex);
pr_debug("Added domain %s\n", domain->name);
+}
+
+/**
+ * __irq_domain_add() - Allocate a new irq_domain data structure
+ * @fwnode: firmware node for the interrupt controller
+ * @size: Size of linear map; 0 for radix mapping only
+ * @hwirq_max: Maximum number of interrupts supported by controller
+ * @direct_max: Maximum value of direct maps; Use ~0 for no limit; 0 for no
+ * direct mapping
+ * @ops: domain callbacks
+ * @host_data: Controller private data pointer
+ *
+ * Allocates and initializes an irq_domain structure.
+ * Returns pointer to IRQ domain, or NULL on failure.
+ */
+struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, unsigned int size,
+ irq_hw_number_t hwirq_max, int direct_max,
+ const struct irq_domain_ops *ops,
+ void *host_data)
+{
+ struct irq_domain *domain;
+
+ domain = __irq_domain_create(fwnode, size, hwirq_max, direct_max,
+ ops, host_data);
+ if (domain)
+ __irq_domain_publish(domain);
+
return domain;
}
EXPORT_SYMBOL_GPL(__irq_domain_add);
@@ -1143,13 +1164,16 @@ struct irq_domain *irq_domain_create_hierarchy(struct irq_domain *parent,
struct irq_domain *domain;
if (size)
- domain = irq_domain_create_linear(fwnode, size, ops, host_data);
+ domain = __irq_domain_create(fwnode, size, size, 0, ops, host_data);
else
- domain = irq_domain_create_tree(fwnode, ops, host_data);
+ domain = __irq_domain_create(fwnode, 0, ~0, 0, ops, host_data);
+
if (domain) {
domain->root = parent->root;
domain->parent = parent;
domain->flags |= flags;
+
+ __irq_domain_publish(domain);
}
return domain;
--
Without deviation from the norm, progress is not possible.
