On 05/17/2011 05:26 PM, Ingo Molnar wrote:
>
> * Huang Ying <ying.huang@xxxxxxxxx> wrote:
>
>> On 05/17/2011 04:46 PM, Ingo Molnar wrote:
>>>
>>> * Huang Ying <ying.huang@xxxxxxxxx> wrote:
>>>
>>>> memory_failure() is the entry point for HWPoison memory error
>>>> recovery. It must be called in process context. But commonly
>>>> hardware memory errors are notified via MCE or NMI, so some delayed
>>>> execution mechanism must be used. In MCE handler, a work queue + ring
>>>> buffer mechanism is used.
>>>>
>>>> In addition to MCE, now APEI (ACPI Platform Error Interface) GHES
>>>> (Generic Hardware Error Source) can be used to report memory errors
>>>> too. To add support to APEI GHES memory recovery, a mechanism similar
>>>> to that of MCE is implemented. memory_failure_queue() is the new
>>>> entry point that can be called in IRQ context. The next step is to
>>>> make MCE handler uses this interface too.
>>>>
>>>> Signed-off-by: Huang Ying <ying.huang@xxxxxxxxx>
>>>> Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
>>>> Cc: Wu Fengguang <fengguang.wu@xxxxxxxxx>
>>>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>>>> ---
>>>> include/linux/mm.h | 1
>>>> mm/memory-failure.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 93 insertions(+)
>>>
>>> I have to say i disagree with how this is designed and how this is exposed to
>>> user-space - and i pointed this out before.
>>>
>>> It's up to Len whether you muck up drivers/acpi/ but here you are patching mm/
>>> again ...
>>>
>>> I just had a quick look into the current affairs of mm/memory-inject.c and it
>>> has become an *even* nastier collection of hacks since the last time i
>>> commented on its uglies.
>>>
>>> Special hack upon special hack, totally disorganized code, special-purpose,
>>> partly ioctl driven opaque information extraction to user-space using the
>>> erst-dbg device interface. We have all the maintenance overhead and little of
>>> the gains from hw error event features...
>>
>> Like the name suggested, erst-dbg is only for debugging. [...]
>
> Great, if printk does everything then can the debugging code be removed so that
> tooling does not accidentally make non-debugging use of it? I can write a patch
> for that.
The erst-dbg is only used for us to test whether the BIOS ERST
implementation works. If you have concerns about its mis-usage, how
about moving it to debugfs to make it clear that it is not a API, just
for debugging?
>> [...] It is not a user space interface. The user space interface used by
>> APEI now is printk.
>
> We definitely want printks obviously and primarily - often that is the only
> thing the admin sees, and most of the time there's no automatable 'policy
> action' anyway: human intervention is still the most common 'action' that is
> performed on exceptional system events.
>
> Does all the (unspecified) tooling you are enabling here work based off on
> printk only, or does it perhaps make use of the erst-dbg hack? :-)
The only tool makes use of erst-dbg is the debugging tool to test BIOS
ERST implementation. There is absolutely NO other tool I am enabling
uses erst-dbg.
> [ Wrt. printks we definitely would like to have a printk free-form-ASCII event
> gateway for tooling wants to use printk events in the regular flow of events
> that are not available via the syslog - Steve sent a print-string-event patch
> for that some time ago and that works well. ]
Thanks for your reminding, I will take a look at it.
>>> In this patch you add:
>>>
>>> +struct memory_failure_entry {
>>> + unsigned long pfn;
>>> + int trapno;
>>> + int flags;
>>> +};
>>>
>>> Instead of exposing this event to other users who might be interested in these
>>> events - such as the RAS daemon under development by Boris.
>>>
>>> We have a proper framework (ring-buffer, NMI execution, etc.) for reporting
>>> events, why are you not using (and extending) it instead of creating this nasty
>>> looking, isolated, ACPI specific low level feature?
>>
>> This patch has nothing to do with hardware error event reporting. It is just
>> about hardware error recovering.
>
> Hardware error event reporting and recovery go hand in hand. First is the
> event, the second is the action.
>
> Your structure demonstrates this already: it's called memory_failure_entry. It
> does:
>
> + * This function is called by the low level hardware error handler
> + * when it detects hardware memory corruption of a page. It schedules
> + * the recovering of error page, including dropping pages, killing
> + * processes etc.
>
> So based off an error event it does one from a short list of in-kernel policy
> actions.
>
> If put into a proper framework this would be a lot more widely useful: we could
> for example trigger the killing of tasks (and other policy action) if other
> (bad) events are triggered - not just the ones that fit into the narrow ACPI
> scheme you have here.
>
> Certain fatal IO errors would be an example, or SLAB memory corruptions or OOM
> errors - or any other event we are able to report today.
>
> So why are we not working towards integrating this into our event
> reporting/handling framework, as i suggested it from day one on when you
> started posting these patches?
The memory_failure_queue() introduced in this patch is general, that is,
it can be used not only by ACPI/APEI, but also any other hardware error
handlers, including your event reporting/handling framework.
Best Regards,
Huang Ying
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
