Messages in this thread | | | Date | Sun, 9 Nov 2008 00:03:40 -0800 | From | "Yinghai Lu" <> | Subject | Re: [RFC PATCH] sparse_irq aka dyn_irq |
| |
On Sat, Nov 8, 2008 at 11:38 PM, Ingo Molnar <mingo@elte.hu> wrote: > > General impression: very nice patch! > > A lot of the structural problems have been addressed: the descriptor > lookup is now hashed, the dynarray stuff got cleaned up / eliminated, > the irq_desc->chip_data binding is very nice as well. > > (And the patch needs to be split up like it was in the past, once all > review feedback has been seen and addressed.) > >> +config HAVE_SPARSE_IRQ >> + bool >> + default y > > i think it should be made user-configurable - at least initially. It > should not cause extra complications, right?
io_apic.c will get more complicated.
> >> + if (irq < NR_IRQS_LEGACY) { > > please s/NR_IRQS_LEGACY/NR_IRQS_X86_LEGACY - this is never used > outside of x86 code.
will use that in kernel/irq/handle.c too, because dyn_array is dumped.
> >> + cfg_new = desc_new->chip_data; > > the chip_data binding is a nice touch. > >> - irq_want = build_irq_for_pci_dev(dev) + 0x100; >> + irq_want = build_irq_for_pci_dev(dev) + 0xfff; > > please replace magic constant with a properly named constant. > >> - if (WARN_ON(nr > NR_IRQS)) >> - nr = NR_IRQS; > > this will have to stay for the !SPARSE_IRQ case.
Yes
> >> +++ linux-2.6/arch/x86/mm/init_32.c >> @@ -66,6 +66,7 @@ static unsigned long __meminitdata table >> static unsigned long __meminitdata table_top; >> >> static int __initdata after_init_bootmem; >> +int after_bootmem; >> >> static __init void *alloc_low_page(unsigned long *phys) >> { >> @@ -987,6 +988,8 @@ void __init mem_init(void) >> >> set_highmem_pages_init(); >> >> + after_bootmem = 1; > > this hack can go away once we have a proper percpu_alloc() that can be > used early enough.
where is that fancy patch? current percpu_alloc(), will keep big pointer in array..., instead of put that pointer in percpu_area
64bit has that after_bootmem already.
> >> +#ifndef CONFIG_HAVE_SPARSE_IRQ > > i'd suggest s/HAVE_SPARSE_IRQ/SPARSE_IRQ - as the HAVE_* flags are for > architecture code to signal the presence of a facility.
OK
> >> +#ifndef CONFIG_HAVE_SPARSE_IRQ >> if (irq >= nr_irqs) >> return; >> +#endif > > we should hide as many ugly #ifdefs as possible, and define nr_irqs to > NR_IRQS in the !SPARSE_IRQ case. > >> +++ linux-2.6/drivers/pci/htirq.c >> @@ -82,6 +82,18 @@ void unmask_ht_irq(unsigned int irq) >> write_ht_irq_msg(irq, &msg); >> } >> >> +static unsigned int build_irq_for_pci_dev(struct pci_dev *dev) >> +{ >> + unsigned int irq; >> + >> + irq = dev->bus->number; >> + irq <<= 8; >> + irq |= dev->devfn; >> + irq <<= 12; >> + >> + return irq; > > magic constants should be named.
should add more comment here.
> >> +#ifdef CONFIG_HAVE_SPARSE_IRQ >> + irq = create_irq_nr(irq_want + idx); >> +#else >> irq = create_irq(); >> +#endif > > please eliminate this #ifdef by adding one new API: > create_irq_nr(idx), which just maps to the create_irq() API in the > !SPARSE_IRQ case. > >> static struct irq_2_iommu *irq_2_iommu(unsigned int irq) >> { >> - return (irq < nr_irqs) ? irq_2_iommuX + irq : NULL; >> + struct irq_desc *desc; >> + >> + desc = irq_to_desc(irq); >> + >> + BUG_ON(!desc); >> + >> + return desc->irq_2_iommu; > > the BUG_ON() is not too friendly, please do something like this > instead: > > if (WARN_ON_ONCE(!desc)) > return NULL; > >> +#ifndef CONFIG_HAVE_SPARSE_IRQ >> /* protect irq_2_iommu_alloc later */ >> if (irq >= nr_irqs) >> return -1; >> +#endif > > this #ifdef can be eliminated too and turned into straight code via > the #define nr_irqs NR_IRQS trick in the !SPARSE_IRQ case. > >> - for_each_irq_desc(i, desc) >> + for_each_irq_desc(i, desc) { >> desc->affinity = cpumask_of_cpu(0); >> + } end_for_each_irq_desc(); > > Sidenote: later on, once the patch is upstream, we should do a global > rename: > > s/for_each_irq_desc/do_each_irq_desc > s/end_for_each_irq_desc/while_each_irq_desc > > as it's much harder to miss the "while" in a "do ..." loop, than it is > to miss the "end" in a "for" loop. > >> +#ifdef CONFIG_HAVE_SPARSE_IRQ >> +static struct irq_desc irq_desc_init = { >> + .irq = -1U, >> + .status = IRQ_DISABLED, >> + .chip = &no_irq_chip, >> + .handle_irq = handle_bad_irq, >> + .depth = 1, >> + .lock = __SPIN_LOCK_UNLOCKED(irq_desc_init.lock), >> +#ifdef CONFIG_SMP >> + .affinity = CPU_MASK_ALL >> +#endif >> +}; > > please align structure fields vertically. > >> +static struct irq_desc irq_desc_legacy[NR_IRQS_LEGACY] __cacheline_aligned_in_smp = { >> + [0 ... NR_IRQS_LEGACY-1] = { >> + .irq = -1U, >> + .status = IRQ_DISABLED, >> + .chip = &no_irq_chip, >> + .handle_irq = handle_bad_irq, >> + .depth = 1, >> + .lock = __SPIN_LOCK_UNLOCKED(irq_desc_init.lock), >> +#ifdef CONFIG_SMP >> + .affinity = CPU_MASK_ALL >> +#endif >> + } >> +}; > > same here. > >> @@ -199,7 +200,6 @@ extern void reinit_intr_remapped_IO_APIC >> #endif >> >> extern int probe_nr_irqs(void); >> - >> #else /* !CONFIG_X86_IO_APIC */ >> #define io_apic_assign_pci_irqs 0 >> static const int timer_through_8259 = 0; > > that's a spurious removal of a newline.
.....
YH
| |