x86: use vector_desc instead of vector_irq
in [Kernel]
|
Prev: [patch v2] pcrypt: handle crypto_get_attr_type() errors
Next: About source IP address of IGMP Join sent on loopback interface
From: Thomas Gleixner on 22 Mar 2010 10:00 On Sun, 21 Mar 2010, Yinghai Lu wrote: > Eric pointed out that radix tree version of irq_to_desc will magnify delay on > the path of handle_irq. > > use vector_desc to reduce the calling of irq_to_desc. > > next step: need to change all ack, mask, umask, eoi for all irq_chip to take irq_desc That's not relevant for this change. > > -typedef int vector_irq_t[NR_VECTORS]; > -DECLARE_PER_CPU(vector_irq_t, vector_irq); > -extern void setup_vector_irq(int cpu); > +typedef struct irq_desc *vector_desc_t[NR_VECTORS]; Why do we need that typedef ? Please use plain struct irq_desc * > +DECLARE_PER_CPU(vector_desc_t, vector_desc); > +extern void setup_vector_desc(int cpu); .... > void destroy_irq(unsigned int irq) > { > unsigned long flags; > + struct irq_desc *desc; > + struct irq_cfg *cfg; > > dynamic_irq_cleanup_keep_chip_data(irq); > > free_irte(irq); > raw_spin_lock_irqsave(&vector_lock, flags); > - __clear_irq_vector(irq, get_irq_chip_data(irq)); > + desc = irq_to_desc(irq); > + cfg = desc->chip_data; > + __clear_irq_vector(desc, cfg); __clear_irq_vector(desc, desc->chip_data); should be sufficient, right ? > raw_spin_unlock_irqrestore(&vector_lock, flags); > } > > @@ -3377,6 +3376,7 @@ void destroy_irq(unsigned int irq) > static int msi_compose_msg(struct pci_dev *pdev, unsigned int irq, > struct msi_msg *msg, u8 hpet_id) > { > + struct irq_desc *desc; > struct irq_cfg *cfg; > int err; > unsigned dest; > @@ -3384,8 +3384,9 @@ static int msi_compose_msg(struct pci_dev *pdev, unsigned int irq, > if (disable_apic) > return -ENXIO; > > - cfg = irq_cfg(irq); > - err = assign_irq_vector(irq, cfg, apic->target_cpus()); > + desc = irq_to_desc(irq); > + cfg = desc->chip_data; > + err = assign_irq_vector(desc, cfg, apic->target_cpus()); Ditto > if (err) > return err; > > @@ -3876,14 +3877,16 @@ static struct irq_chip ht_irq_chip = { > > int arch_setup_ht_irq(unsigned int irq, struct pci_dev *dev) > { > + struct irq_desc *desc; > struct irq_cfg *cfg; > int err; > > if (disable_apic) > return -ENXIO; > > - cfg = irq_cfg(irq); > - err = assign_irq_vector(irq, cfg, apic->target_cpus()); > + desc = irq_to_desc(irq); > + cfg = desc->chip_data; > + err = assign_irq_vector(desc, cfg, apic->target_cpus()); Ditto > if (!err) { > struct ht_irq_msg msg; > unsigned dest; > diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c > index 91fd0c7..f71625c 100644 > --- a/arch/x86/kernel/irq.c > +++ b/arch/x86/kernel/irq.c > @@ -229,19 +229,19 @@ unsigned int __irq_entry do_IRQ(struct pt_regs *regs) > > /* high bit used in ret_from_ code */ > unsigned vector = ~regs->orig_ax; > - unsigned irq; > + struct irq_desc *desc; > > exit_idle(); > irq_enter(); > > - irq = __get_cpu_var(vector_irq)[vector]; > + desc = __get_cpu_var(vector_desc)[vector]; > > - if (!handle_irq(irq, regs)) { > + if (!handle_irq(desc, regs)) { > ack_APIC_irq(); > > if (printk_ratelimit()) > - pr_emerg("%s: %d.%d No irq handler for vector (irq %d)\n", > - __func__, smp_processor_id(), vector, irq); > + pr_emerg("%s: %d.%d No irq handler for vector\n", That printk is confusing. It's not lacking an irq handler. The vector is simply not assigned. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Eric W. Biederman on 22 Mar 2010 10:10 Thomas Gleixner <tglx(a)linutronix.de> writes: > On Sun, 21 Mar 2010, Yinghai Lu wrote: > >> Eric pointed out that radix tree version of irq_to_desc will magnify delay on >> the path of handle_irq. >> >> use vector_desc to reduce the calling of irq_to_desc. >> >> next step: need to change all ack, mask, umask, eoi for all irq_chip to take irq_desc > > That's not relevant for this change. > >> >> -typedef int vector_irq_t[NR_VECTORS]; >> -DECLARE_PER_CPU(vector_irq_t, vector_irq); >> -extern void setup_vector_irq(int cpu); >> +typedef struct irq_desc *vector_desc_t[NR_VECTORS]; > > Why do we need that typedef ? Please use plain struct irq_desc * Well at least originally DECLARE_PER_CPU chocked when given a complex type. Does: DECLARE_PER_CPU(struct irq_desc *[NR_VECTORS], vector_desc); work? >> +DECLARE_PER_CPU(vector_desc_t, vector_desc); >> +extern void setup_vector_desc(int cpu); > ... >> void destroy_irq(unsigned int irq) >> { >> unsigned long flags; >> + struct irq_desc *desc; >> + struct irq_cfg *cfg; >> >> dynamic_irq_cleanup_keep_chip_data(irq); >> >> free_irte(irq); >> raw_spin_lock_irqsave(&vector_lock, flags); >> - __clear_irq_vector(irq, get_irq_chip_data(irq)); >> + desc = irq_to_desc(irq); >> + cfg = desc->chip_data; >> + __clear_irq_vector(desc, cfg); > > __clear_irq_vector(desc, desc->chip_data); > > should be sufficient, right ? You want to deliberately loose a modicum of type safety? > >> raw_spin_unlock_irqrestore(&vector_lock, flags); >> } >> >> @@ -3377,6 +3376,7 @@ void destroy_irq(unsigned int irq) >> static int msi_compose_msg(struct pci_dev *pdev, unsigned int irq, >> struct msi_msg *msg, u8 hpet_id) >> { >> + struct irq_desc *desc; >> struct irq_cfg *cfg; >> int err; >> unsigned dest; >> @@ -3384,8 +3384,9 @@ static int msi_compose_msg(struct pci_dev *pdev, unsigned int irq, >> if (disable_apic) >> return -ENXIO; >> >> - cfg = irq_cfg(irq); >> - err = assign_irq_vector(irq, cfg, apic->target_cpus()); >> + desc = irq_to_desc(irq); >> + cfg = desc->chip_data; >> + err = assign_irq_vector(desc, cfg, apic->target_cpus()); > > Ditto > >> if (err) >> return err; >> >> @@ -3876,14 +3877,16 @@ static struct irq_chip ht_irq_chip = { >> >> int arch_setup_ht_irq(unsigned int irq, struct pci_dev *dev) >> { >> + struct irq_desc *desc; >> struct irq_cfg *cfg; >> int err; >> >> if (disable_apic) >> return -ENXIO; >> >> - cfg = irq_cfg(irq); >> - err = assign_irq_vector(irq, cfg, apic->target_cpus()); >> + desc = irq_to_desc(irq); >> + cfg = desc->chip_data; >> + err = assign_irq_vector(desc, cfg, apic->target_cpus()); > > Ditto > >> if (!err) { >> struct ht_irq_msg msg; >> unsigned dest; >> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c >> index 91fd0c7..f71625c 100644 >> --- a/arch/x86/kernel/irq.c >> +++ b/arch/x86/kernel/irq.c >> @@ -229,19 +229,19 @@ unsigned int __irq_entry do_IRQ(struct pt_regs *regs) >> >> /* high bit used in ret_from_ code */ >> unsigned vector = ~regs->orig_ax; >> - unsigned irq; >> + struct irq_desc *desc; >> >> exit_idle(); >> irq_enter(); >> >> - irq = __get_cpu_var(vector_irq)[vector]; >> + desc = __get_cpu_var(vector_desc)[vector]; >> >> - if (!handle_irq(irq, regs)) { >> + if (!handle_irq(desc, regs)) { >> ack_APIC_irq(); >> >> if (printk_ratelimit()) >> - pr_emerg("%s: %d.%d No irq handler for vector (irq %d)\n", >> - __func__, smp_processor_id(), vector, irq); >> + pr_emerg("%s: %d.%d No irq handler for vector\n", > > That printk is confusing. It's not lacking an irq handler. The > vector is simply not assigned. Long evolution. Do you have a suggestion of better wording? Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Thomas Gleixner on 22 Mar 2010 10:20
> >> -typedef int vector_irq_t[NR_VECTORS]; > >> -DECLARE_PER_CPU(vector_irq_t, vector_irq); > >> -extern void setup_vector_irq(int cpu); > >> +typedef struct irq_desc *vector_desc_t[NR_VECTORS]; > > > > Why do we need that typedef ? Please use plain struct irq_desc * > > Well at least originally DECLARE_PER_CPU chocked when given a complex > type. Does: > DECLARE_PER_CPU(struct irq_desc *[NR_VECTORS], vector_desc); > work? Hmm, I thought that was fixed, but I might be wrong as usual. > > >> +DECLARE_PER_CPU(vector_desc_t, vector_desc); > >> +extern void setup_vector_desc(int cpu); > > ... > >> void destroy_irq(unsigned int irq) > >> { > >> unsigned long flags; > >> + struct irq_desc *desc; > >> + struct irq_cfg *cfg; > >> > >> dynamic_irq_cleanup_keep_chip_data(irq); > >> > >> free_irte(irq); > >> raw_spin_lock_irqsave(&vector_lock, flags); > >> - __clear_irq_vector(irq, get_irq_chip_data(irq)); > >> + desc = irq_to_desc(irq); > >> + cfg = desc->chip_data; > >> + __clear_irq_vector(desc, cfg); > > > > __clear_irq_vector(desc, desc->chip_data); > > > > should be sufficient, right ? > > You want to deliberately loose a modicum of type safety? I really have a hard time to see how assigning a void pointer to a struct irq_cfg pointer is anymore type safe than using the void pointer as for the function argument right away. > >> if (printk_ratelimit()) > >> - pr_emerg("%s: %d.%d No irq handler for vector (irq %d)\n", > >> - __func__, smp_processor_id(), vector, irq); > >> + pr_emerg("%s: %d.%d No irq handler for vector\n", > > > > That printk is confusing. It's not lacking an irq handler. The > > vector is simply not assigned. > > Long evolution. Do you have a suggestion of better wording? You mean hysterical raisins. Ok, how about: pr_emerg("irq: %d.d irq vector not assigned\n", ...); Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |