On Thu, Feb 09, 2023 at 04:00:55PM +0000, Marc Zyngier wrote:
> 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
> > @@ -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);
> > @@ -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?
I went back and forth over that a bit, but decided to only use
domain->root->mutex in paths that can be called for hierarchical
domains (i.e. the "shared code paths" mentioned above).
Using it in paths that are clearly only called for non-hierarchical
domains where domain->root == domain felt a bit lazy.
The counter argument is of course that using domain->root->lock allows
people to think less about the code they are changing, but that's not
necessarily always a good thing.
Also note that the lockdep asserts in the revmap helpers would catch
anyone using domain->mutex where they should not (i.e. using
domain->mutex for an hierarchical domain).
> > 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.
Indeed we do, even if device links should make this harder to hit these
days.
> Splitting the work would help, as per the following patch.
Looks good to me. Do you want to submit that as a patch that I'll rebase
on or should I submit it as part of a v6?
Johan
