lkml.org 
[lkml]   [2008]   [Nov]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] sparse_irq aka dyn_irq v13
    On Thu, Nov 13, 2008 at 1:18 PM, Andrew Morton
    <akpm@linux-foundation.org> wrote:
    > On Thu, 13 Nov 2008 12:16:56 -0800
    > Yinghai Lu <yinghai@kernel.org> wrote:
    >
    >> From: Yinghai Lu <yinghai@kernel.org>
    >> Subject: sparseirq v13
    >
    > My overall view on this is that it takes some of the kernel's most
    > fragile and most problem-dense code and makes it much more complex, and
    > by adding new configuration options it significantly worsens our
    > testing coverage.
    >
    > The patch is HHHHHUUUUUUUUUUGGGGGEEE! Did it really need to be a
    > single megapatch?
    >
    > Other architectures want (or have) sparse interrupts. Are those guys
    > paying attention here?
    >
    > I don't have a clue what all this does. I hope those who will work on
    > this code are sufficiently familiar with it all to be able to maintain
    > it when there are close to zero comments in some of our most tricky and
    > problem-prone code.
    >
    >>
    >> ...
    >>
    >> +config SPARSE_IRQ
    >> + bool "Support sparse irq numbering"
    >> + depends on PCI_MSI || HT_IRQ
    >> + default y
    >> + help
    >> + This enables support for sparse irq, esp for msi/msi-x. the irq
    >> + number will be bus/dev/fn + 12bit. You may need if you have lots of
    >> + cards supports msi-x installed.
    >> +
    >> + If you don't know what to do here, say Y.
    >> +
    >> +config MOVE_IRQ_DESC
    >> + bool "Move irq desc when changing irq smp_affinity"
    >> + depends on SPARSE_IRQ && SMP
    >> + default y
    >> + help
    >> + This enables moving irq_desc to cpu/node that irq will use handled.
    >> +
    >> + If you don't know what to do here, say Y.
    >
    > Do these reeeealy have to exist? How are users to know which to
    > choose? Which option will distros choose and why did we make them have
    > to decide?

    want to use that as marker, so later could split the patch to small
    ones by enabling by steps.

    >
    >>
    >> ...
    >>
    >> +{
    >> + struct irq_pin_list *pin;
    >> + int node;
    >> +
    >> + if (cpu < 0)
    >> + cpu = smp_processor_id();
    >> + node = cpu_to_node(cpu);
    >> +
    >> + pin = kzalloc_node(sizeof(*pin), GFP_KERNEL, node);
    >
    > It's a bug to call smp_processor_id() from preemptible code and it's a
    > bug to use GFP_KERNEL in non-preemptible code. How can this be?

    the could should be executed in boot stage, only bsp is running, or
    interrupt conext
    via irq_complete_move

    >
    >> + printk(KERN_DEBUG " alloc irq_2_pin on cpu %d node %d\n", cpu, node);
    >> +
    >> + return pin;
    >> +}
    >>
    >>
    >> ...
    >>
    >> -static struct irq_cfg *irq_cfg_alloc(unsigned int irq)
    >> +static struct irq_cfg *get_one_free_irq_cfg(int cpu)
    >> {
    >> - return irq_cfg(irq);
    >> + struct irq_cfg *cfg;
    >> + int node;
    >> +
    >> + if (cpu < 0)
    >> + cpu = smp_processor_id();
    >> + node = cpu_to_node(cpu);
    >> +
    >> + cfg = kzalloc_node(sizeof(*cfg), GFP_KERNEL, node);
    >
    > See above.
    >
    >> + printk(KERN_DEBUG " alloc irq_cfg on cpu %d node %d\n", cpu, node);
    >> +
    >> + return cfg;
    >> }
    >
    > So all callers of this function must test the return value and if it is
    > NULL, take appropriate action.
    >
    > That is something which should have been documented in this function's
    > interface description. Only that doesn't exist.
    >
    > The one caller which I checked (arch_init_copy_chip_data()) fails to
    > check for this and will oops.

    will add one function to wrap it.

    >
    >>
    >> ...
    >>
    >> +static void set_extra_move_desc(struct irq_desc *desc, cpumask_t mask)
    >> +{
    >> + struct irq_cfg *cfg = desc->chip_data;
    >> +
    >> + if (!cfg->move_in_progress) {
    >> + /* it means that domain is not changed */
    >> + cpumask_t tmp;
    >> +
    >> + cpus_and(tmp, desc->affinity, mask);
    >> + if (cpus_empty(tmp))
    >> + cfg->move_desc_in_progress_in_same_domain = 1;
    >> + }
    >
    > Aren't we trying to avoid on-stack cpumask_t's?
    >
    > I'd have though that this one could be eliminated via the use of
    > cpus_intersects()?

    will change to that.

    there some other in __assign_irq_vector...

    >
    >> }
    >> +#endif
    >>
    >> ...
    >>
    >> static struct irq_chip lapic_chip __read_mostly = {
    >> .name = "local-APIC",
    >> .mask = mask_lapic_irq,
    >> @@ -2574,7 +2834,9 @@ int timer_through_8259 __initdata;
    >> */
    >> static inline void __init check_timer(void)
    >
    > An inlined __init function makes little sense.

    should be done other patch...?

    >
    >>
    >> ...
    >>
    >> @@ -3306,10 +3590,13 @@ static void dmar_msi_set_affinity(unsign
    >> if (cpus_empty(tmp))
    >> return;
    >
    > `tmp' is always a bad choice of identifier.

    other patch?

    >
    >>
    >> ...
    >>
    >> --- linux-2.6.orig/arch/x86/mm/init_32.c
    >> +++ 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;
    >
    > This isn't a very well-chosen identifier for an x86-specific global.

    it is not used, will remove that.

    >
    >>
    >> ...
    >>
    >> @@ -98,6 +126,7 @@ int __ht_create_irq(struct pci_dev *dev,
    >> int max_irq;
    >> int pos;
    >> int irq;
    >> + unsigned int irq_want;
    >>
    >> pos = pci_find_ht_capability(dev, HT_CAPTYPE_IRQ);
    >> if (!pos)
    >> @@ -125,7 +154,12 @@ int __ht_create_irq(struct pci_dev *dev,
    >> cfg->msg.address_lo = 0xffffffff;
    >> cfg->msg.address_hi = 0xffffffff;
    >>
    >> + irq_want = build_irq_for_pci_dev(dev);
    >> +#ifdef CONFIG_SPARSE_IRQ
    >> + irq = create_irq_nr(irq_want + idx);
    >> +#else
    >> irq = create_irq();
    >> +#endif
    >
    > irq_want is unused if CONFIG_SPARSE_IRQ=n.
    >
    >> if (irq <= 0) {
    >> kfree(cfg);
    >> Index: linux-2.6/drivers/pci/intr_remapping.c
    >> ===================================================================
    >> --- linux-2.6.orig/drivers/pci/intr_remapping.c
    >> +++ linux-2.6/drivers/pci/intr_remapping.c
    >> @@ -19,17 +19,73 @@ struct irq_2_iommu {
    >> u8 irte_mask;
    >> };
    >>
    >> -static struct irq_2_iommu irq_2_iommuX[NR_IRQS];
    >> +#ifdef CONFIG_SPARSE_IRQ
    >> +static struct irq_2_iommu *get_one_free_irq_2_iommu(int cpu)
    >> +{
    >> + struct irq_2_iommu *iommu;
    >> + int node;
    >> +
    >> + if (cpu < 0)
    >> + cpu = smp_processor_id();
    >> + node = cpu_to_node(cpu);
    >> +
    >> + iommu = kzalloc_node(sizeof(*iommu), GFP_KERNEL, node);
    >
    > See above.
    >
    >> + printk(KERN_DEBUG "alloc irq_2_iommu on cpu %d node %d\n", cpu, node);
    >> +
    >> + return iommu;
    >> +}
    >>
    >>
    >> ...
    >>
    >> --- linux-2.6.orig/fs/proc/stat.c
    >> +++ linux-2.6/fs/proc/stat.c
    >> @@ -27,6 +27,9 @@ static int show_stat(struct seq_file *p,
    >> u64 sum = 0;
    >> struct timespec boottime;
    >> unsigned int per_irq_sum;
    >> +#ifdef CONFIG_GENERIC_HARDIRQS
    >> + struct irq_desc *desc;
    >> +#endif
    >>
    >> user = nice = system = idle = iowait =
    >> irq = softirq = steal = cputime64_zero;
    >> @@ -44,10 +47,9 @@ static int show_stat(struct seq_file *p,
    >> softirq = cputime64_add(softirq, kstat_cpu(i).cpustat.softirq);
    >> steal = cputime64_add(steal, kstat_cpu(i).cpustat.steal);
    >> guest = cputime64_add(guest, kstat_cpu(i).cpustat.guest);
    >> -
    >> - for_each_irq_nr(j)
    >> + for_each_irq_desc(j, desc) {
    >
    > This won't compile if CONFIG_GENERIC_HARDIRQS=n, I suspect.
    >
    we have

    #ifndef CONFIG_GENERIC_HARDIRQS
    #include <asm/irq.h>
    # define nr_irqs NR_IRQS

    # define for_each_irq_desc(irq, desc) \
    for (irq = 0; irq < nr_irqs; irq++)
    # define end_for_each_irq_desc()


    >> sum += kstat_irqs_cpu(j, i);
    >> -
    >> + } end_for_each_irq_desc();
    >> sum += arch_irq_stat_cpu(i);
    >> }
    >> sum += arch_irq_stat();
    >> @@ -90,14 +92,17 @@ static int show_stat(struct seq_file *p,
    >> seq_printf(p, "intr %llu", (unsigned long long)sum);
    >>
    >> /* sum again ? it could be updated? */
    >> - for_each_irq_nr(j) {
    >> + for_each_irq_desc(j, desc) {
    >
    > ditto.
    >
    >>
    >> ...
    >>
    >> +#define end_for_each_irq_desc()
    >> +#endif
    >> +
    >> +#define desc_chip_ack(irq, descp) desc->chip->ack(irq)
    >> +#define desc_chip_mask(irq, descp) desc->chip->mask(irq)
    >> +#define desc_chip_mask_ack(irq, descp) desc->chip->mask_ack(irq)
    >> +#define desc_chip_unmask(irq, descp) desc->chip->unmask(irq)
    >> +#define desc_chip_eoi(irq, descp) desc->chip->eoi(irq)
    >
    > Was it necessary to implement these as macros?

    try to avoid bunch of #idef with different parameters that need to be passed.

    >
    >>
    >> ...
    >>
    >> @@ -49,6 +56,299 @@ void handle_bad_irq(unsigned int irq, st
    >> int nr_irqs = NR_IRQS;
    >> EXPORT_SYMBOL_GPL(nr_irqs);
    >>
    >> +void __init __attribute__((weak)) arch_early_irq_init_work(void)
    >> +{
    >> +}
    >> +
    >> +#ifdef CONFIG_SPARSE_IRQ
    >> +static struct irq_desc irq_desc_init = {
    >> + .irq = -1U,
    >
    > Plain old `-1' would be better here. It works in all cases and the
    > reader doesn't need to go and check that this field really is an
    > unsigned int and it won't need editing if that field gets changed to
    > long
    .
    ok.

    >
    >> + .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
    >> +};
    >> +
    >> +static void init_kstat_irqs(struct irq_desc *desc, int cpu, int nr)
    >> +{
    >> + unsigned long bytes;
    >> + char *ptr;
    >> + int node;
    >> +
    >> + /* Compute how many bytes we need per irq and allocate them */
    >> + bytes = nr * sizeof(unsigned int);
    >> +
    >> + if (cpu < 0)
    >> + cpu = smp_processor_id();
    >> +
    >> + node = cpu_to_node(cpu);
    >> + ptr = kzalloc_node(bytes, GFP_KERNEL, node);
    >
    > See above.
    >
    >> + printk(KERN_DEBUG " alloc kstat_irqs on cpu %d node %d\n", cpu, node);
    >> +
    >> + desc->kstat_irqs = (unsigned int *)ptr;
    >> +}
    >> +
    >>
    >> ...
    >>
    >> +#endif
    >> +/*
    >> + * Protect the sparse_irqs_free freelist:
    >> + */
    >> +static DEFINE_SPINLOCK(sparse_irq_lock);
    >> +LIST_HEAD(sparse_irqs_head);
    >
    > It's strange that the list is global and is accessed from other .c
    > files, but the lock which protects it is static
    .
    only protect that when try to append the list. with rcu add tail.

    >
    >> +/*
    >> + * The sparse irqs are in a hash-table as well, for fast lookup:
    >> + */
    >> +#define SPARSEIRQHASH_BITS (13 - 1)
    >> +#define SPARSEIRQHASH_SIZE (1UL << SPARSEIRQHASH_BITS)
    >> +#define __sparseirqhashfn(key) hash_long((unsigned long)key, SPARSEIRQHASH_BITS)
    >> +#define sparseirqhashentry(key) (sparseirqhash_table + __sparseirqhashfn((key)))
    >
    > Why implement these via macros?

    copied from sched.c

    >
    >> +static struct list_head sparseirqhash_table[SPARSEIRQHASH_SIZE];
    >> +
    >> +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
    >> + }
    >> +};
    >> +
    >> +/* FIXME: use bootmem alloc ...*/
    >> +static unsigned int kstat_irqs_legacy[NR_IRQS_LEGACY][NR_CPUS];
    >
    > Do these need to be 32-bit? Maybe they'll fit in 16-bit, dunno.
    >

    struct irq_desc {
    unsigned int irq;
    #ifdef CONFIG_SPARSE_IRQ
    struct list_head list;
    struct list_head hash_entry;
    struct timer_rand_state *timer_rand_state;
    unsigned int *kstat_irqs;

    >> +void __init early_irq_init_work(void)
    >
    > The use of "_work" implies that this function is invoked by
    > schedule_work(). But it isn't

    ok, will remove it.

    >
    >> +{
    >> + struct irq_desc *desc;
    >> + int legacy_count;
    >> + int i;
    >> +
    >> + /* init_work to init list for sparseirq */
    >> + for (i = 0; i < SPARSEIRQHASH_SIZE; i++)
    >> + INIT_LIST_HEAD(sparseirqhash_table + i);
    >> +
    >> + desc = irq_desc_legacy;
    >> + legacy_count = ARRAY_SIZE(irq_desc_legacy);
    >> +
    >> + for (i = 0; i < legacy_count; i++) {
    >> + struct list_head *hash_head;
    >> +
    >> + hash_head = sparseirqhashentry(i);
    >> + desc[i].irq = i;
    >> + desc[i].kstat_irqs = kstat_irqs_legacy[i];
    >> + list_add_tail(&desc[i].hash_entry, hash_head);
    >> + list_add_tail(&desc[i].list, &sparse_irqs_head);
    >> + }
    >> +
    >> + arch_early_irq_init_work();
    >> +}
    >> +
    >>
    >> ...
    >>
    >> +struct irq_desc *irq_to_desc_alloc_cpu(unsigned int irq, int cpu)
    >> +{
    >> + struct irq_desc *desc;
    >> + struct list_head *hash_head;
    >> + unsigned long flags;
    >> + int node;
    >> +
    >> + desc = irq_to_desc(irq);
    >> + if (desc)
    >> + return desc;
    >> +
    >> + hash_head = sparseirqhashentry(irq);
    >> +
    >> + spin_lock_irqsave(&sparse_irq_lock, flags);
    >> +
    >> + /*
    >> + * We have to do the hash-walk again, to avoid races
    >> + * with another CPU:
    >> + */
    >> + list_for_each_entry(desc, hash_head, hash_entry)
    >> + if (desc->irq == irq)
    >> + goto out_unlock;
    >> +
    >> + if (cpu < 0)
    >> + cpu = smp_processor_id();
    >> +
    >> + node = cpu_to_node(cpu);
    >> + desc = kzalloc_node(sizeof(*desc), GFP_KERNEL, node);
    >
    > Oh for gawd's sake. PLEASE read Documentation/SubmitChecklist.
    > Carefully. We've already discussed this.
    there are 13 errors with checkpatch scripts. seems all about macro definition.

    >
    > You cannot do a GFP_KERNEL allocation under spin_lock_irqsave().
    >
    >> + printk(KERN_DEBUG " alloc irq_desc for %d aka %#x on cpu %d node %d\n",
    >> + irq, irq, cpu, node);
    >> + init_one_irq_desc(irq, desc, cpu);
    >> +
    >> + /*
    >> + * We use RCU's safe list-add method to make
    >> + * parallel walking of the hash-list safe:
    >> + */
    >> + list_add_tail_rcu(&desc->hash_entry, hash_head);
    >> + /*
    >> + * Add it to the global list:
    >> + */
    >> + list_add_tail_rcu(&desc->list, &sparse_irqs_head);
    >> +
    >> +out_unlock:
    >> + spin_unlock_irqrestore(&sparse_irq_lock, flags);
    >> +
    >> + return desc;
    >> +}
    >> +
    >> +struct irq_desc *irq_to_desc_alloc(unsigned int irq)
    >> +{
    >> + return irq_to_desc_alloc_cpu(irq, -1);
    >> +}
    >> +
    >> +#ifdef CONFIG_MOVE_IRQ_DESC
    >> +static struct irq_desc *__real_move_irq_desc(struct irq_desc *old_desc,
    >> + int cpu)
    >> +{
    >> + struct irq_desc *desc;
    >> + unsigned int irq;
    >> + struct list_head *hash_head;
    >> + unsigned long flags;
    >> + int node;
    >> +
    >> + irq = old_desc->irq;
    >> +
    >> + hash_head = sparseirqhashentry(irq);
    >> +
    >> + spin_lock_irqsave(&sparse_irq_lock, flags);
    >> + /*
    >> + * We have to do the hash-walk again, to avoid races
    >> + * with another CPU:
    >> + */
    >> + list_for_each_entry(desc, hash_head, hash_entry)
    >> + if (desc->irq == irq && old_desc != desc)
    >> + goto out_unlock;
    >> +
    >> + if (cpu < 0)
    >> + cpu = smp_processor_id();
    >> +
    >> + node = cpu_to_node(cpu);
    >> + desc = kzalloc_node(sizeof(*desc), GFP_KERNEL, node);
    >
    > Ditto.
    >
    > Also, the return value from the memory allocation attempt is not
    > checked.
    >
    >> + printk(KERN_DEBUG " move irq_desc for %d aka %#x to cpu %d node %d\n",
    >> + irq, irq, cpu, node);
    >> +
    >> + init_copy_one_irq_desc(irq, old_desc, desc, cpu);
    >> +
    >> + list_replace_rcu(&old_desc->hash_entry, &desc->hash_entry);
    >> + list_replace_rcu(&old_desc->list, &desc->list);
    >> +
    >> + /* free the old one */
    >> + free_one_irq_desc(old_desc);
    >> + kfree(old_desc);
    >> +
    >> +out_unlock:
    >> + spin_unlock_irqrestore(&sparse_irq_lock, flags);
    >> +
    >> + return desc;
    >> +}
    >> +
    >>
    >> ...
    >>
    >> --- linux-2.6.orig/init/main.c
    >> +++ linux-2.6/init/main.c
    >> @@ -542,6 +542,15 @@ void __init __weak thread_info_cache_ini
    >> {
    >> }
    >>
    >> +void __init __attribute__((weak)) arch_early_irq_init_work(void)
    >> +{
    >> +}
    >> +
    >> +void __init __attribute__((weak)) early_irq_init_work(void)
    >> +{
    >> + arch_early_irq_init_work();
    >> +}
    >
    > Please use __weak

    ok

    thanks for reviewing.

    YH


    \
     
     \ /
      Last update: 2008-11-13 23:03    [W:0.067 / U:0.036 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site