Patchwork [v3,02/27] x86, irq: Modify irq chip once for irq remapping

login
register
mail settings
Submitter Yinghai Lu
Date June 7, 2013, 10:30 p.m.
Message ID <1370644273-10495-3-git-send-email-yinghai@kernel.org>
Download mbox | patch
Permalink /patch/249876/
State Not Applicable
Headers show

Comments

Yinghai Lu - June 7, 2013, 10:30 p.m.
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(-)
Sebastian Siewior - June 9, 2013, 2:54 p.m.
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
Yinghai Lu - June 10, 2013, 11:17 p.m.
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

Patch

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;
 }