| Message ID | 20170406045537.1573-2-haozhong.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 06.04.17 at 06:55, <haozhong.zhang@intel.com> wrote: > 1. Distinguish 'severity_cpu' used in mcheck_cmn_handler() and > mce_softirq(), which should be different variables. Otherwise, they > may interfere with each other if MC# comes during mce_softirq(). > 2. Always (re-)initialize 'severity_cpu' to clear historical information. I agree with this for the mcheck_cmn_handler() case, but ... > @@ -1704,11 +1703,16 @@ static int mce_delayed_action(mctelem_cookie_t mctc) > /* Softirq Handler for this MCE# processing */ > static void mce_softirq(void) > { > + static atomic_t severity_cpu; > int cpu = smp_processor_id(); > unsigned int workcpu; > > mce_printk(MCE_VERBOSE, "CPU%d enter softirq\n", cpu); > > + mce_barrier_enter(&mce_softirq_init_bar); > + atomic_set(&severity_cpu, -1); > + mce_barrier_exit(&mce_softirq_init_bar); ... I don't think this is needed, as right after the following comment it'll be set unconditionally. > @@ -1774,6 +1778,7 @@ void mce_handler_init(void) > mce_barrier_init(&mce_severity_bar); > mce_barrier_init(&mce_trap_bar); > mce_barrier_init(&mce_handler_init_bar); > + mce_barrier_init(&mce_softirq_init_bar); Just like the variables you move, all these mce_*_bar ones are really private to their respective functions. I've taken a not to put together a patch to move the pre-existing ones, but please don't introduce any new ones with file scope. Jan
On 04/06/17 02:06 -0600, Jan Beulich wrote: > >>> On 06.04.17 at 06:55, <haozhong.zhang@intel.com> wrote: > > 1. Distinguish 'severity_cpu' used in mcheck_cmn_handler() and > > mce_softirq(), which should be different variables. Otherwise, they > > may interfere with each other if MC# comes during mce_softirq(). > > 2. Always (re-)initialize 'severity_cpu' to clear historical information. > > I agree with this for the mcheck_cmn_handler() case, but ... > > > @@ -1704,11 +1703,16 @@ static int mce_delayed_action(mctelem_cookie_t mctc) > > /* Softirq Handler for this MCE# processing */ > > static void mce_softirq(void) > > { > > + static atomic_t severity_cpu; > > int cpu = smp_processor_id(); > > unsigned int workcpu; > > > > mce_printk(MCE_VERBOSE, "CPU%d enter softirq\n", cpu); > > > > + mce_barrier_enter(&mce_softirq_init_bar); > > + atomic_set(&severity_cpu, -1); > > + mce_barrier_exit(&mce_softirq_init_bar); > > ... I don't think this is needed, as right after the following comment > it'll be set unconditionally. > > > @@ -1774,6 +1778,7 @@ void mce_handler_init(void) > > mce_barrier_init(&mce_severity_bar); > > mce_barrier_init(&mce_trap_bar); > > mce_barrier_init(&mce_handler_init_bar); > > + mce_barrier_init(&mce_softirq_init_bar); > > Just like the variables you move, all these mce_*_bar ones are > really private to their respective functions. I've taken a not to > put together a patch to move the pre-existing ones, but please > don't introduce any new ones with file scope. > How is about introducing a static initializer for mce barrier so that I can initialize static ones at their declaration? Or is the static initializer completely not needed because all fields of mce barrier are currently initialized to 0? Thanks, Haozhong
>>> On 06.04.17 at 10:49, <haozhong.zhang@intel.com> wrote: > On 04/06/17 02:06 -0600, Jan Beulich wrote: >> >>> On 06.04.17 at 06:55, <haozhong.zhang@intel.com> wrote: >> > @@ -1774,6 +1778,7 @@ void mce_handler_init(void) >> > mce_barrier_init(&mce_severity_bar); >> > mce_barrier_init(&mce_trap_bar); >> > mce_barrier_init(&mce_handler_init_bar); >> > + mce_barrier_init(&mce_softirq_init_bar); >> >> Just like the variables you move, all these mce_*_bar ones are >> really private to their respective functions. I've taken a not to >> put together a patch to move the pre-existing ones, but please >> don't introduce any new ones with file scope. > > How is about introducing a static initializer for mce barrier so that > I can initialize static ones at their declaration? Or is the static > initializer completely not needed because all fields of mce barrier > are currently initialized to 0? Yes, there should be a static initializer introduced for that purpose, despite all currently present fields wanting to simply be zero. Jan
diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c index 7618120..d9ab80a 100644 --- a/xen/arch/x86/cpu/mcheck/mce.c +++ b/xen/arch/x86/cpu/mcheck/mce.c @@ -177,7 +177,7 @@ void mce_need_clearbank_register(mce_need_clearbank_t cbfunc) static struct mce_softirq_barrier mce_inside_bar, mce_severity_bar; static struct mce_softirq_barrier mce_trap_bar; -static struct mce_softirq_barrier mce_handler_init_bar; +static struct mce_softirq_barrier mce_handler_init_bar, mce_softirq_init_bar; /* * mce_logout_lock should only be used in the trap handler, @@ -187,8 +187,6 @@ static struct mce_softirq_barrier mce_handler_init_bar; */ static DEFINE_SPINLOCK(mce_logout_lock); -static atomic_t severity_cpu = ATOMIC_INIT(-1); - const struct mca_error_handler *__read_mostly mce_dhandlers; const struct mca_error_handler *__read_mostly mce_uhandlers; unsigned int __read_mostly mce_dhandler_num; @@ -452,7 +450,7 @@ static int mce_urgent_action(const struct cpu_user_regs *regs, /* Shared #MC handler. */ void mcheck_cmn_handler(const struct cpu_user_regs *regs) { - static atomic_t found_error; + static atomic_t severity_cpu, found_error; static cpumask_t mce_fatal_cpus; struct mca_banks *bankmask = mca_allbanks; struct mca_banks *clear_bank = __get_cpu_var(mce_clear_banks); @@ -461,6 +459,7 @@ void mcheck_cmn_handler(const struct cpu_user_regs *regs) struct mca_summary bs; mce_barrier_enter(&mce_handler_init_bar); + atomic_set(&severity_cpu, -1); atomic_set(&found_error, 0); cpumask_clear(&mce_fatal_cpus); mce_barrier_exit(&mce_handler_init_bar); @@ -1704,11 +1703,16 @@ static int mce_delayed_action(mctelem_cookie_t mctc) /* Softirq Handler for this MCE# processing */ static void mce_softirq(void) { + static atomic_t severity_cpu; int cpu = smp_processor_id(); unsigned int workcpu; mce_printk(MCE_VERBOSE, "CPU%d enter softirq\n", cpu); + mce_barrier_enter(&mce_softirq_init_bar); + atomic_set(&severity_cpu, -1); + mce_barrier_exit(&mce_softirq_init_bar); + mce_barrier_enter(&mce_inside_bar); /* @@ -1774,6 +1778,7 @@ void mce_handler_init(void) mce_barrier_init(&mce_severity_bar); mce_barrier_init(&mce_trap_bar); mce_barrier_init(&mce_handler_init_bar); + mce_barrier_init(&mce_softirq_init_bar); spin_lock_init(&mce_logout_lock); open_softirq(MACHINE_CHECK_SOFTIRQ, mce_softirq); }
1. Distinguish 'severity_cpu' used in mcheck_cmn_handler() and mce_softirq(), which should be different variables. Otherwise, they may interfere with each other if MC# comes during mce_softirq(). 2. Always (re-)initialize 'severity_cpu' to clear historical information. Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> --- xen/arch/x86/cpu/mcheck/mce.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)