lkml.org 
[lkml]   [2011]   [Oct]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 5/8] x86, common.c, smpboot.c: Init BSP during BSP online and don't offline BSP if irq is bound to it
    On Wed, 5 Oct 2011, Fenghua Yu wrote:

    > From: Fenghua Yu <fenghua.yu@intel.com>
    >
    > During BSP online, enable x2apic and initialize BSP. Don't offline BSP if
    > any irq is bound to it.
    >
    > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
    > ---
    > arch/x86/include/asm/processor.h | 1 +
    > arch/x86/kernel/cpu/common.c | 13 ++++++++++---
    > arch/x86/kernel/smpboot.c | 38 +++++++++++++++++++++++++++++++-------
    > 3 files changed, 42 insertions(+), 10 deletions(-)
    >
    > diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
    > index 0d1171c..d648cae 100644
    > --- a/arch/x86/include/asm/processor.h
    > +++ b/arch/x86/include/asm/processor.h
    > @@ -161,6 +161,7 @@ extern struct pt_regs *idle_regs(struct pt_regs *);
    >
    > extern void early_cpu_init(void);
    > extern void identify_boot_cpu(void);
    > +extern void identify_boot_cpu_online(void);
    > extern void identify_secondary_cpu(struct cpuinfo_x86 *);
    > extern void print_cpu_info(struct cpuinfo_x86 *);
    > extern void init_scattered_cpuid_features(struct cpuinfo_x86 *c);
    > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
    > index 6218439..2ceefb9 100644
    > --- a/arch/x86/kernel/cpu/common.c
    > +++ b/arch/x86/kernel/cpu/common.c
    > @@ -911,6 +911,14 @@ void __init identify_boot_cpu(void)
    > #endif
    > }
    >
    > +void __cpuinit identify_boot_cpu_online(void)
    > +{
    > + numa_add_cpu(smp_processor_id());
    > +#ifdef CONFIG_X86_32
    > + enable_sep_cpu();
    > +#endif

    Why this change? This has nothing to do with the changelog. And we
    have 3 other call sites for enable_sep_cpu().

    > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
    > index 9f548cb..cdfa425 100644
    > --- a/arch/x86/kernel/smpboot.c
    > +++ b/arch/x86/kernel/smpboot.c
    > @@ -50,6 +50,7 @@
    > #include <linux/tboot.h>
    > #include <linux/stackprotector.h>
    > #include <linux/gfp.h>
    > +#include <linux/kernel_stat.h>

    What is this include for?

    > #include <asm/acpi.h>
    > #include <asm/desc.h>
    > @@ -136,8 +137,8 @@ EXPORT_PER_CPU_SYMBOL(cpu_info);
    > atomic_t init_deasserted;
    >
    > /*
    > - * Report back to the Boot Processor.
    > - * Running on AP.
    > + * Report back to the Boot Processor during boot time or to the caller processor
    > + * during CPU online.
    > */
    > static void __cpuinit smp_callin(void)
    > {
    > @@ -224,6 +225,13 @@ static void __cpuinit smp_callin(void)
    > smp_store_cpu_info(cpuid);
    >
    > /*
    > + * This function won't run on the BSP during boot time. It run
    > + * on BSP only when BSP is offlined and onlined again.
    > + */
    > + if (cpuid == 0)
    > + identify_boot_cpu_online();

    Again, what's the point? numa_add_cpu() is called in identify_cpu()
    already and you just added that crap here because you failed to fix
    smp_store_cpu_info(). That whole patch set is just a sloppy hack which
    leaves tons of cpu0 assumptions all over the place instead of cleaning
    them up completely.

    > + if (cpu == 0) {
    > + int i;
    > + for (i = 0; i < NR_IRQS; i++) {
    > + struct irq_desc *desc = irq_to_desc(i);
    > +
    > + if (!desc)
    > + continue;
    > +
    > + if (irqd_irq_disabled(&desc->irq_data))
    > + continue;
    > +
    > + if (!irq_can_set_affinity(i)) {
    > + pr_debug("irq%d can't move out of BSP\n", i);
    > + return -EBUSY;
    > + }

    This is just disgusting.

    Have you ever looked at other code which iterates over the interrupt
    descriptors and if yes have you noticed that all that code uses
    iterator functions for a fcking good reason? Hint SPARSE_IRQ

    What gives you the guarantee that the interrupt is not going to be
    enabled right after you offlined the cpu? Do you really think that
    chekcing whether the interrupt is disabled is sufficient ????

    Is there any guarantee, that an interrupt which is currently not
    assigned is going to be requested after you offlined cpu0 ?

    We know upfront whether we are using interrupt chips which are not
    capable of irq affinity settings. So why the hell do you want to poke
    in the interrupt descriptors?

    Thanks,

    tglx


    \
     
     \ /
      Last update: 2011-10-05 21:15    [W:0.038 / U:31.256 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site