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 C8742C77B6F for ; Tue, 11 Apr 2023 14:28:26 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 6D775280009; Tue, 11 Apr 2023 10:28:26 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 687D2280001; Tue, 11 Apr 2023 10:28:26 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5508E280009; Tue, 11 Apr 2023 10:28:26 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 45616280001 for ; Tue, 11 Apr 2023 10:28:26 -0400 (EDT) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id E3D2A160E19 for ; Tue, 11 Apr 2023 14:28:25 +0000 (UTC) X-FDA: 80669340570.29.BABDD07 Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by imf12.hostedemail.com (Postfix) with ESMTP id 650CE40012 for ; Tue, 11 Apr 2023 14:28:21 +0000 (UTC) Authentication-Results: imf12.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf12.hostedemail.com: domain of wangkefeng.wang@huawei.com designates 45.249.212.187 as permitted sender) smtp.mailfrom=wangkefeng.wang@huawei.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1681223304; 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=zYrqLWZvb9Rw6NFGnQnFf2NHGDDPlYHTPf0w8L9hXls=; b=RgCah8FhQiQSyrI4+4TLn+KuegnzD2Ipnj/lSg3a4ITMXhsJ4mermWqHk6oJpremlwB6+w rUtduQ7Ji7oNL0eWrkD9lrsDXM1m6p8F6LNLe4+ZoBj3yemk1eOpDaBWJPpTY4tBUDLyp9 HktRmAyp24GWnfnZ8WCbI0OrUnTNycY= ARC-Authentication-Results: i=1; imf12.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf12.hostedemail.com: domain of wangkefeng.wang@huawei.com designates 45.249.212.187 as permitted sender) smtp.mailfrom=wangkefeng.wang@huawei.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1681223304; a=rsa-sha256; cv=none; b=WQVkA/seNe4kihiZfG4NqtC7Tu6aXLT7tV5YbSevH/Lxd4rnG+2fmKYhxztk9MhvDZVLjg vp+wFJcktJAvPwnGWGbmZWoboxj3mb4vusNxw8BmtiYdNVD75jJxI2w0Me7YcZhKhDup04 G3yrnd7EsEcIeeJpUUMnibRkaqrISrc= Received: from dggpemm500001.china.huawei.com (unknown [172.30.72.57]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4Pwp7d5JQYzrZK3; Tue, 11 Apr 2023 22:26:53 +0800 (CST) Received: from [10.174.177.243] (10.174.177.243) by dggpemm500001.china.huawei.com (7.185.36.107) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.23; Tue, 11 Apr 2023 22:28:15 +0800 Message-ID: <84fd1fad-604e-0835-2813-199195aaab69@huawei.com> Date: Tue, 11 Apr 2023 22:28:14 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.5.1 Subject: Re: [PATCH v5 2/2] ACPI: APEI: handle synchronous exceptions in task work Content-Language: en-US To: Shuai Xue , , , , CC: , , , , , , , , , , , , , , , , , , References: <20221027042445.60108-1-xueshuai@linux.alibaba.com> <20230411104842.37079-3-xueshuai@linux.alibaba.com> From: Kefeng Wang In-Reply-To: <20230411104842.37079-3-xueshuai@linux.alibaba.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.174.177.243] X-ClientProxiedBy: dggems702-chm.china.huawei.com (10.3.19.179) To dggpemm500001.china.huawei.com (7.185.36.107) X-CFilter-Loop: Reflected X-Rspam-User: X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: 650CE40012 X-Stat-Signature: r3knish8xpoeiw3kfagomxnw3x44w1mf X-HE-Tag: 1681223301-792241 X-HE-Meta: U2FsdGVkX19KiC3VqWcOT+3BTM4e9j6f/vyvt4Xa4yMTao/F5SDQeKKWKZg/2zroWU/MtZZckqO9b41aC4xe9wJlIMWumvVs6/6zQ/Cj6qtN4FLL4nfx8flSTymSP9qpsqa0W3JHRA0l94S6Y+AyLAd/Z8V+HxRtQgBur/dzmIuUDAoR0rpIB4OVJ1j+DkKNfi2R3mnvXD4sENwD7bqgaRVOzZK1co5LNFiBUqON6J5WlPAruvqwSsaRIUC0+xp+BCVtVsZD3xN6QGX2H4BQDu95zM6vNenOlrxW2A0mb8YUtD5V6LZfNmbGqfzXMXG7Tvg0RwjRFU6tN9KYJyDdZkM29Y/4Fx/DsAYlz3Z/2dSIeDo4l/9JOjJlpUWepSx4u8nX6VuUaKis70rKYHrzr8wfGIDxDraruLCnen8jpFQ8e5kjS5/2M6bhn/ipEY8uPB6Yx9htha0v8aM7hem2Y1GlAsJ/Dg836zFx2fNmouNEztjonmWxcvJFTz7mD5B+bnk2i78L9rqAm7uAkD8LLSszqdDwihg3MZXBWzeolfx+DOaK6IqGNEf25REUtHzBeKioc1xTfChvwcv3nV366z4V7HITPDGoQvtHsO5lpcXE2pEV4uVwa0whdxHYEV+jbxZlSb3mubvZZb4HxqBU+m0hv6FVOi/vPfSHJooDa0nebgbOmCuEjeZZsVeGJesGa+QNOhkiXhNYGcLsGKGrS+w4RpeKHAXZKBWeaA0hlay1NTnkrkaPpl0EG5h+8qmRoKUlZLyou8js65H5lRX0nJpk2DGqyEpEvL1L0dgQUQ2vMwb1qjDNsnt37/cb1lU6hXqLtY3c/esigqJOVgrFyXwuOyoe9EJILj3LhKIY7p8PR7INkspf1+lS0TrMjLOZHulyB45VgD7bp3mcvbgJdBI8hGtmO0bFK6p9OwBanX75SaSPFHIWMugD2R035XDr00Pfy0yC0+Rso5yCRx/ EhL66WY3 9cWCyoXpqOh8Hb9jnjgwGVidS/0N1jVaSr5xSfIbeDwvD0SGOOcmTicMJTfe5ro+vhBW84dEbkJEG5l4tyVaZgFW9c0os6Dj4JgjfU5Kv5/4Lu6SZi1T4H367RimWl2apWvMUBP4OIhlyWBqARqpE/v0c4GdYspSr4NOwwKPTnlthY5nA387ZLUjLof8nqn73+VCt4jCl7mDRjEM= 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: On 2023/4/11 18:48, Shuai Xue wrote: > Hardware errors could be signaled by synchronous 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. > 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 directy notify > 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(). > > 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. > - abnormal branches such as invalid PA, unexpected severity, no memory > failure config support, invalid GUID section, OOM, etc. > > 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. > > Fixes: 7f17b4a121d0 ("ACPI: APEI: Kick the memory_failure() queue for synchronous errors") > Signed-off-by: Shuai Xue > Tested-by: Ma Wupeng > --- > drivers/acpi/apei/ghes.c | 91 +++++++++++++++++++++++++++------------- > include/acpi/ghes.h | 3 -- > mm/memory-failure.c | 13 ------ > 3 files changed, 61 insertions(+), 46 deletions(-) > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index c479b85899f5..4b70955e25f9 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -452,28 +452,51 @@ static void ghes_clear_estatus(struct ghes *ghes, > } > > /* > - * Called as task_work before returning to user-space. > - * Ensure any queued work has been done before we return to the context that > - * triggered the notification. > + * struct sync_task_work - for synchronous RAS event > + * > + * @twork: callback_head for task work > + * @pfn: page frame number of corrupted page > + * @flags: fine tune action taken > + * > + * Structure to pass task work to be handled before > + * ret_to_user via task_work_add(). > */ > -static void ghes_kick_task_work(struct callback_head *head) > +struct sync_task_work { > + struct callback_head twork; > + u64 pfn; > + int flags; > +}; > + > +static void memory_failure_cb(struct callback_head *twork) > { > - struct acpi_hest_generic_status *estatus; > - struct ghes_estatus_node *estatus_node; > - u32 node_len; > + int ret; > + struct sync_task_work *twcb = > + container_of(twork, struct sync_task_work, twork); > > - estatus_node = container_of(head, struct ghes_estatus_node, task_work); > - if (IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE)) > - memory_failure_queue_kick(estatus_node->task_work_cpu); > + ret = memory_failure(twcb->pfn, twcb->flags); > + kfree(twcb); > > - estatus = GHES_ESTATUS_FROM_NODE(estatus_node); > - node_len = GHES_ESTATUS_NODE_LEN(cper_estatus_len(estatus)); > - gen_pool_free(ghes_estatus_pool, (unsigned long)estatus_node, node_len); > + if (!ret) > + return; > + > + /* > + * -EHWPOISON from memory_failure() means that it already sent SIGBUS > + * to the current process with the proper error info, This should be part of the comments of function memory_failure(), > + * -EOPNOTSUPP means hwpoison_filter() filtered the error event, > + * and this part is already there > + * In both cases, no further processing is required. > + */ so, after that, I think we could drop this comment, also the same comment in x86's kill_me_maybe(). > + if (ret == -EHWPOISON || ret == -EOPNOTSUPP) > + return; > + > + pr_err("Memory error not recovered"); > + force_sig(SIGBUS); > } > > static bool ghes_do_memory_failure(u64 physical_addr, int flags) > { > unsigned long pfn; > + struct sync_task_work *twcb; > > if (!IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE)) > return false; > @@ -486,6 +509,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; > + > + 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; > } > @@ -1000,9 +1035,8 @@ static void ghes_proc_in_irq(struct irq_work *irq_work) > struct ghes_estatus_node *estatus_node; > struct acpi_hest_generic *generic; > struct acpi_hest_generic_status *estatus; > - bool task_work_pending; > + bool queued, sync; > u32 len, node_len; > - int ret; > > llnode = llist_del_all(&ghes_estatus_llist); > /* > @@ -1015,27 +1049,25 @@ static void ghes_proc_in_irq(struct irq_work *irq_work) > estatus_node = llist_entry(llnode, struct ghes_estatus_node, > llnode); > estatus = GHES_ESTATUS_FROM_NODE(estatus_node); > + sync = is_hest_sync_notify(estatus_node->ghes); > len = cper_estatus_len(estatus); > node_len = GHES_ESTATUS_NODE_LEN(len); > - task_work_pending = ghes_do_proc(estatus_node->ghes, estatus); > + > + queued = ghes_do_proc(estatus_node->ghes, estatus) > + /* > + * If no memory failure work is queued for abnormal synchronous > + * errors, do a force kill. > + */ > + if (sync && !queued) > + force_sig(SIGBUS); It's better to move this part into function ghes_do_proc(), because there is already an is_hest_sync_notify(), and no need return value, so make ghes_do_proc() a void function, Apart from this, Reviewed-by: Kefeng Wang > + > if (!ghes_estatus_cached(estatus)) { > generic = estatus_node->generic; > if (ghes_print_estatus(NULL, generic, estatus)) > ghes_estatus_cache_add(generic, estatus); > } > - > - if (task_work_pending && current->mm) { > - estatus_node->task_work.func = ghes_kick_task_work; > - estatus_node->task_work_cpu = smp_processor_id(); > - ret = task_work_add(current, &estatus_node->task_work, > - TWA_RESUME); > - if (ret) > - estatus_node->task_work.func = NULL; > - } > - > - if (!estatus_node->task_work.func) > - gen_pool_free(ghes_estatus_pool, > - (unsigned long)estatus_node, node_len); > + gen_pool_free(ghes_estatus_pool, (unsigned long)estatus_node, > + node_len); > > llnode = next; > } > @@ -1096,7 +1128,6 @@ static int ghes_in_nmi_queue_one_entry(struct ghes *ghes, > > estatus_node->ghes = ghes; > estatus_node->generic = ghes->generic; > - estatus_node->task_work.func = NULL; > estatus = GHES_ESTATUS_FROM_NODE(estatus_node); > > if (__ghes_read_estatus(estatus, buf_paddr, fixmap_idx, len)) { > diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h > index 3c8bba9f1114..e5e0c308d27f 100644 > --- a/include/acpi/ghes.h > +++ b/include/acpi/ghes.h > @@ -35,9 +35,6 @@ struct ghes_estatus_node { > struct llist_node llnode; > struct acpi_hest_generic *generic; > struct ghes *ghes; > - > - int task_work_cpu; > - struct callback_head task_work; > }; > > struct ghes_estatus_cache { > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index fae9baf3be16..6ea8c325acb3 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -2355,19 +2355,6 @@ static void memory_failure_work_func(struct work_struct *work) > } > } > > -/* > - * Process memory_failure work queued on the specified CPU. > - * Used to avoid return-to-userspace racing with the memory_failure workqueue. > - */ > -void memory_failure_queue_kick(int cpu) > -{ > - struct memory_failure_cpu *mf_cpu; > - > - mf_cpu = &per_cpu(memory_failure_cpu, cpu); > - cancel_work_sync(&mf_cpu->work); > - memory_failure_work_func(&mf_cpu->work); > -} > - > static int __init memory_failure_init(void) > { > struct memory_failure_cpu *mf_cpu;