linux-kernel - Re: [RFC PATCH] sparse_irq aka dyn

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
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20081109073813.GA17180@elte.hu>
Date:	Sun, 9 Nov 2008 08:38:13 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Yinghai Lu <yinghai@...nel.org>
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


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?

> +	if (irq < NR_IRQS_LEGACY) {

please s/NR_IRQS_LEGACY/NR_IRQS_X86_LEGACY - this is never used 
outside of x86 code.

> +		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.

> +++ 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.

> +#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.

> +#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.

> +#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.

all in one, i cannot see fundamental problems in this patch.

	Ingo
--
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/