Messages in this thread | | | Subject | Re: [RFC 2/2] ACPI: APEI: fix reboot caused by synchronous error loop because of memory_failure() failed | From | Lv Ying <> | Date | Thu, 8 Dec 2022 10:44:52 +0800 |
| |
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c >> index 3b6ac3694b8d..4c1c558f7161 100644 >> --- a/mm/memory-failure.c >> +++ b/mm/memory-failure.c >> @@ -2266,7 +2266,11 @@ static void __memory_failure_work_func(struct >> work_struct *work, bool sync) >> break; >> if (entry.flags & MF_SOFT_OFFLINE) >> soft_offline_page(entry.pfn, entry.flags); >> - else if (!sync || (entry.flags & MF_ACTION_REQUIRED)) >> + else if (sync) { >> + if ((entry.flags & MF_ACTION_REQUIRED) && >> + memory_failure(entry.pfn, entry.flags)) >> + force_sig_mceerr(BUS_MCEERR_AR, 0, 0); >> + } else >> memory_failure(entry.pfn, entry.flags); > Hi, > > Some of the ideas in this patch are wrong :-( > > 1. As Shuai Xue said, it is wrong to judge synchronization error and > asynchronization error through functions such as > memory_failure_queue_kick()/ghes_proc()/ghes_proc_in_irq(), because both > synchronization error and asynchronization error may go to the same > notification. > Hi Bixuan:
Thanks for your review. I agree with you that ghes_proc_in_irq() is called in SDEI, SEA, NMI notify type, they are NMI-like notify, this function run some job which may not be NMI safe in IRQ context. And NMI may be asynchronous error.
However, cureent kernel use ghes_kick_task_work in ghes_proc_in_irq(), there is an assumption here that ghes_proc_in_irq() are currently in the context of a synchronous exception, although this is not appropriate.
The challenge for my patch is to prove the rationality of distinguishing synchronous errors. I do not have a good idea yet of distinguishing synchronous error by looking through ACPI/UEFI spec, so I sent this patchset for more input. And I resent RFC PATCH v1 [1]add this as TODO.
> 2. There is no need to pass 'sync' to __memory_failure_work_func(), > because memory_failure() can directly handle synchronous and > asynchronous errors according to entry.flags & MF_ACTION_REQUIRED: > > entry.flags & MF_ACTION_REQUIRED == 1: Action: poison page and kill task > for synchronous error > entry.flags & MF_ACTION_REQUIRED == 0: Action: poison page for > asynchronous error > > Reference x86: > do_machine_check # MCE, synchronous > ->kill_me_maybe > ->memory_failure(p->mce_addr >> PAGE_SHIFT, MF_ACTION_REQUIRED); > > uc_decode_notifier # CMCI, asynchronous > ->memory_failure(pfn, 0) > > At the same time, the modification here is repeated with your patch 01 > if (sev == GHES_SEV_RECOVERABLE && sec_sev == GHES_SEV_RECOVERABLE) > - flags = 0; > + flags = sync ? MF_ACTION_REQUIRED : 0; >
Thanks, there is indeed no need to pass 'sync' to __memory_failure_work_func(). MF_ACTION_REQUIRED can cover this, I will update it in the next version patchset.
> 3. Why add 'force_sig_mceerr(BUS_MCEERR_AR, 0, 0)' after > memory_failure(pfn, MF_ACTION_REQUIRED)? > The task will be killed in memory_failure(): > if poisoned, kill_accessing_process()->kill_proc() > if not poisoned, hwpoison_user_mappings()->collect_procs()->kill_procs() > > Reference x86 to handle synchronous error: > kill_me_maybe() > { > int flags = MF_ACTION_REQUIRED; > ret = memory_failure(p->mce_addr >> PAGE_SHIFT, flags); > if (!ret) { > ... > return; > } > if (ret == -EHWPOISON || ret == -EOPNOTSUPP) > return; > > pr_err("Memory error not recovered"); > kill_me_now(cb); > } >
Thanks again, this patch is based on synchronous error is not distinguished from asynchronous error, in that case, kill_accessing_process() run in kthread worker may not kill current thread. Now, based on the first patch, this SEA loop can be handled. But this patch is also needed reference x86 kill_me_maybe(), I update this patch in RFC PATCH v1[1]. I will integrate this patch into the first patch, because this patch commit message is not suitable based on the first patch.
[1] https://lore.kernel.org/linux-mm/20221207093935.1972530-1-lvying6@huawei.com/T/
-- Thanks! Lv Ying
| |