Re: [PATCH 5/9] HWPoison: add memory_failure_queue()

[Huang Ying <ying.huang@intel.com>]

On 05/17/2011 05:26 PM, Ingo Molnar wrote:
> 
> * Huang Ying <ying.huang@intel.com> wrote:
> 
>> On 05/17/2011 04:46 PM, Ingo Molnar wrote:
>>>
>>> * Huang Ying <ying.huang@intel.com> 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@intel.com>
>>>> Cc: Andi Kleen <ak@linux.intel.com>
>>>> Cc: Wu Fengguang <fengguang.wu@intel.com>
>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>> ---
>>>>  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