| Message ID | 1370644273-10495-3-git-send-email-yinghai@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Fri, Jun 07, 2013 at 03:30:48PM -0700, Yinghai Lu wrote: > Current code: after irq remapping is enabled, irq_chip fields are modified > during every irq setup. > mp_register_gsi > io_apic_set_pci_routing > io_apic_setup_irq_pin > setup_ioapic_irq > ioapic_register_intr > setup_remapped_irq > native_setup_msi_irqs > setup_msi_irq > setup_remapped_irq > default_setup_hpet_msi > setup_remapped_irq > that is not efficient. > > We only need to modify those irq chip one time just after we enable > irq mapping. The overhead you talk about is calling setup_remapped_irq() for every interrupt from the mp_register_gsi() call chain? MSI & HPET should happen only once, or do I miss something? > Change irq_remap_modify_chip_defaults() to __init as it only gets > called during booting stage, via irq_remap_modify_chips(). > > Affected irq_chip: ioapic_chip, msi_chip, hpet_msi_type. > We don't need to use #ifdef in irq_remap_modify_chips(): > IRQ_REMAP only support x86_64 and X86_IO_APIC and PCI_MSI. > HPET_TIMER is set when x86_64 is set. > When we have IRQ_REMAP enabled, al three chips are defined and > used. all, not al Still, the user could disable hpet or apic from the commandline but this should cause any harm as the irq chips shouldn't be used then. > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c > index 904611b..ff50b90 100644 > --- a/arch/x86/kernel/apic/apic.c > +++ b/arch/x86/kernel/apic/apic.c > @@ -1552,6 +1552,8 @@ void enable_x2apic(void) > int __init enable_IR(void) > { > #ifdef CONFIG_IRQ_REMAP > + int ret; > + > if (!irq_remapping_supported()) { > pr_debug("intr-remapping not supported\n"); > return -1; > @@ -1563,7 +1565,12 @@ int __init enable_IR(void) > return -1; > } > > - return irq_remapping_enable(); > + ret = irq_remapping_enable(); > + > + if (ret >= 0) > + irq_remap_modify_chips(); This looks like ehm, well not well. Could you please change this to: ret = irq_remapping_enable(); if (ret) return ret; irq_remap_modify_chips(); > + > + return ret; > #endif > return -1; > } > diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c > index 07ce86a..21ef344 100644 > --- a/drivers/iommu/irq_remapping.c > +++ b/drivers/iommu/irq_remapping.c > @@ -373,19 +373,26 @@ static void ir_print_prefix(struct irq_data *data, struct seq_file *p) > seq_printf(p, " IR-%s", data->chip->name); > } > > -static void irq_remap_modify_chip_defaults(struct irq_chip *chip) > +static void __init irq_remap_modify_chip_defaults(struct irq_chip *chip) > { > + printk(KERN_DEBUG "irq_chip: %s ==> IR-%s", chip->name, chip->name); If you need this please use pr_debug and add a \n at the end. > chip->irq_print_chip = ir_print_prefix; > chip->irq_ack = ir_ack_apic_edge; > chip->irq_eoi = ir_ack_apic_level; > chip->irq_set_affinity = x86_io_apic_ops.set_affinity; > } > > +void __init irq_remap_modify_chips(void) > +{ > + irq_remap_modify_chip_defaults(&ioapic_chip); > + irq_remap_modify_chip_defaults(&msi_chip); > + irq_remap_modify_chip_defaults(&hpet_msi_type); > +} > + > bool setup_remapped_irq(int irq, struct irq_cfg *cfg, struct irq_chip *chip) > { > if (!irq_remapped(cfg)) > return false; > irq_set_status_flags(irq, IRQ_MOVE_PCNTXT); > - irq_remap_modify_chip_defaults(chip); chip is not required, and can be removed. > return true; > } Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jun 9, 2013 at 7:54 AM, Sebastian Andrzej Siewior <sebastian@breakpoint.cc> wrote: > On Fri, Jun 07, 2013 at 03:30:48PM -0700, Yinghai Lu wrote: >> Current code: after irq remapping is enabled, irq_chip fields are modified >> during every irq setup. >> mp_register_gsi >> io_apic_set_pci_routing >> io_apic_setup_irq_pin >> setup_ioapic_irq >> ioapic_register_intr >> setup_remapped_irq >> native_setup_msi_irqs >> setup_msi_irq >> setup_remapped_irq >> default_setup_hpet_msi >> setup_remapped_irq >> that is not efficient. >> >> We only need to modify those irq chip one time just after we enable >> irq mapping. > > The overhead you talk about is calling setup_remapped_irq() for every > interrupt from the mp_register_gsi() call chain? MSI & HPET should happen only > once, or do I miss something? yes, and for every pci_enable_msi per pci device. > >> Change irq_remap_modify_chip_defaults() to __init as it only gets >> called during booting stage, via irq_remap_modify_chips(). >> >> Affected irq_chip: ioapic_chip, msi_chip, hpet_msi_type. >> We don't need to use #ifdef in irq_remap_modify_chips(): >> IRQ_REMAP only support x86_64 and X86_IO_APIC and PCI_MSI. >> HPET_TIMER is set when x86_64 is set. >> When we have IRQ_REMAP enabled, al three chips are defined and >> used. > > all, not al > > Still, the user could disable hpet or apic from the commandline but this > should cause any harm as the irq chips shouldn't be used then. should "not" ? > >> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c >> index 904611b..ff50b90 100644 >> --- a/arch/x86/kernel/apic/apic.c >> +++ b/arch/x86/kernel/apic/apic.c >> @@ -1552,6 +1552,8 @@ void enable_x2apic(void) >> int __init enable_IR(void) >> { >> #ifdef CONFIG_IRQ_REMAP >> + int ret; >> + >> if (!irq_remapping_supported()) { >> pr_debug("intr-remapping not supported\n"); >> return -1; >> @@ -1563,7 +1565,12 @@ int __init enable_IR(void) >> return -1; >> } >> >> - return irq_remapping_enable(); >> + ret = irq_remapping_enable(); >> + >> + if (ret >= 0) >> + irq_remap_modify_chips(); > > This looks like ehm, well not well. > Could you please change this to: > ret = irq_remapping_enable(); > if (ret) > return ret; > > irq_remap_modify_chips(); > >> + >> + return ret; So you want to use ret = irq_remapping_enable(); if (ret < 0) return ret; irq_remap_modify_chips(); return ret; instead of: ret = irq_remapping_enable(); if (ret >= 0) irq_remap_modify_chips(); return ret; >> #endif >> return -1; >> } >> diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c >> index 07ce86a..21ef344 100644 >> --- a/drivers/iommu/irq_remapping.c >> +++ b/drivers/iommu/irq_remapping.c >> @@ -373,19 +373,26 @@ static void ir_print_prefix(struct irq_data *data, struct seq_file *p) >> seq_printf(p, " IR-%s", data->chip->name); >> } >> >> -static void irq_remap_modify_chip_defaults(struct irq_chip *chip) >> +static void __init irq_remap_modify_chip_defaults(struct irq_chip *chip) >> { >> + printk(KERN_DEBUG "irq_chip: %s ==> IR-%s", chip->name, chip->name); > > If you need this please use pr_debug and add a \n at the end. No, I hate pr_debug, as it is useless unless have DEBUG defined. later even ask user to append "debug ignore_loglevel", we still get nothing unless user recompile the kernel. will add "\n", not sure how that get dropped. > >> chip->irq_print_chip = ir_print_prefix; >> chip->irq_ack = ir_ack_apic_edge; >> chip->irq_eoi = ir_ack_apic_level; >> chip->irq_set_affinity = x86_io_apic_ops.set_affinity; >> } >> >> +void __init irq_remap_modify_chips(void) >> +{ >> + irq_remap_modify_chip_defaults(&ioapic_chip); >> + irq_remap_modify_chip_defaults(&msi_chip); >> + irq_remap_modify_chip_defaults(&hpet_msi_type); >> +} >> + >> bool setup_remapped_irq(int irq, struct irq_cfg *cfg, struct irq_chip *chip) >> { >> if (!irq_remapped(cfg)) >> return false; >> irq_set_status_flags(irq, IRQ_MOVE_PCNTXT); >> - irq_remap_modify_chip_defaults(chip); > > chip is not required, and can be removed. yes. Thanks Yinghai -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h index 459e50a..eaff3ad 100644 --- a/arch/x86/include/asm/io_apic.h +++ b/arch/x86/include/asm/io_apic.h @@ -215,6 +215,11 @@ static inline void io_apic_modify(unsigned int apic, unsigned int reg, unsigned extern void io_apic_eoi(unsigned int apic, unsigned int vector); +extern struct irq_chip ioapic_chip; +extern struct irq_chip msi_chip; +extern struct irq_chip hpet_msi_type; +void irq_remap_modify_chips(void); + #else /* !CONFIG_X86_IO_APIC */ #define io_apic_assign_pci_irqs 0 diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c index 904611b..ff50b90 100644 --- a/arch/x86/kernel/apic/apic.c +++ b/arch/x86/kernel/apic/apic.c @@ -1552,6 +1552,8 @@ void enable_x2apic(void) int __init enable_IR(void) { #ifdef CONFIG_IRQ_REMAP + int ret; + if (!irq_remapping_supported()) { pr_debug("intr-remapping not supported\n"); return -1; @@ -1563,7 +1565,12 @@ int __init enable_IR(void) return -1; } - return irq_remapping_enable(); + ret = irq_remapping_enable(); + + if (ret >= 0) + irq_remap_modify_chips(); + + return ret; #endif return -1; } diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c index 9ed796c..f78f7f6 100644 --- a/arch/x86/kernel/apic/io_apic.c +++ b/arch/x86/kernel/apic/io_apic.c @@ -1238,8 +1238,6 @@ void __setup_vector_irq(int cpu) raw_spin_unlock(&vector_lock); } -static struct irq_chip ioapic_chip; - #ifdef CONFIG_X86_32 static inline int IO_APIC_irq_trigger(int irq) { @@ -2511,7 +2509,7 @@ static void ack_apic_level(struct irq_data *data) ioapic_irqd_unmask(data, cfg, masked); } -static struct irq_chip ioapic_chip __read_mostly = { +struct irq_chip ioapic_chip __read_mostly = { .name = "IO-APIC", .irq_startup = startup_ioapic_irq, .irq_mask = mask_ioapic_irq, @@ -3093,7 +3091,7 @@ msi_set_affinity(struct irq_data *data, const struct cpumask *mask, bool force) * IRQ Chip for MSI PCI/PCI-X/PCI-Express Devices, * which implement the MSI or MSI-X Capability Structure. */ -static struct irq_chip msi_chip = { +struct irq_chip msi_chip = { .name = "PCI-MSI", .irq_unmask = unmask_msi_irq, .irq_mask = mask_msi_irq, @@ -3240,7 +3238,7 @@ static int hpet_msi_set_affinity(struct irq_data *data, return IRQ_SET_MASK_OK_NOCOPY; } -static struct irq_chip hpet_msi_type = { +struct irq_chip hpet_msi_type = { .name = "HPET_MSI", .irq_unmask = hpet_msi_unmask, .irq_mask = hpet_msi_mask, diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c index 07ce86a..21ef344 100644 --- a/drivers/iommu/irq_remapping.c +++ b/drivers/iommu/irq_remapping.c @@ -373,19 +373,26 @@ static void ir_print_prefix(struct irq_data *data, struct seq_file *p) seq_printf(p, " IR-%s", data->chip->name); } -static void irq_remap_modify_chip_defaults(struct irq_chip *chip) +static void __init irq_remap_modify_chip_defaults(struct irq_chip *chip) { + printk(KERN_DEBUG "irq_chip: %s ==> IR-%s", chip->name, chip->name); chip->irq_print_chip = ir_print_prefix; chip->irq_ack = ir_ack_apic_edge; chip->irq_eoi = ir_ack_apic_level; chip->irq_set_affinity = x86_io_apic_ops.set_affinity; } +void __init irq_remap_modify_chips(void) +{ + irq_remap_modify_chip_defaults(&ioapic_chip); + irq_remap_modify_chip_defaults(&msi_chip); + irq_remap_modify_chip_defaults(&hpet_msi_type); +} + bool setup_remapped_irq(int irq, struct irq_cfg *cfg, struct irq_chip *chip) { if (!irq_remapped(cfg)) return false; irq_set_status_flags(irq, IRQ_MOVE_PCNTXT); - irq_remap_modify_chip_defaults(chip); return true; }
Current code: after irq remapping is enabled, irq_chip fields are modified during every irq setup. mp_register_gsi io_apic_set_pci_routing io_apic_setup_irq_pin setup_ioapic_irq ioapic_register_intr setup_remapped_irq native_setup_msi_irqs setup_msi_irq setup_remapped_irq default_setup_hpet_msi setup_remapped_irq that is not efficient. We only need to modify those irq chip one time just after we enable irq mapping. Change irq_remap_modify_chip_defaults() to __init as it only gets called during booting stage, via irq_remap_modify_chips(). Affected irq_chip: ioapic_chip, msi_chip, hpet_msi_type. We don't need to use #ifdef in irq_remap_modify_chips(): IRQ_REMAP only support x86_64 and X86_IO_APIC and PCI_MSI. HPET_TIMER is set when x86_64 is set. When we have IRQ_REMAP enabled, al three chips are defined and used. Signed-off-by: Yinghai Lu <yinghai@kernel.org> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Sebastian Andrzej Siewior <sebastian@breakpoint.cc> Cc: Joerg Roedel <joro@8bytes.org> --- arch/x86/include/asm/io_apic.h | 5 +++++ arch/x86/kernel/apic/apic.c | 9 ++++++++- arch/x86/kernel/apic/io_apic.c | 8 +++----- drivers/iommu/irq_remapping.c | 11 +++++++++-- 4 files changed, 25 insertions(+), 8 deletions(-)