Messages in this thread | | | Date | Wed, 7 Dec 2022 20:29:04 +0800 | Subject | Re: [RFC 2/2] ACPI: APEI: fix reboot caused by synchronous error loop because of memory_failure() failed | From | Bixuan Cui <> |
| |
在 2022/12/5 19:51, Lv Ying 写道: > 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.
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;
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, Bixuan Cui
| |