| Message ID | 20210406093557.1073423-2-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleaning up some of the irqdomain features | expand |
Le 06/04/2021 à 11:35, Marc Zyngier a écrit : > irq_linear_revmap() is supposed to be a fast path for domain > lookups, but it only exposes low-level details of the irqdomain > implementation, details which are better kept private. Can you elaborate with more details ? > > The *overhead* between the two is only a function call and > a couple of tests, so it is likely that noone can show any > meaningful difference compared to the cost of taking an > interrupt. Do you have any measurement ? Can you make the "likely" a certitude ? > > Reimplement irq_linear_revmap() with irq_find_mapping() > in order to preserve source code compatibility, and > rename the internal field for a measure. This is in complete contradiction with commit https://github.com/torvalds/linux/commit/d3dcb436 At that time, irq_linear_revmap() was less complex than what irq_find_mapping() is today, and nevertheless it was considered worth restoring in as a fast path. What has changed since then ? Can you also explain the reason for the renaming of "linear_revmap" into "revmap" ? What is that "measure" ? > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > include/linux/irqdomain.h | 22 +++++++++------------- > kernel/irq/irqdomain.c | 6 +++--- > 2 files changed, 12 insertions(+), 16 deletions(-) > > diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h > index 33cacc8af26d..b9600f24878a 100644 > --- a/include/linux/irqdomain.h > +++ b/include/linux/irqdomain.h > @@ -154,9 +154,9 @@ struct irq_domain_chip_generic; > * Revmap data, used internally by irq_domain > * @revmap_direct_max_irq: The largest hwirq that can be set for controllers that > * support direct mapping > - * @revmap_size: Size of the linear map table @linear_revmap[] > + * @revmap_size: Size of the linear map table @revmap[] > * @revmap_tree: Radix map tree for hwirqs that don't fit in the linear map > - * @linear_revmap: Linear table of hwirq->virq reverse mappings > + * @revmap: Linear table of hwirq->virq reverse mappings > */ > struct irq_domain { > struct list_head link; > @@ -180,7 +180,7 @@ struct irq_domain { > unsigned int revmap_size; > struct radix_tree_root revmap_tree; > struct mutex revmap_tree_mutex; > - unsigned int linear_revmap[]; > + unsigned int revmap[]; > }; > > /* Irq domain flags */ > @@ -396,24 +396,20 @@ static inline unsigned int irq_create_mapping(struct irq_domain *host, > return irq_create_mapping_affinity(host, hwirq, NULL); > } > > - > /** > - * irq_linear_revmap() - Find a linux irq from a hw irq number. > + * irq_find_mapping() - Find a linux irq from a hw irq number. > * @domain: domain owning this hardware interrupt > * @hwirq: hardware irq number in that domain space > - * > - * This is a fast path alternative to irq_find_mapping() that can be > - * called directly by irq controller code to save a handful of > - * instructions. It is always safe to call, but won't find irqs mapped > - * using the radix tree. > */ > +extern unsigned int irq_find_mapping(struct irq_domain *host, > + irq_hw_number_t hwirq); > + > static inline unsigned int irq_linear_revmap(struct irq_domain *domain, > irq_hw_number_t hwirq) > { > - return hwirq < domain->revmap_size ? domain->linear_revmap[hwirq] : 0; > + return irq_find_mapping(domain, hwirq); > } > -extern unsigned int irq_find_mapping(struct irq_domain *host, > - irq_hw_number_t hwirq); > + > extern unsigned int irq_create_direct_mapping(struct irq_domain *host); > extern int irq_create_strict_mappings(struct irq_domain *domain, > unsigned int irq_base, > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c > index d10ab1d689d5..dfa716305ea9 100644 > --- a/kernel/irq/irqdomain.c > +++ b/kernel/irq/irqdomain.c > @@ -486,7 +486,7 @@ static void irq_domain_clear_mapping(struct irq_domain *domain, > irq_hw_number_t hwirq) > { > if (hwirq < domain->revmap_size) { > - domain->linear_revmap[hwirq] = 0; > + domain->revmap[hwirq] = 0; > } else { > mutex_lock(&domain->revmap_tree_mutex); > radix_tree_delete(&domain->revmap_tree, hwirq); > @@ -499,7 +499,7 @@ static void irq_domain_set_mapping(struct irq_domain *domain, > struct irq_data *irq_data) > { > if (hwirq < domain->revmap_size) { > - domain->linear_revmap[hwirq] = irq_data->irq; > + domain->revmap[hwirq] = irq_data->irq; > } else { > mutex_lock(&domain->revmap_tree_mutex); > radix_tree_insert(&domain->revmap_tree, hwirq, irq_data); > @@ -920,7 +920,7 @@ unsigned int irq_find_mapping(struct irq_domain *domain, > > /* Check if the hwirq is in the linear revmap. */ > if (hwirq < domain->revmap_size) > - return domain->linear_revmap[hwirq]; > + return domain->revmap[hwirq]; > > rcu_read_lock(); > data = radix_tree_lookup(&domain->revmap_tree, hwirq); >
Christophe, On Tue, 06 Apr 2021 12:21:33 +0100, Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > > > Le 06/04/2021 à 11:35, Marc Zyngier a écrit : > > irq_linear_revmap() is supposed to be a fast path for domain > > lookups, but it only exposes low-level details of the irqdomain > > implementation, details which are better kept private. > > Can you elaborate with more details ? Things like directly picking into the revmap are positively awful, and doesn't work if the domain has been constructed using the radix tree. Which on its own is totally broken, because things like irq_domain_create_hierarchy() will pick an implementation or the other. > > > > > The *overhead* between the two is only a function call and > > a couple of tests, so it is likely that noone can show any > > meaningful difference compared to the cost of taking an > > interrupt. > > Do you have any measurement ? I did measure things on arm64, and couldn't come up with any difference other than noise. > Can you make the "likely" a certitude ? Of course not. You can always come up with an artificial CPU implementation that has a very small exception entry overhead, and a ridiculously slow memory subsystem. Do I care about these? No. If you can come up with realistic platforms that show a regression with this patch, I'm all ears. > > > > > Reimplement irq_linear_revmap() with irq_find_mapping() > > in order to preserve source code compatibility, and > > rename the internal field for a measure. > > This is in complete contradiction with commit https://github.com/torvalds/linux/commit/d3dcb436 > > At that time, irq_linear_revmap() was less complex than what > irq_find_mapping() is today, and nevertheless it was considered worth > restoring in as a fast path. What has changed since then ? Over 8 years? Plenty. The use of irqdomains has been generalised, we have domain hierarchies, and if anything, this commit introduces the buggy behaviour I was mentioning above. I also don't see any mention of actual performance in that commit. And if we're worried about a fast path, being able to directly cache the irq_data in the revmap, hence skipping the irq_desc lookup that inevitable follows, is a much more interesting prospect than the "get useless data quick" that irq_linear_revmap() implements. This latter optimisation is what I am after. > Can you also explain the reason for the renaming of "linear_revmap" > into "revmap" ? What is that "measure" ? To catch the potential direct use of the reverse map field. Thanks, M.
diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h index 33cacc8af26d..b9600f24878a 100644 --- a/include/linux/irqdomain.h +++ b/include/linux/irqdomain.h @@ -154,9 +154,9 @@ struct irq_domain_chip_generic; * Revmap data, used internally by irq_domain * @revmap_direct_max_irq: The largest hwirq that can be set for controllers that * support direct mapping - * @revmap_size: Size of the linear map table @linear_revmap[] + * @revmap_size: Size of the linear map table @revmap[] * @revmap_tree: Radix map tree for hwirqs that don't fit in the linear map - * @linear_revmap: Linear table of hwirq->virq reverse mappings + * @revmap: Linear table of hwirq->virq reverse mappings */ struct irq_domain { struct list_head link; @@ -180,7 +180,7 @@ struct irq_domain { unsigned int revmap_size; struct radix_tree_root revmap_tree; struct mutex revmap_tree_mutex; - unsigned int linear_revmap[]; + unsigned int revmap[]; }; /* Irq domain flags */ @@ -396,24 +396,20 @@ static inline unsigned int irq_create_mapping(struct irq_domain *host, return irq_create_mapping_affinity(host, hwirq, NULL); } - /** - * irq_linear_revmap() - Find a linux irq from a hw irq number. + * irq_find_mapping() - Find a linux irq from a hw irq number. * @domain: domain owning this hardware interrupt * @hwirq: hardware irq number in that domain space - * - * This is a fast path alternative to irq_find_mapping() that can be - * called directly by irq controller code to save a handful of - * instructions. It is always safe to call, but won't find irqs mapped - * using the radix tree. */ +extern unsigned int irq_find_mapping(struct irq_domain *host, + irq_hw_number_t hwirq); + static inline unsigned int irq_linear_revmap(struct irq_domain *domain, irq_hw_number_t hwirq) { - return hwirq < domain->revmap_size ? domain->linear_revmap[hwirq] : 0; + return irq_find_mapping(domain, hwirq); } -extern unsigned int irq_find_mapping(struct irq_domain *host, - irq_hw_number_t hwirq); + extern unsigned int irq_create_direct_mapping(struct irq_domain *host); extern int irq_create_strict_mappings(struct irq_domain *domain, unsigned int irq_base, diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index d10ab1d689d5..dfa716305ea9 100644 --- a/kernel/irq/irqdomain.c +++ b/kernel/irq/irqdomain.c @@ -486,7 +486,7 @@ static void irq_domain_clear_mapping(struct irq_domain *domain, irq_hw_number_t hwirq) { if (hwirq < domain->revmap_size) { - domain->linear_revmap[hwirq] = 0; + domain->revmap[hwirq] = 0; } else { mutex_lock(&domain->revmap_tree_mutex); radix_tree_delete(&domain->revmap_tree, hwirq); @@ -499,7 +499,7 @@ static void irq_domain_set_mapping(struct irq_domain *domain, struct irq_data *irq_data) { if (hwirq < domain->revmap_size) { - domain->linear_revmap[hwirq] = irq_data->irq; + domain->revmap[hwirq] = irq_data->irq; } else { mutex_lock(&domain->revmap_tree_mutex); radix_tree_insert(&domain->revmap_tree, hwirq, irq_data); @@ -920,7 +920,7 @@ unsigned int irq_find_mapping(struct irq_domain *domain, /* Check if the hwirq is in the linear revmap. */ if (hwirq < domain->revmap_size) - return domain->linear_revmap[hwirq]; + return domain->revmap[hwirq]; rcu_read_lock(); data = radix_tree_lookup(&domain->revmap_tree, hwirq);
irq_linear_revmap() is supposed to be a fast path for domain lookups, but it only exposes low-level details of the irqdomain implementation, details which are better kept private. The *overhead* between the two is only a function call and a couple of tests, so it is likely that noone can show any meaningful difference compared to the cost of taking an interrupt. Reimplement irq_linear_revmap() with irq_find_mapping() in order to preserve source code compatibility, and rename the internal field for a measure. Signed-off-by: Marc Zyngier <maz@kernel.org> --- include/linux/irqdomain.h | 22 +++++++++------------- kernel/irq/irqdomain.c | 6 +++--- 2 files changed, 12 insertions(+), 16 deletions(-)