On Sun, Jun 9, 2013 at 7:54 AM, Sebastian Andrzej Siewior
<sebastian@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
