* [PATCH 1/2] xen/mce: fix static variables 'found_error' and 'mce_fatal_cpus'
@ 2017-04-06 4:55 Haozhong Zhang
2017-04-06 4:55 ` [PATCH 2/2] xen/mce: fix static variable 'severity_cpu' Haozhong Zhang
2017-04-06 7:54 ` [PATCH 1/2] xen/mce: fix static variables 'found_error' and 'mce_fatal_cpus' Jan Beulich
0 siblings, 2 replies; 8+ messages in thread
From: Haozhong Zhang @ 2017-04-06 4:55 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Jan Beulich, Haozhong Zhang
1. Move them into mcheck_cmn_handler() which is their only user.
2. Always (re-)initialize them to clear historical information.
Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
xen/arch/x86/cpu/mcheck/mce.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index 11d0e23..7618120 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -177,6 +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;
/*
* mce_logout_lock should only be used in the trap handler,
@@ -187,8 +188,6 @@ static struct mce_softirq_barrier mce_trap_bar;
static DEFINE_SPINLOCK(mce_logout_lock);
static atomic_t severity_cpu = ATOMIC_INIT(-1);
-static atomic_t found_error = ATOMIC_INIT(0);
-static cpumask_t mce_fatal_cpus;
const struct mca_error_handler *__read_mostly mce_dhandlers;
const struct mca_error_handler *__read_mostly mce_uhandlers;
@@ -453,12 +452,19 @@ 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 cpumask_t mce_fatal_cpus;
struct mca_banks *bankmask = mca_allbanks;
struct mca_banks *clear_bank = __get_cpu_var(mce_clear_banks);
uint64_t gstatus;
mctelem_cookie_t mctc = NULL;
struct mca_summary bs;
+ mce_barrier_enter(&mce_handler_init_bar);
+ atomic_set(&found_error, 0);
+ cpumask_clear(&mce_fatal_cpus);
+ mce_barrier_exit(&mce_handler_init_bar);
+
mce_spin_lock(&mce_logout_lock);
if (clear_bank != NULL) {
@@ -1767,6 +1773,7 @@ void mce_handler_init(void)
mce_barrier_init(&mce_inside_bar);
mce_barrier_init(&mce_severity_bar);
mce_barrier_init(&mce_trap_bar);
+ mce_barrier_init(&mce_handler_init_bar);
spin_lock_init(&mce_logout_lock);
open_softirq(MACHINE_CHECK_SOFTIRQ, mce_softirq);
}
--
2.10.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] xen/mce: fix static variable 'severity_cpu'
2017-04-06 4:55 [PATCH 1/2] xen/mce: fix static variables 'found_error' and 'mce_fatal_cpus' Haozhong Zhang
@ 2017-04-06 4:55 ` Haozhong Zhang
2017-04-06 8:06 ` Jan Beulich
2017-04-06 7:54 ` [PATCH 1/2] xen/mce: fix static variables 'found_error' and 'mce_fatal_cpus' Jan Beulich
1 sibling, 1 reply; 8+ messages in thread
From: Haozhong Zhang @ 2017-04-06 4:55 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Jan Beulich, Haozhong Zhang
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(-)
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);
}
--
2.10.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] xen/mce: fix static variables 'found_error' and 'mce_fatal_cpus'
2017-04-06 4:55 [PATCH 1/2] xen/mce: fix static variables 'found_error' and 'mce_fatal_cpus' Haozhong Zhang
2017-04-06 4:55 ` [PATCH 2/2] xen/mce: fix static variable 'severity_cpu' Haozhong Zhang
@ 2017-04-06 7:54 ` Jan Beulich
2017-04-06 8:49 ` Haozhong Zhang
1 sibling, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2017-04-06 7:54 UTC (permalink / raw)
To: Haozhong Zhang; +Cc: Andrew Cooper, xen-devel
>>> On 06.04.17 at 06:55, <haozhong.zhang@intel.com> wrote:
> 1. Move them into mcheck_cmn_handler() which is their only user.
This part is uncontroversial.
> 2. Always (re-)initialize them to clear historical information.
But without further explanation I'm not convinced this part is correct.
That's a good indication that you should split patches.
I assume, btw, that you're aware that these patches won't go
in very soon (not until after 4.9 has been branched off), unless
they fix an actual bug.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] xen/mce: fix static variable 'severity_cpu'
2017-04-06 4:55 ` [PATCH 2/2] xen/mce: fix static variable 'severity_cpu' Haozhong Zhang
@ 2017-04-06 8:06 ` Jan Beulich
2017-04-06 8:49 ` Haozhong Zhang
0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2017-04-06 8:06 UTC (permalink / raw)
To: Haozhong Zhang; +Cc: Andrew Cooper, xen-devel
>>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] xen/mce: fix static variables 'found_error' and 'mce_fatal_cpus'
2017-04-06 7:54 ` [PATCH 1/2] xen/mce: fix static variables 'found_error' and 'mce_fatal_cpus' Jan Beulich
@ 2017-04-06 8:49 ` Haozhong Zhang
2017-04-06 9:17 ` Jan Beulich
0 siblings, 1 reply; 8+ messages in thread
From: Haozhong Zhang @ 2017-04-06 8:49 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, xen-devel
On 04/06/17 01:54 -0600, Jan Beulich wrote:
> >>> On 06.04.17 at 06:55, <haozhong.zhang@intel.com> wrote:
> > 1. Move them into mcheck_cmn_handler() which is their only user.
>
> This part is uncontroversial.
>
> > 2. Always (re-)initialize them to clear historical information.
>
> But without further explanation I'm not convinced this part is correct.
> That's a good indication that you should split patches.
>
> I assume, btw, that you're aware that these patches won't go
> in very soon (not until after 4.9 has been branched off), unless
> they fix an actual bug.
>
Please drop this patch as it does not fix any bug. I was blind.
'found_error' is already cleared after mcheck_cmn_handler() used it.
Non-empty 'mce_fatal_errors' results in mc_panic(), so there is no
need to clear it.
Sorry for the noise.
Haozhong
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] xen/mce: fix static variable 'severity_cpu'
2017-04-06 8:06 ` Jan Beulich
@ 2017-04-06 8:49 ` Haozhong Zhang
2017-04-06 9:18 ` Jan Beulich
0 siblings, 1 reply; 8+ messages in thread
From: Haozhong Zhang @ 2017-04-06 8:49 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, xen-devel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] xen/mce: fix static variables 'found_error' and 'mce_fatal_cpus'
2017-04-06 8:49 ` Haozhong Zhang
@ 2017-04-06 9:17 ` Jan Beulich
0 siblings, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2017-04-06 9:17 UTC (permalink / raw)
To: Haozhong Zhang; +Cc: Andrew Cooper, xen-devel
>>> On 06.04.17 at 10:49, <haozhong.zhang@intel.com> wrote:
> On 04/06/17 01:54 -0600, Jan Beulich wrote:
>> >>> On 06.04.17 at 06:55, <haozhong.zhang@intel.com> wrote:
>> > 1. Move them into mcheck_cmn_handler() which is their only user.
>>
>> This part is uncontroversial.
>>
>> > 2. Always (re-)initialize them to clear historical information.
>>
>> But without further explanation I'm not convinced this part is correct.
>> That's a good indication that you should split patches.
>>
>> I assume, btw, that you're aware that these patches won't go
>> in very soon (not until after 4.9 has been branched off), unless
>> they fix an actual bug.
>>
>
> Please drop this patch as it does not fix any bug. I was blind.
> 'found_error' is already cleared after mcheck_cmn_handler() used it.
> Non-empty 'mce_fatal_errors' results in mc_panic(), so there is no
> need to clear it.
>
> Sorry for the noise.
Well, reducing the scope of the variables is still a worthwhile thing.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] xen/mce: fix static variable 'severity_cpu'
2017-04-06 8:49 ` Haozhong Zhang
@ 2017-04-06 9:18 ` Jan Beulich
0 siblings, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2017-04-06 9:18 UTC (permalink / raw)
To: Haozhong Zhang; +Cc: Andrew Cooper, xen-devel
>>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-04-06 9:18 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-06 4:55 [PATCH 1/2] xen/mce: fix static variables 'found_error' and 'mce_fatal_cpus' Haozhong Zhang
2017-04-06 4:55 ` [PATCH 2/2] xen/mce: fix static variable 'severity_cpu' Haozhong Zhang
2017-04-06 8:06 ` Jan Beulich
2017-04-06 8:49 ` Haozhong Zhang
2017-04-06 9:18 ` Jan Beulich
2017-04-06 7:54 ` [PATCH 1/2] xen/mce: fix static variables 'found_error' and 'mce_fatal_cpus' Jan Beulich
2017-04-06 8:49 ` Haozhong Zhang
2017-04-06 9:17 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.