Re: [PATCH 10/10] MCE: Add Action-Required support

[Tony Luck <tony.luck@intel.com>]

2011/6/10 Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>:
>> +static int kernel_ar_recoverable(struct mce *m, int tolerant)
>> +{
>> +     if (tolerant >= 2)
>> +             return MCE_AR_SEVERITY;
>> +     if (!(m->mcgstatus & MCG_STATUS_EIPV) || !m->ip)
>> +             return MCE_PANIC_SEVERITY;
>> +     if (search_exception_tables(m->ip))
>> +             return MCE_AR_SEVERITY;
>> +     return MCE_PANIC_SEVERITY;
>> +}
>> +
>
> You said "Just handle errors in user mode for now." but ...?

Oops. I dropped the kernel handling bits from mce.c, but forget to
clean them out of mce-severity.c

Will fix.

>> +     if (!mce_usable_address(m))
>> +             mce_panic("No address for Action-Required Machine Check",
>> +                       m, msg);
>> +     if (!(m->mcgstatus & MCG_STATUS_EIPV))
>> +             mce_panic("No EIPV for Action-Required Machine Check",
>> +                       m, msg);
>
> When can this happen?

Probably never (or only if there was a chip bug. AR errors should
provide addresses
(otherwise there is no way to perform any useful "action").

> Why not create new severity {PANIC, "Action Required: but No EIPV", ...}
> in severity table?

Very good idea.

>> +static void ar_fallback(struct task_struct *me, unsigned long pfn)
>> +{
>> +     if (signal_pending(me) && sigismember(&me->pending.signal, SIGBUS))
>> +             return;
>
> Is it safe for _AR if SIGBUS is pending but blocked?
> I think force_sig() is reasonable for such situation.

Agreed - it is not safe if the signal is blocked.  force_sig() sounds better.

>> +     if (worst == MCE_AR_SEVERITY) {
>> +             mce_action_required(&m, msg, regs);
>
> Comprehensible name would be appreciated, e.g.:
>                mce_request_dpc_for_action_required(pfn);

I'm always in the mood for better names ... but I'm not sure what
the "dpc" in this name is trying to tell me.

> And if we cannot request context for recovery, it is better to suppress
> trailing attempts, e.g. before mce_end():
>
>        if (!no_way_out && severity == MCE_AR_SEVERITY) {
>                err = mce_request_dpc_for_action_required(pfn);
>                if (err) {
>                        atomic_inc(&global_nwo);
>                        severity = MCE_PANIC_SEVERITY;  /* escalated */
>                }
>        }

Agreed - if we can't perform the recovery action, we should escalate
the severity.

-Tony