Date:   Thu, 09 Feb 2023 16:00:55 +0000
Message-ID: <86cz6izv48.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Johan Hovold <johan+linaro@kernel.org>
Cc:     Thomas Gleixner <tglx@linutronix.de>, x86@kernel.org,
        platform-driver-x86@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-mips@vger.kernel.org,
        linux-kernel@vger.kernel.org, Hsin-Yi Wang <hsinyi@chromium.org>,
        Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Subject: Re: [PATCH v5 19/19] irqdomain: Switch to per-domain locking
In-Reply-To: <20230209132323.4599-20-johan+linaro@kernel.org>
References: <20230209132323.4599-1-johan+linaro@kernel.org>
        <20230209132323.4599-20-johan+linaro@kernel.org>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Thu, 09 Feb 2023 13:23:23 +0000,
Johan Hovold <johan+linaro@kernel.org> 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@chromium.org>
> Tested-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  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.