Message-ID: <86802c440808141342k6cb0dc59ud067018b0a171232@mail.gmail.com>
Date:	Thu, 14 Aug 2008 13:42:48 -0700
From:	"Yinghai Lu" <yhlu.kernel@...il.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	"Ingo Molnar" <mingo@...e.hu>,
	"Thomas Gleixner" <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org,
	"Alan Cox" <alan@...rguk.ukuu.org.uk>,
	"Andrew Morton" <akpm@...ux-foundation.org>
Subject: Re: [PATCH 00/53] dyn_array/nr_irqs/sparse_irq support v10

On Thu, Aug 14, 2008 at 1:05 PM, Eric W. Biederman
<ebiederm@...ssion.com> wrote:
> "Yinghai Lu" <yhlu.kernel@...il.com> writes:
>
>> On Thu, Aug 14, 2008 at 6:26 AM, Ingo Molnar <mingo@...e.hu> wrote:
>>>
>>> (lkml Cc:-ed)
>>>
>>> * Yinghai Lu <yhlu.kernel@...il.com> wrote:
>>>
>>>> v10: make the x86 32 bit support sparse_irq too.
>>>>     also make remove irqbalance in kernel for 32 bit
>>>>     and make 32 bit support per_cpu vector. so start merging or io_apic_xx.c
>>>>
>>>>     01-03 is some fix for current tree
>>>>     04: is revert for one patch hide in sched/ and tip/master
>>>>
>>>> based on tip/master
>>>>
>>>> to do:
>>>>     merge io_apic_xx.c
>>>
>>> ok, i've created a new tip/irq/sparseirq topic branch for this.
>>>
>>> Could you please send future updates against:
>>>
>>>   git-checkout tip/master
>>>   git-merge tip/irq/sparseirq
>>>
>>> Not yet integrated into tip/master, i've still got some other backlog.
>>>
>>> A couple of observations about the general structure of the sparse irqs
>>> code:
>>>
>>> - the new APIs should probably live in mm/bootmem.c not in init/main.c,
>>>  and in bootmem.h. Also, it should be outlined clearly why this new API
>>>  is needed as a wrapper ontop of existing bootmem mechanisms.
>>
>> move
>> pre_alloc_dyn_array, per_cpu_dyn_array_size, per_cpu_alloc_dyn_array
>> to mm/bootmem.c?
>>
>>>
>>> - the #ifdef complications, while fine for migration, should be
>>>  eliminated. Could we introduce some compatible form for the
>>>  definition/allocation APIs that work even if an architecture still
>>>  uses a flat irq array? That would remove most of the uglinesses.
>> # git grep CONFIG_HAVE_DYN_ARRAY | wc -l
>> 9
>>
>> #git grep CONFIG_HAVE_SPARSE_IRQ | wc -l
>> 39
>>
>> arch/x86/kernel/io_apic_32.c:#ifdef CONFIG_HAVE_SPARSE_IRQ_DEBUG
>> arch/x86/kernel/io_apic_32.c:#ifndef CONFIG_HAVE_SPARSE_IRQ
>> arch/x86/kernel/io_apic_64.c:#ifdef CONFIG_HAVE_SPARSE_IRQ_DEBUG
>> arch/x86/kernel/io_apic_64.c:#ifndef CONFIG_HAVE_SPARSE_IRQ
>> arch/x86/kernel/irq_32.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> arch/x86/kernel/irq_64.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> drivers/char/random.c:#ifndef CONFIG_HAVE_SPARSE_IRQ
>> drivers/char/random.c:#ifndef CONFIG_HAVE_SPARSE_IRQ
>> drivers/pci/intr_remapping.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> drivers/pci/intr_remapping.c:#else /* !CONFIG_HAVE_SPARSE_IRQ */
>> drivers/pci/intr_remapping.c:#ifndef    CONFIG_HAVE_SPARSE_IRQ
>> fs/proc/proc_misc.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> fs/proc/proc_misc.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> fs/proc/proc_misc.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> include/linux/irq.h:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> include/linux/irq.h:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> include/linux/irq.h:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> include/linux/irq.h:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> include/linux/irq.h:#if defined(CONFIG_INTR_REMAP) &&
>> defined(CONFIG_HAVE_SPARSE_IRQ)
>> include/linux/irq.h:#ifndef CONFIG_HAVE_SPARSE_IRQ
>> kernel/irq/chip.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> kernel/irq/chip.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> kernel/irq/chip.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> kernel/irq/chip.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> kernel/irq/chip.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> kernel/irq/handle.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> kernel/irq/handle.c:#ifndef CONFIG_HAVE_SPARSE_IRQ
>> kernel/irq/handle.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> kernel/irq/handle.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> kernel/irq/handle.c:#ifdef CONFIG_HAVE_SPARSE_IRQ_DEBUG
>> kernel/irq/handle.c:#ifndef CONFIG_HAVE_SPARSE_IRQ
>> kernel/irq/handle.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> kernel/irq/handle.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> kernel/irq/handle.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> kernel/irq/handle.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> kernel/irq/handle.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> kernel/irq/manage.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> kernel/irq/manage.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>> kernel/irq/migration.c:#ifdef CONFIG_HAVE_SPARSE_IRQ
>>
>> because try to avoid more api to be changed to take irq_desc as parameter.
>>
>>>
>>> - some of the irq < 16 checks in x86 look a bit ugly, can we do anything
>>>  about them?
>>
>> is_legacy_irq(irq) ? or legacy_irq(irq)?
>
>
>>
>>>
>>> - i think as a final-ish commit we should just remove the dyn-array
>>>  define and make all architectures use this facility. You converted
>>>  most of them - how many are still missing? Sparse-irq is more
>>>  intrusive so that should probably stay a Kconfig knob.
>>
>> so when config_sparse_irq is not selected, need to use dyn_array to
>> allocate irq_desc and irq_cfg, or just static array?
>>
>>>
>>> - there was one aspect of NR_IRQS that was nice and useful: it acted as
>>>  a sanity check against BIOS bugs and driver bugs that pass in some
>>>  irrealistically high irq number. Now we'll just try to allocate some
>>>  really high IRQ and accept it. I _think_ there should be a single,
>>>  simple 'absolute maximum IRQ number' define which should set the
>>>  maximum possible IRQ number. Lets call it NR_IRQS_MAX or so, and set
>>>  it to NR_CPUS*NR_IO_APICS*224 or so?
>
> We may want an is_valid_isa_irq() for drivers.  Otherwise drivers
> should not be passing in irqs.  And the request_irq path should
> valid the irq number but it should not allocate it.  So we will
> have failures with invalid irq numbers.
>
> For acpi, mptables and the like we can easily have a check for some
> reasonably high value if there is value in that.  Last I looked parts
> of that code were using numbers like 1024 and 4096 already.
>
>> when sparse_irq is used, irq is [0, -1U]
>> driver could be irq_desc(irq) to check if irq is valid or not. and use
>> irq_desc_with_new(new) to get a new one.
>
> Note that request_irq should do this for free.
>
>> when generic_hardirqs is not used (s390, m68k, sparc), we need to
>> create some stubs in linux/interrupt.h for them
>
> s390 is weird.  I'm not even convinced they use linux/interrupt.h
>
> Yep.  If we need more than the proper checks in request_irq.
>
>>> - i'm a bit worried about linecount increase in general:
>>>
>>>     247 files changed, 3671 insertions(+), 2052 deletions(-)
>>>
>>>  could we work on reducing that somewhat? The new infrastructure bits
>>>  in mm/* and kernel/irq/* will be unavoidable, what we should
>>>  concentrate on is to make usage of the new facility just as
>>>  straightforward, easy and compact as the old irq_desc[] usages.
>> before that we could use get_irq_desc() and get_irq_desc_without_new()
>> and according to eric, i changed that to irq_desc_with_new and irq_desc().
>>
>> so the array irq_desc[] need to be changed to irq_descX[]
>> ( or *irq_desc to *irq_descX for dyn_array)
>
> My mistake.  irq_desc() is a bad name for the conversion method
> because it conflicts like that.  I really just did not want something
> wordy, or something that suggests you need to call put_irq_desc.  BothÜ
> of which get_irq_desc() are.

it took me one day to change them. anyway irq_desc() is better than get_irq_desc

>
> What I wound up using in my tree is a little different.
>
> I introduced an opaque/empty structure: struct irq to be used in places
> where we need pointers instead of an integer irq number in the interfaces.
>
> I have a version of the genirq api that works on struct irq instead of
> unsigned int irq.
>
> I have functions:
> struct irq * to_irq(unsigned int nr);
> struct irq_desc *to_idesc(struct irq *);
> unsigned int irq_nr(struct irq *irq);

struct irq {
  unsigned int nr;
  struct irq_desc *idesc;
}

?

>
> Is there any reason why the migration path for architectures can not be:
> Until they are converted:
> #define NR_IRQS and use the irq_desc array.
>
> If they are just using a dynamically allocated array.
> #define NR_IRQS nr_irqs
>
> Once we kill the array entirely.
> #undef NR_IRQS or
> #define NR_IRQS 0xfffff000
why not use -1U here?

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