From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id D11C5C10DC1 for ; Fri, 1 Dec 2023 07:03:48 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C738D6B04CC; Fri, 1 Dec 2023 02:03:47 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id C23086B04CD; Fri, 1 Dec 2023 02:03:47 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id AEA706B04CE; Fri, 1 Dec 2023 02:03:47 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id A0B396B04CC for ; Fri, 1 Dec 2023 02:03:47 -0500 (EST) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 66D0BA00E8 for ; Fri, 1 Dec 2023 07:03:47 +0000 (UTC) X-FDA: 81517359294.09.9C0C91A Received: from out30-119.freemail.mail.aliyun.com (out30-119.freemail.mail.aliyun.com [115.124.30.119]) by imf15.hostedemail.com (Postfix) with ESMTP id CA6C6A0011 for ; Fri, 1 Dec 2023 07:03:42 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=alibaba.com; spf=pass (imf15.hostedemail.com: domain of xueshuai@linux.alibaba.com designates 115.124.30.119 as permitted sender) smtp.mailfrom=xueshuai@linux.alibaba.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1701414224; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=/idfxbIaysKaaK5YhkZgqHzUJz2uavwRYBbbWQ5lQCU=; b=rUXFaGzLGCvbeEogndiqXd8UDWtxaG5SwEXHOpxL56DiIo+xqztuYSDRpc/f+vGspTH3PE ZhQBt0JHiedZ0YFjnQ5ozHymbI80mFd91l6oAJ93BHLGxyFKirGtp3eL7QTwT96zb+UqCa Xk8ERrg2Wm1qdgY7vuegsxg/XKfeuMI= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=alibaba.com; spf=pass (imf15.hostedemail.com: domain of xueshuai@linux.alibaba.com designates 115.124.30.119 as permitted sender) smtp.mailfrom=xueshuai@linux.alibaba.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1701414224; a=rsa-sha256; cv=none; b=6tMXA4bD1KMExzoQKBPHfnVOMtxt57vN8mYXCCND7YF7VptLteedkli36azvxg2sFLZpdf taGWoLIz/UC6IH+D3okqAFtxFSC4uOQXlDBRCttn46aMt7o3RDryqy1WYgHC03BW7INUVk 9QvXYYATyuNo10DWdzS7xme6GiDHx0Y= X-Alimail-AntiSpam:AC=PASS;BC=-1|-1;BR=01201311R131e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=ay29a033018046050;MF=xueshuai@linux.alibaba.com;NM=1;PH=DS;RN=34;SR=0;TI=SMTPD_---0VxXwK5O_1701414213; Received: from 30.240.114.121(mailfrom:xueshuai@linux.alibaba.com fp:SMTPD_---0VxXwK5O_1701414213) by smtp.aliyun-inc.com; Fri, 01 Dec 2023 15:03:37 +0800 Message-ID: Date: Fri, 1 Dec 2023 15:03:28 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v9 2/2] ACPI: APEI: handle synchronous exceptions in task work Content-Language: en-US To: James Morse , rafael@kernel.org, wangkefeng.wang@huawei.com, tanxiaofei@huawei.com, mawupeng1@huawei.com, tony.luck@intel.com, linmiaohe@huawei.com, naoya.horiguchi@nec.com, gregkh@linuxfoundation.org, will@kernel.org, jarkko@kernel.org Cc: linux-acpi@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, akpm@linux-foundation.org, linux-edac@vger.kernel.org, acpica-devel@lists.linuxfoundation.org, stable@vger.kernel.org, x86@kernel.org, justin.he@arm.com, ardb@kernel.org, ying.huang@intel.com, ashish.kalra@amd.com, baolin.wang@linux.alibaba.com, bp@alien8.de, tglx@linutronix.de, mingo@redhat.com, dave.hansen@linux.intel.com, lenb@kernel.org, hpa@zytor.com, robert.moore@intel.com, lvying6@huawei.com, xiexiuqi@huawei.com, zhuo.song@linux.alibaba.com References: <20221027042445.60108-1-xueshuai@linux.alibaba.com> <20231007072818.58951-3-xueshuai@linux.alibaba.com> <874f0170-a829-47db-8882-52b9ed8e869d@arm.com> From: Shuai Xue In-Reply-To: <874f0170-a829-47db-8882-52b9ed8e869d@arm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: CA6C6A0011 X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: z3jkqxiibpqy4z9by5zuozrir8wtjksh X-HE-Tag: 1701414222-21923 X-HE-Meta: U2FsdGVkX19P1o8SS1Sev0ua0wsfAS0aV0cJSm7sQztjlXfMOM4y5uzpaISFRZdFcZqzWIziN2zb05CTsVLOh+byn8zJwXL2ohGz+DX2hKgXzJzob5Uv2bi+gvOrc8p7yTc/GGo7QnvlaIhgNamVG+QlMKcIPhvzKMilZ2bWK9P/UC6TVv0pe8XtzXMVB26g/vIezWmhlFnVix3GO1ZWBO8BJI4Q/3IE2y4dTXPMEIuXMSjWYmZpjp5VbnOUpUPHGzJN8/TsTvbcAEBLpTxRaK1JpFfjR0bM18JhxIPNLcICu2AoMOO9Iz0TyYZ6nI2YbhFzCMFRLeNZKbGbIIgAivvA+A+D/DFb8uVYwJjBHTh3c4OO49AEkSfp94ADP4tRCcq5zPzNbeafDcV3XnmjCmmvga9z3POzOms9MJ5vcvdZwpKBD+qsK+ukp056+eDEik578LRx46dkS3Y++QbQLOwVhuynvFWATOCXVEw60m7zVfVseM0X1v2DprdtN/3kHoqGzCi1WNBu/NNhyuxc3mgFALvGeWc5j498SOEdf+jncyQH33kdLcyLjg5X+EE4b9XCTfyfxj+JswMydCfRDS2wLdzk7pEMG8m/OEMj8SPlAReLcaXldnpLHjcwNAD8irMs7paWDj+p/QmUBONrt8XPjdW30DUQuAucseHYYAyc2pSKZ8tHs9QqKxMUvxnWMSbauzK2/jJ7ggHgSmDyZdT0j6q6drlm1O9963Pqa1JFiF3rQ7n9oYZePwMVV47IrKCQDZowyyvH4K/XHoyu6ds0jbxFeISIF2yMwAwz66F6MH45kr3bRcT+56x7Jdku7GSgObhVtbwgSvMmEBfsUzRtaD2ym2Mfua3q67YXzR4PgLYBwNX+OBHJPJR5Ph8PQV55xF4Lcf52b0mgDi4h2AHp0ZUEn1V8KXqvsz2qjKVpAImawwJR2mHh0grlxMI+V+NLeazMve0drIv8Nlz oyp0tykf sJQia4NQulhS1mEw6hP04kWOFphd9jZlhrzIh2bQtcAg5/kcg/aR/5iFwexRNowEvv/AGJe8lm72MbAwnYVfV8tC8Aj1R6Qj/NA1QeXb6hQQXU5U= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 2023/12/1 01:39, James Morse wrote: > Hi Shuai, > > On 07/10/2023 08:28, Shuai Xue wrote: >> Hardware errors could be signaled by synchronous interrupt, > > I'm struggling with 'synchronous interrupt'. Do you mean arm64's 'precise' (all > instructions before the exception were executed, and none after). > Otherwise, surely any interrupt from a background scrubber is inherently asynchronous! > I am sorry, this is typo. I mean asynchronous interrupt. > >> e.g. when an >> error is detected by a background scrubber, or signaled by synchronous >> exception, e.g. when an uncorrected error is consumed. Both synchronous and >> asynchronous error are queued and handled by a dedicated kthread in >> workqueue. >> >> commit 7f17b4a121d0 ("ACPI: APEI: Kick the memory_failure() queue for >> synchronous errors") keep track of whether memory_failure() work was >> queued, and make task_work pending to flush out the workqueue so that the >> work for synchronous error is processed before returning to user-space. > > It does it regardless, if user-space was interrupted by APEI any work queued as a result > of that should be completed before we go back to user-space. Otherwise we can bounce > between user-space and firmware, with the kernel only running the APEI code, and never > making progress. > Agreed. > >> The trick ensures that the corrupted page is unmapped and poisoned. And >> after returning to user-space, the task starts at current instruction which >> triggering a page fault in which kernel will send SIGBUS to current process >> due to VM_FAULT_HWPOISON. >> >> However, the memory failure recovery for hwpoison-aware mechanisms does not >> work as expected. For example, hwpoison-aware user-space processes like >> QEMU register their customized SIGBUS handler and enable early kill mode by >> seting PF_MCE_EARLY at initialization. Then the kernel will directly notify > > (setting, directly) Thank you. Will fix it. > >> the process by sending a SIGBUS signal in memory failure with wrong > >> si_code: the actual user-space process accessing the corrupt memory >> location, but its memory failure work is handled in a kthread context, so >> it will send SIGBUS with BUS_MCEERR_AO si_code to the actual user-space >> process instead of BUS_MCEERR_AR in kill_proc(). > > This is hard to parse, "the user-space process is accessing"? (dropping 'actual' and > adding 'is') Will fix it. > > > Wasn't this behaviour fixed by the previous patch? > > What problem are you fixing here? Nope. The memory_failure() runs in a kthread context, but not the user-space process which consuming poison data. // kill_proc() in memory-failure.c if ((flags & MF_ACTION_REQUIRED) && (t == current)) ret = force_sig_mceerr(BUS_MCEERR_AR, (void __user *)tk->addr, addr_lsb); else ret = send_sig_mceerr(BUS_MCEERR_AO, (void __user *)tk->addr, addr_lsb, t); So, even we queue memory_failure() with MF_ACTION_REQUIRED flags in previous patch, it will still send a sigbus with BUS_MCEERR_AO in the else branch of kill_proc(). > > >> To this end, separate synchronous and asynchronous error handling into >> different paths like X86 platform does: >> >> - valid synchronous errors: queue a task_work to synchronously send SIGBUS >> before ret_to_user. > >> - valid asynchronous errors: queue a work into workqueue to asynchronously >> handle memory failure. > > Why? The signal issue was fixed by the previous patch. Why delay the handling of a > poisoned memory location further? The signal issue is not fixed completely. See my reply above. > > >> - abnormal branches such as invalid PA, unexpected severity, no memory >> failure config support, invalid GUID section, OOM, etc. > > ... do what? If no memory failure work is queued for abnormal errors, do a force kill. Will also add this comment to commit log. > > >> Then for valid synchronous errors, the current context in memory failure is >> exactly belongs to the task consuming poison data and it will send SIBBUS >> with proper si_code. > > >> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c >> index 6f35f724cc14..1675ff77033d 100644 >> --- a/arch/x86/kernel/cpu/mce/core.c >> +++ b/arch/x86/kernel/cpu/mce/core.c >> @@ -1334,17 +1334,10 @@ static void kill_me_maybe(struct callback_head *cb) >> return; >> } >> >> - /* >> - * -EHWPOISON from memory_failure() means that it already sent SIGBUS >> - * to the current process with the proper error info, >> - * -EOPNOTSUPP means hwpoison_filter() filtered the error event, >> - * >> - * In both cases, no further processing is required. >> - */ >> if (ret == -EHWPOISON || ret == -EOPNOTSUPP) >> return; >> >> - pr_err("Memory error not recovered"); >> + pr_err("Sending SIGBUS to current task due to memory error not recovered"); >> kill_me_now(cb); >> } >> > > I'm not sure how this hunk is relevant to the commit message. I handle memory_failure() error code in its arm64 call site memory_failure_cb() with some comments, similar to x86 call site kill_me_maybe(). I moved these two part comments to function declaration, followed by review comments from Kefeng. I should split this into a separate patch. Will do it in next version. > > >> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c >> index 88178aa6222d..014401a65ed5 100644 >> --- a/drivers/acpi/apei/ghes.c >> +++ b/drivers/acpi/apei/ghes.c >> @@ -484,6 +497,18 @@ static bool ghes_do_memory_failure(u64 physical_addr, int flags) >> return false; >> } >> >> + if (flags == MF_ACTION_REQUIRED && current->mm) { >> + twcb = kmalloc(sizeof(*twcb), GFP_ATOMIC); >> + if (!twcb) >> + return false; > > Yuck - New failure modes! This is why the existing code always has this memory allocated > in struct ghes_estatus_node. Are you suggesting to move fields of struct sync_task_work to struct ghes_estatus_node, and use ghes_estatus_node here? Or we can just alloc struct sync_task_work with gen_pool_alloc from ghes_estatus_pool. > > >> + twcb->pfn = pfn; >> + twcb->flags = flags; >> + init_task_work(&twcb->twork, memory_failure_cb); >> + task_work_add(current, &twcb->twork, TWA_RESUME); >> + return true; >> + } >> + >> memory_failure_queue(pfn, flags); >> return true; >> } > > [..] > >> @@ -696,7 +721,14 @@ static bool ghes_do_proc(struct ghes *ghes, >> } >> } >> >> - return queued; >> + /* >> + * If no memory failure work is queued for abnormal synchronous >> + * errors, do a force kill. >> + */ >> + if (sync && !queued) { >> + pr_err("Sending SIGBUS to current task due to memory error not recovered"); >> + force_sig(SIGBUS); >> + } >> } > > I think this is a lot of churn, and this hunk is the the only meaningful change in > behaviour. Can you explain how this happens? For example: - invalid GUID section in ghes_do_proc() - CPER_MEM_VALID_PA is not set, unexpected severity in ghes_handle_memory_failure(). - CONFIG_ACPI_APEI_MEMORY_FAILURE is not enabled, !pfn_vaild(pfn) in ghes_do_memory_failure() > > > Wouldn't it be simpler to split ghes_kick_task_work() to have a sync/async version. > The synchronous version can unconditionally force_sig_mceerr(BUS_MCEERR_AR, ...) after > memory_failure_queue_kick() - but that still means memory_failure() is unable to disappear > errors that it fixed - see MF_RECOVERED. Sorry, I don't think so. Unconditionally send a sigbus is not a good choice. For example, if a sync memory error detected in instruction memory error, the kernel should transparently fix and no signal should be send. ./einj_mem_uc instr [168522.751671] Memory failure: 0x89dedd: corrupted page was clean: dropped without side effects [168522.751679] Memory failure: 0x89dedd: recovery action for clean LRU page: Recovered With this patch set, the instr case behaves consistently on both the arm64 and x86 platforms. The complex page error_states are handled in memory_failure(). IMHO, we should left this part to it. > >> diff --git a/mm/memory-failure.c b/mm/memory-failure.c >> index 4d6e43c88489..0d02f8a0b556 100644 >> --- a/mm/memory-failure.c >> +++ b/mm/memory-failure.c >> @@ -2161,9 +2161,12 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags, >> * Must run in process context (e.g. a work queue) with interrupts >> * enabled and no spinlocks held. >> * >> - * Return: 0 for successfully handled the memory error, >> - * -EOPNOTSUPP for hwpoison_filter() filtered the error event, >> - * < 0(except -EOPNOTSUPP) on failure. >> + * Return values: >> + * 0 - success >> + * -EOPNOTSUPP - hwpoison_filter() filtered the error event. >> + * -EHWPOISON - sent SIGBUS to the current process with the proper >> + * error info by kill_accessing_process(). >> + * other negative values - failure >> */ >> int memory_failure(unsigned long pfn, int flags) >> { > > I'm not sure how this hunk is relevant to the commit message. As mentioned, I will split this into a separate patch. > > > Thanks, > > James Thank you for valuable comments. Best Regards, Shuai