Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1514576 > unrolled thread

[PATCH 22/25] x86/mcheck: Do the init in one place

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 22/25] x86/mcheck: Do the init in one place Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-03 16:00 +0100
    Re: [PATCH 22/25] x86/mcheck: Do the init in one place Borislav Petkov <bp@alien8.de> - 2016-11-07 19:50 +0100
      Re: [PATCH 22/25] x86/mcheck: Do the init in one place "Luck, Tony" <tony.luck@intel.com> - 2016-11-07 20:00 +0100
        Re: [PATCH 22/25] x86/mcheck: Do the init in one place Borislav Petkov <bp@alien8.de> - 2016-11-07 21:20 +0100
          Re: [PATCH 22/25] x86/mcheck: Do the init in one place Borislav Petkov <bp@alien8.de> - 2016-11-08 10:30 +0100
            Re: [PATCH 22/25] x86/mcheck: Do the init in one place Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-09 15:30 +0100
              Re: [PATCH 22/25] x86/mcheck: Do the init in one place Borislav Petkov <bp@alien8.de> - 2016-11-09 16:40 +0100
                Re: [PATCH 22/25] x86/mcheck: Do the init in one place Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-09 17:30 +0100
                  Re: [PATCH 22/25] x86/mcheck: Do the init in one place Borislav Petkov <bp@alien8.de> - 2016-11-09 18:10 +0100
                    Re: [PATCH 22/25] x86/mcheck: Do the init in one place Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-09 18:30 +0100
                RE: [PATCH 22/25] x86/mcheck: Do the init in one place "Luck, Tony" <tony.luck@intel.com> - 2016-11-09 19:40 +0100
                  Re: [PATCH 22/25] x86/mcheck: Do the init in one place Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-10 10:10 +0100
                    Re: [PATCH 22/25] x86/mcheck: Do the init in one place Borislav Petkov <bp@alien8.de> - 2016-11-10 10:20 +0100
                      [PATCH 4/7] x86/mcheck: Split threshold_cpu_callback into two callbacks Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-10 18:50 +0100
                      [PATCH 3/7] x86/mcheck: Be prepared for a rollback back to the ONLINE state Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-10 18:50 +0100
                      [PATCH 1/7] x86/mcheck: Move threshold_create_device() Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-10 18:50 +0100
                      [PATCH 5/7] x86/mcheck: reorganize the hotplug callbacks Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-10 18:50 +0100
                        Re: [PATCH 5/7] x86/mcheck: reorganize the hotplug callbacks Borislav Petkov <bp@alien8.de> - 2016-11-11 20:00 +0100
                          Re: [PATCH 5/7] x86/mcheck: reorganize the hotplug callbacks Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-11 20:40 +0100
                            Re: [PATCH 5/7] x86/mcheck: reorganize the hotplug callbacks Borislav Petkov <bp@alien8.de> - 2016-11-11 21:00 +0100
                      [PATCH 2/7] x86/mcheck: Explicit cleanup on failure in mce_amd Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-10 18:50 +0100
                      [PATCH 6/7] x86/mcheck: Move CPU_ONLINE and CPU_DOWN_PREPARE to hotplug state machine Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-10 18:50 +0100
                      [PATCH 7/7] x86/mcheck: Move CPU_DEAD to hotplug state machine Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-10 18:50 +0100
                      x86/mcheck: convert to hotplug state engine, take #2 Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-10 18:50 +0100
                    Re: [PATCH 22/25] x86/mcheck: Do the init in one place Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-10 11:30 +0100
                    Re: [PATCH 22/25] x86/mcheck: Do the init in one place Thomas Gleixner <tglx@linutronix.de> - 2016-11-10 11:30 +0100

Page 1 of 2  [1] 2  Next page →

#1514576 — [PATCH 22/25] x86/mcheck: Do the init in one place

Part of the init (memory allocation and so on) is done
in mcheck_cpu_init(). While moving the the allocation to
mcheck_init_device() (where the hotplug calls are initialized) it
becomes necessary to move the callback (mcheck_cpu_init()), too.

The callback is now removed from identify_cpu() and registered as a
hotplug event which is invoked as the very first one which is shortly
after the original point of invocation (look at smp_store_cpu_info() and
notify_cpu_starting() in smp_callin()).
One "visible" difference is that MCE for the boot CPU is not enabled at
identify_boot_cpu() time but at device_initcall_sync() time. Either way,
both times we had no userland around.

Cc: Tony Luck <tony.luck@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: linux-edac@vger.kernel.org
Cc: x86@kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/mce.h       |  2 --
 arch/x86/kernel/cpu/common.c     |  4 ---
 arch/x86/kernel/cpu/mcheck/mce.c | 77 ++++++++++++++++++++++++----------------
 include/linux/cpuhotplug.h       |  1 +
 4 files changed, 48 insertions(+), 36 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 978be74a12c1..ba8108166aec 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -203,12 +203,10 @@ extern int mce_p5_enabled;
 
 #ifdef CONFIG_X86_MCE
 int mcheck_init(void);
-void mcheck_cpu_init(struct cpuinfo_x86 *c);
 void mcheck_cpu_clear(struct cpuinfo_x86 *c);
 void mcheck_vendor_init_severity(void);
 #else
 static inline int mcheck_init(void) { return 0; }
-static inline void mcheck_cpu_init(struct cpuinfo_x86 *c) {}
 static inline void mcheck_cpu_clear(struct cpuinfo_x86 *c) {}
 static inline void mcheck_vendor_init_severity(void) {}
 #endif
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 9bd910a7dd0a..283b1fa64e69 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -39,7 +39,6 @@
 #include <asm/asm.h>
 #include <asm/bugs.h>
 #include <asm/cpu.h>
-#include <asm/mce.h>
 #include <asm/msr.h>
 #include <asm/pat.h>
 #include <asm/microcode.h>
@@ -1095,9 +1094,6 @@ static void identify_cpu(struct cpuinfo_x86 *c)
 			c->x86_capability[i] |= boot_cpu_data.x86_capability[i];
 	}
 
-	/* Init Machine Check Exception if available. */
-	mcheck_cpu_init(c);
-
 	select_idle_routine(c);
 
 #ifdef CONFIG_NUMA
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 79b5ad9570ca..72af9db8526d 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1436,11 +1436,24 @@ EXPORT_SYMBOL_GPL(mce_notify_irq);
 static int __mcheck_cpu_mce_banks_init(void)
 {
 	int i;
-	u8 num_banks = mca_cfg.banks;
+	u64 cap;
+	unsigned int num_banks;
+
+	rdmsrl(MSR_IA32_MCG_CAP, cap);
+
+	num_banks = cap & MCG_BANKCNT_MASK;
+	pr_info("CPU supports %d MCE banks\n", num_banks);
+
+	if (num_banks > MAX_NR_BANKS) {
+		pr_warn("Using only %u machine check banks out of %u\n",
+			MAX_NR_BANKS, num_banks);
+		num_banks = MAX_NR_BANKS;
+	}
 
 	mce_banks = kzalloc(num_banks * sizeof(struct mce_bank), GFP_KERNEL);
 	if (!mce_banks)
 		return -ENOMEM;
+	mca_cfg.banks = num_banks;
 
 	for (i = 0; i < num_banks; i++) {
 		struct mce_bank *b = &mce_banks[i];
@@ -1462,25 +1475,11 @@ static int __mcheck_cpu_cap_init(void)
 	rdmsrl(MSR_IA32_MCG_CAP, cap);
 
 	b = cap & MCG_BANKCNT_MASK;
-	if (!mca_cfg.banks)
-		pr_info("CPU supports %d MCE banks\n", b);
-
-	if (b > MAX_NR_BANKS) {
-		pr_warn("Using only %u machine check banks out of %u\n",
-			MAX_NR_BANKS, b);
+	if (b > MAX_NR_BANKS)
 		b = MAX_NR_BANKS;
-	}
 
 	/* Don't support asymmetric configurations today */
-	WARN_ON(mca_cfg.banks != 0 && b != mca_cfg.banks);
-	mca_cfg.banks = b;
-
-	if (!mce_banks) {
-		int err = __mcheck_cpu_mce_banks_init();
-
-		if (err)
-			return err;
-	}
+	WARN_ON(b != mca_cfg.banks);
 
 	/* Use accurate RIP reporting if available. */
 	if ((cap & MCG_EXT_P) && MCG_EXT_CNT(cap) >= 9)
@@ -1769,26 +1768,22 @@ void (*machine_check_vector)(struct pt_regs *, long error_code) =
  * Called for each booted CPU to set up machine checks.
  * Must be called with preempt off:
  */
-void mcheck_cpu_init(struct cpuinfo_x86 *c)
+static int mcheck_cpu_starting(unsigned int cpu)
 {
+	struct cpuinfo_x86 *c = &cpu_data(cpu);
+
 	if (mca_cfg.disabled)
-		return;
+		return 0;
 
 	if (__mcheck_cpu_ancient_init(c))
-		return;
+		return 0;
 
 	if (!mce_available(c))
-		return;
+		return 0;
 
 	if (__mcheck_cpu_cap_init() < 0 || __mcheck_cpu_apply_quirks(c) < 0) {
 		mca_cfg.disabled = true;
-		return;
-	}
-
-	if (mce_gen_pool_init()) {
-		mca_cfg.disabled = true;
-		pr_emerg("Couldn't allocate MCE records pool!\n");
-		return;
+		return 0;
 	}
 
 	machine_check_vector = do_machine_check;
@@ -1797,6 +1792,7 @@ void mcheck_cpu_init(struct cpuinfo_x86 *c)
 	__mcheck_cpu_init_vendor(c);
 	__mcheck_cpu_init_clear_banks();
 	__mcheck_cpu_init_timer();
+	return 0;
 }
 
 /*
@@ -2584,11 +2580,26 @@ static __init int mcheck_init_device(void)
 		goto err_out;
 	}
 
+	err = __mcheck_cpu_mce_banks_init();
+	if (err)
+		goto err_out_mem;
+
 	mce_init_banks();
 
+	err = mce_gen_pool_init();
+	if (err) {
+		pr_emerg("Couldn't allocate MCE records pool!\n");
+		goto err_init_pool;
+	}
+
 	err = subsys_system_register(&mce_subsys, NULL);
 	if (err)
-		goto err_out_mem;
+		goto err_init_pool;
+
+	err = cpuhp_setup_state(CPUHP_AP_X86_MCE_STARTING, "x86/mce:starting",
+				mcheck_cpu_starting, NULL);
+	if (err)
+		goto err_init_pool;
 
 	cpu_notifier_register_begin();
 	for_each_online_cpu(i) {
@@ -2630,12 +2641,18 @@ static __init int mcheck_init_device(void)
 	for_each_possible_cpu(i)
 		mce_device_remove(i);
 
+	cpuhp_remove_state(CPUHP_AP_X86_MCE_STARTING);
+
+err_init_pool:
+	mca_cfg.banks = 0;
+	kfree(mce_banks);
+	mce_banks = NULL;
+
 err_out_mem:
 	free_cpumask_var(mce_device_initialized);
 
 err_out:
 	pr_err("Unable to init device /dev/mcelog (rc: %d)\n", err);
-
 	return err;
 }
 device_initcall_sync(mcheck_init_device);
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 3410d83cc2e2..4fe2ba418471 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -64,6 +64,7 @@ enum cpuhp_state {
 	CPUHP_BRINGUP_CPU,
 	CPUHP_AP_IDLE_DEAD,
 	CPUHP_AP_OFFLINE,
+	CPUHP_AP_X86_MCE_STARTING,
 	CPUHP_AP_SCHED_STARTING,
 	CPUHP_AP_RCUTREE_DYING,
 	CPUHP_AP_IRQ_GIC_STARTING,
-- 
2.10.2

[toc] | [next] | [standalone]

#1516472

On Thu, Nov 03, 2016 at 03:50:18PM +0100, Sebastian Andrzej Siewior wrote:
> Part of the init (memory allocation and so on) is done
> in mcheck_cpu_init(). While moving the the allocation to
> mcheck_init_device() (where the hotplug calls are initialized) it
> becomes necessary to move the callback (mcheck_cpu_init()), too.
> 
> The callback is now removed from identify_cpu() and registered as a
> hotplug event which is invoked as the very first one which is shortly
> after the original point of invocation (look at smp_store_cpu_info() and
> notify_cpu_starting() in smp_callin()).
> One "visible" difference is that MCE for the boot CPU is not enabled at
> identify_boot_cpu() time but at device_initcall_sync() time. Either way,
> both times we had no userland around.

Uh, hm, I'm not sure about this: so the issue I see with this is that
the more we're delaying the enabling or MCE reporting - and especially
setting CR4[MCE] - the more we're increasing the window where a MCE
during early boot will cause a shutdown. (This is what happens if
CR4[MCE]=0b).

Perhaps we should split the init into a very early init which doesn't
need to be part of hotplug and the rest, which can do mce_disable_cpu()
and mce_reenable_cpu().

Tony, how do you see this?

> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: linux-edac@vger.kernel.org
> Cc: x86@kernel.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---

...


> @@ -2584,11 +2580,26 @@ static __init int mcheck_init_device(void)
>  		goto err_out;
>  	}
>  
> +	err = __mcheck_cpu_mce_banks_init();
		^^^^^^^^

I guess you can merge this one...

> +	if (err)
> +		goto err_out_mem;
> +
>  	mce_init_banks();
	^^^^^^^^

into this one now.

But let's sort out the bigger issue first.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1516488

On Mon, Nov 07, 2016 at 07:45:32PM +0100, Borislav Petkov wrote:
> On Thu, Nov 03, 2016 at 03:50:18PM +0100, Sebastian Andrzej Siewior wrote:
> > Part of the init (memory allocation and so on) is done
> > in mcheck_cpu_init(). While moving the the allocation to
> > mcheck_init_device() (where the hotplug calls are initialized) it
> > becomes necessary to move the callback (mcheck_cpu_init()), too.
> > 
> > The callback is now removed from identify_cpu() and registered as a
> > hotplug event which is invoked as the very first one which is shortly
> > after the original point of invocation (look at smp_store_cpu_info() and
> > notify_cpu_starting() in smp_callin()).
> > One "visible" difference is that MCE for the boot CPU is not enabled at
> > identify_boot_cpu() time but at device_initcall_sync() time. Either way,
> > both times we had no userland around.
> 
> Uh, hm, I'm not sure about this: so the issue I see with this is that
> the more we're delaying the enabling or MCE reporting - and especially
> setting CR4[MCE] - the more we're increasing the window where a MCE
> during early boot will cause a shutdown. (This is what happens if
> CR4[MCE]=0b).
> 
> Perhaps we should split the init into a very early init which doesn't
> need to be part of hotplug and the rest, which can do mce_disable_cpu()
> and mce_reenable_cpu().
> 
> Tony, how do you see this?

I don't think that helps as much as you'd like it to help (at
least on Intel). A broadcast machine check that finds the boot
CPU has set CR4[MCE]=1 is still going to end up in reset if any
other CPU still has CR4[MCE]=0

-Tony

[toc] | [prev] | [next] | [standalone]

#1516561

On Mon, Nov 07, 2016 at 10:55:24AM -0800, Luck, Tony wrote:
> I don't think that helps as much as you'd like it to help (at
> least on Intel). A broadcast machine check that finds the boot
> CPU has set CR4[MCE]=1 is still going to end up in reset if any
> other CPU still has CR4[MCE]=0

By leaving/moving the setting of CR4 earlier on all cores, we'll
at least make the possible window for such potential resets a lot
smaller...

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1516992

On Mon, Nov 07, 2016 at 09:12:24PM +0100, Borislav Petkov wrote:
> On Mon, Nov 07, 2016 at 10:55:24AM -0800, Luck, Tony wrote:
> > I don't think that helps as much as you'd like it to help (at
> > least on Intel). A broadcast machine check that finds the boot
> > CPU has set CR4[MCE]=1 is still going to end up in reset if any
> > other CPU still has CR4[MCE]=0
> 
> By leaving/moving the setting of CR4 earlier on all cores, we'll
> at least make the possible window for such potential resets a lot
> smaller...

... and in general, I'm still unsure about *why* we need this change for
hotplug. bigeasy, can you please clarify first?

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1518205

On 2016-11-08 10:23:02 [+0100], Borislav Petkov wrote:
> On Mon, Nov 07, 2016 at 09:12:24PM +0100, Borislav Petkov wrote:
> > On Mon, Nov 07, 2016 at 10:55:24AM -0800, Luck, Tony wrote:
> > > I don't think that helps as much as you'd like it to help (at
> > > least on Intel). A broadcast machine check that finds the boot
> > > CPU has set CR4[MCE]=1 is still going to end up in reset if any
> > > other CPU still has CR4[MCE]=0
> > 
> > By leaving/moving the setting of CR4 earlier on all cores, we'll
> > at least make the possible window for such potential resets a lot
> > smaller...
> 
> ... and in general, I'm still unsure about *why* we need this change for
> hotplug. bigeasy, can you please clarify first?

I want to get rid of non-symmetrical part and the arch hook which should
be part of the hp notifier itself. I wouldn't be too much afraid about
the when point in time when the notifier runs: It is the *first*
notifier that will be invoked on the target CPU. This is only a few
lines after the old hook. Nothing else long delaying should be invoked.

> Thanks.

Sebastian

[toc] | [prev] | [next] | [standalone]

#1518265

On Wed, Nov 09, 2016 at 03:22:21PM +0100, Sebastian Andrzej Siewior wrote:
> I want to get rid of non-symmetrical part and the arch hook which should
> be part of the hp notifier itself. I wouldn't be too much afraid about
> the when point in time when the notifier runs: It is the *first*
> notifier that will be invoked on the target CPU. This is only a few
> lines after the old hook. Nothing else long delaying should be invoked.

So I'm not sure *everything* mcheck_cpu_init() does should be in hotplug
and toggle on and off. For example, we should set CR4.MCE once during
init and that's it. Not turn it on and off. It would be pretty pointless
IMO.

That's why the hotplug callback mce_disable_cpu() doesn't fiddle with
CR4 - it only clears the bits in MCi_CTL. And I think we should remain
that way.

The initial, one-time enabling of MCE features per core should be done
only once per boot and that's it.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1518286

On 2016-11-09 16:38:07 [+0100], Borislav Petkov wrote:
> On Wed, Nov 09, 2016 at 03:22:21PM +0100, Sebastian Andrzej Siewior wrote:
> > I want to get rid of non-symmetrical part and the arch hook which should
> > be part of the hp notifier itself. I wouldn't be too much afraid about
> > the when point in time when the notifier runs: It is the *first*
> > notifier that will be invoked on the target CPU. This is only a few
> > lines after the old hook. Nothing else long delaying should be invoked.
> 
> So I'm not sure *everything* mcheck_cpu_init() does should be in hotplug
> and toggle on and off. For example, we should set CR4.MCE once during
> init and that's it. Not turn it on and off. It would be pretty pointless
> IMO.
> 
> That's why the hotplug callback mce_disable_cpu() doesn't fiddle with
> CR4 - it only clears the bits in MCi_CTL. And I think we should remain
> that way.

The behaviour was not changed - it was only reorganized to keep in one
spot.

> The initial, one-time enabling of MCE features per core should be done
> only once per boot and that's it.

It is. You still want this in your hotplug code in case someone replaces
(physically) your CPU (say a larger NUMA box).

Sebastian

[toc] | [prev] | [next] | [standalone]

#1518313

On Wed, Nov 09, 2016 at 05:24:51PM +0100, Sebastian Andrzej Siewior wrote:
> The behaviour was not changed - it was only reorganized to keep in one
> spot.

So there's the full CPU init path down cpu_up() -> ... -> identify_cpu()
where mcheck_cpu_init() is called and then there's also the hotplug
callbacks in mce_cpu_callback().

What you're proposing now is, merge the two.

But then the full path down identify_cpu() could still do
mheck_cpu_init() regardless where you move it.

IOW, I still don't see why this change is needed.

In more OW, why can't you simply do:

	err = cpuhp_setup_state(CPUHP_AP_X86_MCE_STARTING, "x86/mce:starting",
				mce_reenable_cpu, NULL);

and use the current notifier callback?

I still don't get the need for this churn.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1518331

On 2016-11-09 18:01:09 [+0100], Borislav Petkov wrote:
> On Wed, Nov 09, 2016 at 05:24:51PM +0100, Sebastian Andrzej Siewior wrote:
> > The behaviour was not changed - it was only reorganized to keep in one
> > spot.
> 
> So there's the full CPU init path down cpu_up() -> ... -> identify_cpu()
> where mcheck_cpu_init() is called and then there's also the hotplug
> callbacks in mce_cpu_callback().

correct. And it looks like mcheck_cpu_init() is part of the notifier. It
tried to allocate memory which it only did on boot CPU.

> What you're proposing now is, merge the two.

I wouldn't call it a merge. identify_cpu() is one of the first things a
CPU does on its boot. Slightly later invokes the `old' CPU_STARTING
notifier callbacks. I had nothing what would match CPU_STARTING except
mcheck_cpu_init() so I took it and renamed to mcheck_cpu_starting().
The remaining bring up states in mce_cpu_callback() are just CPU_ONLINE
which is also converted away.

> But then the full path down identify_cpu() could still do
> mheck_cpu_init() regardless where you move it.
> 
> IOW, I still don't see why this change is needed.

From what it looks, is that mce_cpu_down_dying() is the counter part to
mcheck_cpu_starting()/mcheck_cpu_init(). One hint for it is that the
latter disable the timer which is started by the former. So if you
rollback to online after the DYNING state you need to do STARTING in
order to start the timer.

> In more OW, why can't you simply do:
> 
> 	err = cpuhp_setup_state(CPUHP_AP_X86_MCE_STARTING, "x86/mce:starting",
> 				mce_reenable_cpu, NULL);

A later patch does:
         err = cpuhp_setup_state(CPUHP_AP_X86_MCE_STARTING, "x86/mce:starting",
                                 mcheck_cpu_starting, mce_cpu_down_dying);

> and us> e the current notifier callback?
> 
> I still don't get the need for this churn.
I tried to clarify it with the timer. You claimed that the MCE feature
itself is not disabled if the CPU goes down. I assumed that this is the
case (somewhere in mce_disable_cpu()) because a CPU which is off should
not response to any MCE errors and then it would be enabled in the
STARTING callback again. This (and timer) was the motivation to get the
MCE stuff in one place and get symmetrical CPU down/up behaviour.

Sebastian

[toc] | [prev] | [next] | [standalone]

#1518404

> That's why the hotplug callback mce_disable_cpu() doesn't fiddle with
> CR4 - it only clears the bits in MCi_CTL. And I think we should remain
> that way.

N.B. See vendor_disable_error_reporting() ... on Intel we don't clear MCi_CTL.

-Tony

[toc] | [prev] | [next] | [standalone]

#1518757

On 2016-11-09 18:37:23 [+0000], Luck, Tony wrote:
> > That's why the hotplug callback mce_disable_cpu() doesn't fiddle with
> > CR4 - it only clears the bits in MCi_CTL. And I think we should remain
> > that way.
> 
> N.B. See vendor_disable_error_reporting() ... on Intel we don't clear MCi_CTL.

okay. So I had a discussion with Boris and we agreed that we don't do
the init part in one place only the timer stuff will be moved.
If I understand Tony correctly then we undo something in
vendor_disable_error_reporting() (on !INTEL CPUs) that we enable in
identify_cpu().

Boris, if my understanding is correct, what now? Do move the whole
hotplug init part to an earlier stage? Or do we drop
vendor_disable_error_reporting()?

> -Tony

Sebastian

[toc] | [prev] | [next] | [standalone]

#1518770

On Thu, Nov 10, 2016 at 10:00:38AM +0100, Sebastian Andrzej Siewior wrote:
> okay. So I had a discussion with Boris and we agreed that we don't do
> the init part in one place only the timer stuff will be moved.
> If I understand Tony correctly then we undo something in
> vendor_disable_error_reporting() (on !INTEL CPUs) that we enable in
> identify_cpu().
> 
> Boris, if my understanding is correct, what now? Do move the whole
> hotplug init part to an earlier stage? Or do we drop
> vendor_disable_error_reporting()?

Nah, we proceed as we discussed it - let's move the timer only. The
mce_gen_pool_init() allocation is not where it should be - it should
run somewhere only once on CPU0 but I haven't found a better place yet
so that the allocation succeeds - mind you, this is very early and
gen_pool_create() uses kmalloc_node() which might not be functioning
yet...

Tony's point is showing that the hotplug notifiers don't do any
bit clearing on Intel, i.e., basically we don't do anything on the
CPU_DOWN_PREPARE path on Intel.

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1519225 — [PATCH 4/7] x86/mcheck: Split threshold_cpu_callback into two callbacks

The threshold_cpu_callback callbacks looks like one of the notifier and
its arguments are almost the same. Split this out and have one ONLINE
and one DEAD callback. This will come handy later once the main code
gets changed to use the callback mechanism.
Also, handle threshold_cpu_callback_online() return value so we don't
continue if the function fails.

Boris Petkov removed the callback pointer and replaced it with proper
functions.

Cc: Tony Luck <tony.luck@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: linux-edac@vger.kernel.org
Cc: x86@kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/mce.h           |  7 +++++--
 arch/x86/kernel/cpu/mcheck/mce.c     | 14 +++++++------
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 38 ++++++++++++++----------------------
 3 files changed, 28 insertions(+), 31 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 9bd7ff5ffbcc..caafad4a211e 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -293,9 +293,7 @@ void do_machine_check(struct pt_regs *, long);
 /*
  * Threshold handler
  */
-
 extern void (*mce_threshold_vector)(void);
-extern void (*threshold_cpu_callback)(unsigned long action, unsigned int cpu);
 
 /* Deferred error interrupt handler */
 extern void (*deferred_error_int_vector)(void);
@@ -377,7 +375,12 @@ struct smca_bank_info {
 };
 
 extern struct smca_bank_info smca_banks[MAX_NR_BANKS];
+extern int mce_threshold_create_device(unsigned int cpu);
+extern int mce_threshold_remove_device(unsigned int cpu);
+#else
 
+static inline int mce_threshold_create_device(unsigned int cpu) { return 0; };
+static inline int mce_threshold_remove_device(unsigned int cpu) { return 0; };
 #endif
 
 #endif /* _ASM_X86_MCE_H */
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index e9ffd6d9e32d..052b5e05c3c4 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -2255,8 +2255,6 @@ static struct bus_type mce_subsys = {
 
 DEFINE_PER_CPU(struct device *, mce_device);
 
-void (*threshold_cpu_callback)(unsigned long action, unsigned int cpu);
-
 static inline struct mce_bank *attr_to_bank(struct device_attribute *attr)
 {
 	return container_of(attr, struct mce_bank, attr);
@@ -2512,13 +2510,17 @@ mce_cpu_callback(struct notifier_block *nfb, unsigned long action, void *hcpu)
 
 	switch (action & ~CPU_TASKS_FROZEN) {
 	case CPU_ONLINE:
+
 		mce_device_create(cpu);
-		if (threshold_cpu_callback)
-			threshold_cpu_callback(action, cpu);
+
+		if (mce_threshold_create_device(cpu)) {
+			mce_device_remove(cpu);
+			return NOTIFY_BAD;
+		}
+
 		break;
 	case CPU_DEAD:
-		if (threshold_cpu_callback)
-			threshold_cpu_callback(action, cpu);
+		mce_threshold_remove_device(cpu);
 		mce_device_remove(cpu);
 		mce_intel_hcpu_update(cpu);
 
diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index e93580c82ef0..c33a3ee2e383 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -55,6 +55,8 @@
 /* Threshold LVT offset is at MSR0xC0000410[15:12] */
 #define SMCA_THR_LVT_OFF	0xF000
 
+static bool thresholding_en;
+
 static const char * const th_names[] = {
 	"load_store",
 	"insn_fetch",
@@ -1077,10 +1079,13 @@ static void threshold_remove_bank(unsigned int cpu, int bank)
 	per_cpu(threshold_banks, cpu)[bank] = NULL;
 }
 
-static void threshold_remove_device(unsigned int cpu)
+int mce_threshold_remove_device(unsigned int cpu)
 {
 	unsigned int bank;
 
+	if (!thresholding_en)
+		return 0;
+
 	for (bank = 0; bank < mca_cfg.banks; ++bank) {
 		if (!(per_cpu(bank_map, cpu) & (1 << bank)))
 			continue;
@@ -1088,15 +1093,19 @@ static void threshold_remove_device(unsigned int cpu)
 	}
 	kfree(per_cpu(threshold_banks, cpu));
 	per_cpu(threshold_banks, cpu) = NULL;
+	return 0;
 }
 
 /* create dir/files for all valid threshold banks */
-static int threshold_create_device(unsigned int cpu)
+int mce_threshold_create_device(unsigned int cpu)
 {
 	unsigned int bank;
 	struct threshold_bank **bp;
 	int err = 0;
 
+	if (!thresholding_en)
+		return 0;
+
 	bp = per_cpu(threshold_banks, cpu);
 	if (bp)
 		return 0;
@@ -1117,40 +1126,23 @@ static int threshold_create_device(unsigned int cpu)
 	}
 	return err;
 err:
-	threshold_remove_device(cpu);
+	mce_threshold_remove_device(cpu);
 	return err;
 }
 
-/* get notified when a cpu comes on/off */
-static void
-amd_64_threshold_cpu_callback(unsigned long action, unsigned int cpu)
-{
-	switch (action) {
-	case CPU_ONLINE:
-	case CPU_ONLINE_FROZEN:
-		threshold_create_device(cpu);
-		break;
-	case CPU_DEAD:
-	case CPU_DEAD_FROZEN:
-		threshold_remove_device(cpu);
-		break;
-	default:
-		break;
-	}
-}
-
 static __init int threshold_init_device(void)
 {
 	unsigned lcpu = 0;
 
 	/* to hit CPUs online before the notifier is up */
 	for_each_online_cpu(lcpu) {
-		int err = threshold_create_device(lcpu);
+		int err = mce_threshold_create_device(lcpu);
 
 		if (err)
 			return err;
 	}
-	threshold_cpu_callback = amd_64_threshold_cpu_callback;
+
+	thresholding_en = true;
 
 	return 0;
 }
-- 
2.10.2

[toc] | [prev] | [next] | [standalone]

#1519227 — [PATCH 3/7] x86/mcheck: Be prepared for a rollback back to the ONLINE state

If we try a CPU down and fail in the middle then we roll back to the
online state. This means we would perform CPU_ONLINE / mce_device_create()
without invoking CPU_DEAD / mce_device_remove() for the cleanup of what was
allocated in CPU_ONLINE.
Be prepared for this and don't allocate the struct if we have it
already.

Cc: Tony Luck <tony.luck@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: linux-edac@vger.kernel.org
Cc: x86@kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/cpu/mcheck/mce.c     | 4 ++++
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index a7fdf453d895..e9ffd6d9e32d 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -2409,6 +2409,10 @@ static int mce_device_create(unsigned int cpu)
 	if (!mce_available(&boot_cpu_data))
 		return -EIO;
 
+	dev = per_cpu(mce_device, cpu);
+	if (dev)
+		return 0;
+
 	dev = kzalloc(sizeof *dev, GFP_KERNEL);
 	if (!dev)
 		return -ENOMEM;
diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index 55cd018bc1ae..e93580c82ef0 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -1097,6 +1097,10 @@ static int threshold_create_device(unsigned int cpu)
 	struct threshold_bank **bp;
 	int err = 0;
 
+	bp = per_cpu(threshold_banks, cpu);
+	if (bp)
+		return 0;
+
 	bp = kzalloc(sizeof(struct threshold_bank *) * mca_cfg.banks,
 		     GFP_KERNEL);
 	if (!bp)
-- 
2.10.2

[toc] | [prev] | [next] | [standalone]

#1519228 — [PATCH 1/7] x86/mcheck: Move threshold_create_device()

Move the threshold_create_device() so it can use
threshold_remove_device() without a forward declaration.

Cc: Tony Luck <tony.luck@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: linux-edac@vger.kernel.org
Cc: x86@kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 50 ++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index 9b5403462936..75a3e3eab81e 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -1010,31 +1010,6 @@ static int threshold_create_bank(unsigned int cpu, unsigned int bank)
 	return err;
 }
 
-/* create dir/files for all valid threshold banks */
-static int threshold_create_device(unsigned int cpu)
-{
-	unsigned int bank;
-	struct threshold_bank **bp;
-	int err = 0;
-
-	bp = kzalloc(sizeof(struct threshold_bank *) * mca_cfg.banks,
-		     GFP_KERNEL);
-	if (!bp)
-		return -ENOMEM;
-
-	per_cpu(threshold_banks, cpu) = bp;
-
-	for (bank = 0; bank < mca_cfg.banks; ++bank) {
-		if (!(per_cpu(bank_map, cpu) & (1 << bank)))
-			continue;
-		err = threshold_create_bank(cpu, bank);
-		if (err)
-			return err;
-	}
-
-	return err;
-}
-
 static void deallocate_threshold_block(unsigned int cpu,
 						 unsigned int bank)
 {
@@ -1114,6 +1089,31 @@ static void threshold_remove_device(unsigned int cpu)
 	kfree(per_cpu(threshold_banks, cpu));
 }
 
+/* create dir/files for all valid threshold banks */
+static int threshold_create_device(unsigned int cpu)
+{
+	unsigned int bank;
+	struct threshold_bank **bp;
+	int err = 0;
+
+	bp = kzalloc(sizeof(struct threshold_bank *) * mca_cfg.banks,
+		     GFP_KERNEL);
+	if (!bp)
+		return -ENOMEM;
+
+	per_cpu(threshold_banks, cpu) = bp;
+
+	for (bank = 0; bank < mca_cfg.banks; ++bank) {
+		if (!(per_cpu(bank_map, cpu) & (1 << bank)))
+			continue;
+		err = threshold_create_bank(cpu, bank);
+		if (err)
+			return err;
+	}
+
+	return err;
+}
+
 /* get notified when a cpu comes on/off */
 static void
 amd_64_threshold_cpu_callback(unsigned long action, unsigned int cpu)
-- 
2.10.2

[toc] | [prev] | [next] | [standalone]

#1519235 — [PATCH 5/7] x86/mcheck: reorganize the hotplug callbacks

Initially I wanted to remove mcheck_cpu_init() from identify_cpu() and let it
become an independent early hotplug callback. The main problem here was that
the init on the boot CPU may happen too late
(device_initcall_sync(mcheck_init_device)) and nobody wanted to risk receiving
and MCE event at boot time leading to a shutdown (if the MCE feature is not yet
enabled).

Here is attempt two: the timming stays as-is but the ordering of the functions
is changed:
- mcheck_cpu_init() (which is run from identify_cpu()) will setup the timer
  struct but won't fire the timer. This is moved to CPU_ONLINE since its
  cleanup part is in CPU_DOWN_PREPARE. So if it is okay to stop the timer early
  in the shutdown phase, it should be okay to start it late in the bring up phase.

- CPU_DOWN_PREPARE disables the MCE feature flags for !INTEL CPUs in
  mce_disable_cpu(). If a failure occures it would be re-enabled on all vendor
  CPUs (including Intel where it was not disabled during shutdown). To keep this
  working I am moving it to CPU_ONLINE. smp_call_function_single() is dropped
  beause the notifier runs nowdays on the target CPU.

- CPU_ONLINE is invoking mce_device_create() + mce_threshold_create_device()
  but its cleanup part is in CPU_DEAD (mce_threshold_remove_device() and
  mce_device_remove()). In order to keep this symmetrical I am moving the clean
  up from CPU_DEAD to CPU_DOWN_PREPARE.

Cc: Tony Luck <tony.luck@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: linux-edac@vger.kernel.org
Cc: x86@kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/x86/kernel/cpu/mcheck/mce.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 052b5e05c3c4..3da6fd94fa2e 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1771,6 +1771,9 @@ void (*machine_check_vector)(struct pt_regs *, long error_code) =
  */
 void mcheck_cpu_init(struct cpuinfo_x86 *c)
 {
+	struct timer_list *t = this_cpu_ptr(&mce_timer);
+	unsigned int cpu = smp_processor_id();
+
 	if (mca_cfg.disabled)
 		return;
 
@@ -1796,7 +1799,7 @@ void mcheck_cpu_init(struct cpuinfo_x86 *c)
 	__mcheck_cpu_init_generic();
 	__mcheck_cpu_init_vendor(c);
 	__mcheck_cpu_init_clear_banks();
-	__mcheck_cpu_init_timer();
+	setup_pinned_timer(t, mce_timer_fn, cpu);
 }
 
 /*
@@ -2470,28 +2473,25 @@ static void mce_device_remove(unsigned int cpu)
 }
 
 /* Make sure there are no machine checks on offlined CPUs. */
-static void mce_disable_cpu(void *h)
+static void mce_disable_cpu(void)
 {
-	unsigned long action = *(unsigned long *)h;
-
 	if (!mce_available(raw_cpu_ptr(&cpu_info)))
 		return;
 
-	if (!(action & CPU_TASKS_FROZEN))
+	if (!cpuhp_tasks_frozen)
 		cmci_clear();
 
 	vendor_disable_error_reporting();
 }
 
-static void mce_reenable_cpu(void *h)
+static void mce_reenable_cpu(void)
 {
-	unsigned long action = *(unsigned long *)h;
 	int i;
 
 	if (!mce_available(raw_cpu_ptr(&cpu_info)))
 		return;
 
-	if (!(action & CPU_TASKS_FROZEN))
+	if (!cpuhp_tasks_frozen)
 		cmci_reenable();
 	for (i = 0; i < mca_cfg.banks; i++) {
 		struct mce_bank *b = &mce_banks[i];
@@ -2510,6 +2510,7 @@ mce_cpu_callback(struct notifier_block *nfb, unsigned long action, void *hcpu)
 
 	switch (action & ~CPU_TASKS_FROZEN) {
 	case CPU_ONLINE:
+	case CPU_DOWN_FAILED:
 
 		mce_device_create(cpu);
 
@@ -2517,11 +2518,10 @@ mce_cpu_callback(struct notifier_block *nfb, unsigned long action, void *hcpu)
 			mce_device_remove(cpu);
 			return NOTIFY_BAD;
 		}
-
+		mce_reenable_cpu();
+		mce_start_timer(cpu, t);
 		break;
 	case CPU_DEAD:
-		mce_threshold_remove_device(cpu);
-		mce_device_remove(cpu);
 		mce_intel_hcpu_update(cpu);
 
 		/* intentionally ignoring frozen here */
@@ -2529,12 +2529,11 @@ mce_cpu_callback(struct notifier_block *nfb, unsigned long action, void *hcpu)
 			cmci_rediscover();
 		break;
 	case CPU_DOWN_PREPARE:
-		smp_call_function_single(cpu, mce_disable_cpu, &action, 1);
+		mce_disable_cpu();
 		del_timer_sync(t);
-		break;
-	case CPU_DOWN_FAILED:
-		smp_call_function_single(cpu, mce_reenable_cpu, &action, 1);
-		mce_start_timer(cpu, t);
+
+		mce_threshold_remove_device(cpu);
+		mce_device_remove(cpu);
 		break;
 	}
 
-- 
2.10.2

[toc] | [prev] | [next] | [standalone]

#1519967 — Re: [PATCH 5/7] x86/mcheck: reorganize the hotplug callbacks

On Thu, Nov 10, 2016 at 06:44:45PM +0100, Sebastian Andrzej Siewior wrote:
> Initially I wanted to remove mcheck_cpu_init() from identify_cpu() and let it
> become an independent early hotplug callback. The main problem here was that
> the init on the boot CPU may happen too late
> (device_initcall_sync(mcheck_init_device)) and nobody wanted to risk receiving
> and MCE event at boot time leading to a shutdown (if the MCE feature is not yet
> enabled).
> 
> Here is attempt two: the timming stays as-is but the ordering of the functions

			   timing

> is changed:
> - mcheck_cpu_init() (which is run from identify_cpu()) will setup the timer
>   struct but won't fire the timer. This is moved to CPU_ONLINE since its
>   cleanup part is in CPU_DOWN_PREPARE. So if it is okay to stop the timer early
>   in the shutdown phase, it should be okay to start it late in the bring up phase.
> 
> - CPU_DOWN_PREPARE disables the MCE feature flags for !INTEL CPUs in

		     disables the MCE error reporting...

>   mce_disable_cpu(). If a failure occures it would be re-enabled on all vendor

				    occurs

>   CPUs (including Intel where it was not disabled during shutdown). To keep this
>   working I am moving it to CPU_ONLINE. smp_call_function_single() is dropped
>   beause the notifier runs nowdays on the target CPU.

"... because the notifier runs on the target CPU now."

Please run your commit messages text through a spellchecker.

> - CPU_ONLINE is invoking mce_device_create() + mce_threshold_create_device()
>   but its cleanup part is in CPU_DEAD (mce_threshold_remove_device() and
>   mce_device_remove()). In order to keep this symmetrical I am moving the clean
>   up from CPU_DEAD to CPU_DOWN_PREPARE.

   cleanup

> 
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: linux-edac@vger.kernel.org
> Cc: x86@kernel.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  arch/x86/kernel/cpu/mcheck/mce.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 052b5e05c3c4..3da6fd94fa2e 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -1771,6 +1771,9 @@ void (*machine_check_vector)(struct pt_regs *, long error_code) =
>   */
>  void mcheck_cpu_init(struct cpuinfo_x86 *c)
>  {
> +	struct timer_list *t = this_cpu_ptr(&mce_timer);
> +	unsigned int cpu = smp_processor_id();
> +
>  	if (mca_cfg.disabled)
>  		return;
>  
> @@ -1796,7 +1799,7 @@ void mcheck_cpu_init(struct cpuinfo_x86 *c)
>  	__mcheck_cpu_init_generic();
>  	__mcheck_cpu_init_vendor(c);
>  	__mcheck_cpu_init_clear_banks();
> -	__mcheck_cpu_init_timer();
> +	setup_pinned_timer(t, mce_timer_fn, cpu);

Why not leave all that setup stuff in __mcheck_cpu_init_timer() ?

...

> @@ -2517,11 +2518,10 @@ mce_cpu_callback(struct notifier_block *nfb, unsigned long action, void *hcpu)
>  			mce_device_remove(cpu);
>  			return NOTIFY_BAD;
>  		}
> -
> +		mce_reenable_cpu();
> +		mce_start_timer(cpu, t);
>  		break;
>  	case CPU_DEAD:
> -		mce_threshold_remove_device(cpu);
> -		mce_device_remove(cpu);
>  		mce_intel_hcpu_update(cpu);
>  
>  		/* intentionally ignoring frozen here */

There's another place for cpuhp_tasks_frozen replacement here:

                /* intentionally ignoring frozen here */
                if (!(action & CPU_TASKS_FROZEN))
                        cmci_rediscover();

into

		if (!cpuhp_tasks_frozen)
			cmci_rediscover();

> @@ -2529,12 +2529,11 @@ mce_cpu_callback(struct notifier_block *nfb, unsigned long action, void *hcpu)
>  			cmci_rediscover();
>  		break;
>  	case CPU_DOWN_PREPARE:
> -		smp_call_function_single(cpu, mce_disable_cpu, &action, 1);
> +		mce_disable_cpu();
>  		del_timer_sync(t);
> -		break;
> -	case CPU_DOWN_FAILED:
> -		smp_call_function_single(cpu, mce_reenable_cpu, &action, 1);
> -		mce_start_timer(cpu, t);
> +
> +		mce_threshold_remove_device(cpu);
> +		mce_device_remove(cpu);
>  		break;
>  	}
>  
> -- 
> 2.10.2
> 
> 

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1519990 — Re: [PATCH 5/7] x86/mcheck: reorganize the hotplug callbacks

On 2016-11-11 19:44:53 [+0100], Borislav Petkov wrote:
> > ---
> >  arch/x86/kernel/cpu/mcheck/mce.c | 31 +++++++++++++++----------------
> >  1 file changed, 15 insertions(+), 16 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> > index 052b5e05c3c4..3da6fd94fa2e 100644
> > --- a/arch/x86/kernel/cpu/mcheck/mce.c
> > +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> > @@ -1771,6 +1771,9 @@ void (*machine_check_vector)(struct pt_regs *, long error_code) =
> >   */
> >  void mcheck_cpu_init(struct cpuinfo_x86 *c)
> >  {
> > +	struct timer_list *t = this_cpu_ptr(&mce_timer);
> > +	unsigned int cpu = smp_processor_id();
> > +
> >  	if (mca_cfg.disabled)
> >  		return;
> >  
> > @@ -1796,7 +1799,7 @@ void mcheck_cpu_init(struct cpuinfo_x86 *c)
> >  	__mcheck_cpu_init_generic();
> >  	__mcheck_cpu_init_vendor(c);
> >  	__mcheck_cpu_init_clear_banks();
> > -	__mcheck_cpu_init_timer();
> > +	setup_pinned_timer(t, mce_timer_fn, cpu);
> 
> Why not leave all that setup stuff in __mcheck_cpu_init_timer() ?
> 
__mcheck_cpu_init_timer() does not only prepare the timer but also fires
it. And __mcheck_cpu_init_timer() is invoked from other places and the
caller probably expects the timer to be fired.

> > @@ -2517,11 +2518,10 @@ mce_cpu_callback(struct notifier_block *nfb, unsigned long action, void *hcpu)
> >  			mce_device_remove(cpu);
> >  			return NOTIFY_BAD;
> >  		}
> > -
> > +		mce_reenable_cpu();
> > +		mce_start_timer(cpu, t);
> >  		break;
> >  	case CPU_DEAD:
> > -		mce_threshold_remove_device(cpu);
> > -		mce_device_remove(cpu);
> >  		mce_intel_hcpu_update(cpu);
> >  
> >  		/* intentionally ignoring frozen here */
> 
> There's another place for cpuhp_tasks_frozen replacement here:
> 
>                 /* intentionally ignoring frozen here */
>                 if (!(action & CPU_TASKS_FROZEN))
>                         cmci_rediscover();
> 
> into
> 
> 		if (!cpuhp_tasks_frozen)
> 			cmci_rediscover();

Correct. This is done later since it still works. For functions like
mce_disable_cpu() which were converted here I replaced the check so I
keep them void and don't care about the argument.

Sebastian

[toc] | [prev] | [next] | [standalone]

#1519999 — Re: [PATCH 5/7] x86/mcheck: reorganize the hotplug callbacks

On Fri, Nov 11, 2016 at 08:36:31PM +0100, Sebastian Andrzej Siewior wrote:
> __mcheck_cpu_init_timer() does not only prepare the timer but also fires
> it. And __mcheck_cpu_init_timer() is invoked from other places and the
> caller probably expects the timer to be fired.

Then please do it right:

static void __mcheck_cap_setup_timer(void)
{
        struct timer_list *t = this_cpu_ptr(&mce_timer);
        unsigned int cpu = smp_processor_id();

        setup_pinned_timer(t, mce_timer_fn, cpu);
}

and call that function then.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web