| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <86802c440811090003g5ac53822y852a4c1096228f8b@mail.gmail.com>
Date: Sun, 9 Nov 2008 00:03:40 -0800
From: "Yinghai Lu" <yinghai@...nel.org>
To: "Ingo Molnar" <mingo@...e.hu>
Cc: "Thomas Gleixner" <tglx@...utronix.de>,
"H. Peter Anvin" <hpa@...or.com>,
"Andrew Morton" <akpm@...ux-foundation.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH] sparse_irq aka dyn_irq
On Sat, Nov 8, 2008 at 11:38 PM, Ingo Molnar <mingo@...e.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists