Re: [RFC PATCH v2 1/1] ACPI: APEI: Make memory_failure() triggered by synchronization errors execute in the current context

[HORIGUCHI NAOYA(堀口 直也)]

On Fri, Dec 09, 2022 at 05:54:07PM +0800, Lv Ying wrote:
> The memory uncorrected error which is detected by an external component and
> notified via an IRQ, can be called asynchronization error. If an error is
> detected as a result of user-space process accessing a corrupt memory
> location, the CPU may take an abort. On arm64 this is a
> 'synchronous external abort', and on a firmware first system it is notified
> via NOTIFY_SEA, this can be called synchronization error.
> 

"synchronization error" in this context looks weird to me, maybe you mean
"synchronous error" ?  There're many places using "synchronization", so
please use consistent wording.

> Currently, synchronization error and asynchronization error both use
> memory_failure_queue to schedule memory_failure() exectute in kworker
> context. Commit 7f17b4a121d0 ("ACPI: APEI: Kick the memory_failure() queue
> for synchronous errors") make task_work pending to flush out the queue,
> cancel_work_sync() in memory_failure_queue_kick() will make
> memory_failure() exectute in kworker context first which will get

s/exectute/execute/

> synchronization error info from kfifo, so task_work later will get nothing
> from kfifo which doesn't work as expected. Even worse, synchronization
> error notification has NMI like properties, (it can interrupt IRQ-masked
> code), task_work may get wrong kfifo entry from interrupted
> asynchronization error which is notified by IRQ.
> 
> Since the memory_failure() triggered by a synchronous exception is
> executed in the kworker context, the early_kill mode of memory_failure()
> will send wrong si_code by SIGBUS signal: current process is kworker
> thread, the actual user-space process accessing the corrupt memory location
> will be collected by find_early_kill_thread(), and then send SIGBUS with
> BUS_MCEERR_AO si_code to the actual user-space process instead of
> BUS_MCEERR_AR. The machine-manager(kvm) use the si_code: BUS_MCEERR_AO for
> 'action optional' early notifications, and BUS_MCEERR_AR for
> 'action required' synchronous/late notifications.
> 
> Make memory_failure() triggered by synchronization errors execute in the
> current context, we do not need workqueue for synchronization error
> anymore, use task_work handle synchronization errors directly. Since,
> synchronization errors and asynchronization errors share the same kfifo,
> use MF_ACTION_REQUIRED flag to distinguish them. And the asynchronization
> error keeps the same as before.
> 
> Currently, it's hard to distinguish synchronization error in APEI. It
> can be determined that the SEA report synchronization error, so
> currently only the synchronization error reported by SEA is distinguished
> and handled in current context.
> 
> Signed-off-by: Lv Ying <lvying6@xxxxxxxxxx>
> ---
>  drivers/acpi/apei/ghes.c | 20 +++++++++-------
>  mm/memory-failure.c      | 50 +++++++++++++++++++++++++++++-----------
>  2 files changed, 48 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 9952f3a792ba..19d62ec2177f 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -423,8 +423,8 @@ 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.
> + * Ensure any queued corrupt page in synchronous errors has been handled before
> + * we return to the user context that triggered the notification.
>   */
>  static void ghes_kick_task_work(struct callback_head *head)
>  {
> @@ -461,7 +461,7 @@ static bool ghes_do_memory_failure(u64 physical_addr, int flags)
>  }
>  
>  static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
> -				       int sev)
> +				       int sev, int notify_type)
>  {
>  	int flags = -1;
>  	int sec_sev = ghes_severity(gdata->error_severity);
> @@ -475,7 +475,7 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
>  	    (gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED))
>  		flags = MF_SOFT_OFFLINE;
>  	if (sev == GHES_SEV_RECOVERABLE && sec_sev == GHES_SEV_RECOVERABLE)
> -		flags = 0;
> +		flags = (notify_type == ACPI_HEST_NOTIFY_SEA) ? MF_ACTION_REQUIRED : 0;
>  
>  	if (flags != -1)
>  		return ghes_do_memory_failure(mem_err->physical_addr, flags);
> @@ -483,7 +483,8 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
>  	return false;
>  }
>  
> -static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, int sev)
> +static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, int sev,
> +		int notify_type)
>  {
>  	struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
>  	bool queued = false;
> @@ -510,7 +511,9 @@ static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, int s
>  		 * and don't filter out 'corrected' error here.
>  		 */
>  		if (is_cache && has_pa) {
> -			queued = ghes_do_memory_failure(err_info->physical_fault_addr, 0);
> +			queued = ghes_do_memory_failure(err_info->physical_fault_addr,
> +					(notify_type == ACPI_HEST_NOTIFY_SEA) ?
> +					MF_ACTION_REQUIRED : 0);
>  			p += err_info->length;
>  			continue;
>  		}
> @@ -631,6 +634,7 @@ static bool ghes_do_proc(struct ghes *ghes,
>  	const guid_t *fru_id = &guid_null;
>  	char *fru_text = "";
>  	bool queued = false;
> +	int notify_type = ghes->generic->notify.type;
>  
>  	sev = ghes_severity(estatus->error_severity);
>  	apei_estatus_for_each_section(estatus, gdata) {
> @@ -648,13 +652,13 @@ static bool ghes_do_proc(struct ghes *ghes,
>  			ghes_edac_report_mem_error(sev, mem_err);
>  
>  			arch_apei_report_mem_error(sev, mem_err);
> -			queued = ghes_handle_memory_failure(gdata, sev);
> +			queued = ghes_handle_memory_failure(gdata, sev, notify_type);
>  		}
>  		else if (guid_equal(sec_type, &CPER_SEC_PCIE)) {
>  			ghes_handle_aer(gdata);
>  		}
>  		else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) {
> -			queued = ghes_handle_arm_hw_error(gdata, sev);
> +			queued = ghes_handle_arm_hw_error(gdata, sev, notify_type);
>  		} else {
>  			void *err = acpi_hest_get_payload(gdata);
>  
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index bead6bccc7f2..82238ec86acd 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2204,7 +2204,11 @@ struct memory_failure_cpu {
>  static DEFINE_PER_CPU(struct memory_failure_cpu, memory_failure_cpu);
>  
>  /**
> - * memory_failure_queue - Schedule handling memory failure of a page.
> + * memory_failure_queue
> + * - Schedule handling memory failure of a page for asynchronous error, memory
> + *   failure page will be executed in kworker thread
> + * - put corrupt memory info into kfifo for synchronous error, task_work will
> + *   handle them before returning to the user

I think that the top description of kernel-doc function documentation needs
to be brief, so could you move the above 2 items downward as details?
Maybe the first line can be updated like below (scheduling is done conditionally
with your change):

/**                                                                    
 * memory_failure_queue - Queue memory failure event
 * @pfn: Page Number of the corrupted page                             
 * @flags: Flags for memory failure handling
 *
 * ... (full details)

And maybe existing comment in "full details" is obsolete since commit
7f17b4a121d0 ("ACPI: APEI: Kick the memory_failure() queue for synchronous
errors"), so could you update the whole description to explain the new
behavior with some background information as done in patch description?

>   * @pfn: Page Number of the corrupted page
>   * @flags: Flags for memory failure handling
>   *
> @@ -2217,6 +2221,11 @@ static DEFINE_PER_CPU(struct memory_failure_cpu, memory_failure_cpu);
>   * happen outside the current execution context (e.g. when
>   * detected by a background scrubber)
>   *
> + * This function can also be used in synchronous errors which was detected as a

"... errors which was ..." seems unmatched in plurality.

> + * result of user-space accessing a corrupt memory location, just put memory

s/corrupt/corrupted/

> + * error info into kfifo, and then, task_work get and handle it in current
> + * execution context instead of scheduling kworker to handle it

Please put a period at the end of sentence. kernel-doc comment is
converted to auto-generated documentation, so it needs to look like
natural English text.
See https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation

> + *
>   * Can run in IRQ context.
>   */
> @@ -2230,9 +2239,10 @@ void memory_failure_queue(unsigned long pfn, int flags)
>  
>  	mf_cpu = &get_cpu_var(memory_failure_cpu);
>  	spin_lock_irqsave(&mf_cpu->lock, proc_flags);
> -	if (kfifo_put(&mf_cpu->fifo, entry))
> -		schedule_work_on(smp_processor_id(), &mf_cpu->work);
> -	else
> +	if (kfifo_put(&mf_cpu->fifo, entry)) {
> +		if (!(entry.flags & MF_ACTION_REQUIRED))
> +			schedule_work_on(smp_processor_id(), &mf_cpu->work);
> +	} else
>  		pr_err("buffer overflow when queuing memory failure at %#lx\n",
>  		       pfn);
>  	spin_unlock_irqrestore(&mf_cpu->lock, proc_flags);
> @@ -2240,12 +2250,15 @@ void memory_failure_queue(unsigned long pfn, int flags)
>  }
>  EXPORT_SYMBOL_GPL(memory_failure_queue);
>  
> -static void memory_failure_work_func(struct work_struct *work)
> +/*
> + * (a)synchronous error info should be consumed by the corresponding handler
> + */
> +static void __memory_failure_work_func(struct work_struct *work, bool sync)
>  {
>  	struct memory_failure_cpu *mf_cpu;
>  	struct memory_failure_entry entry = { 0, };
>  	unsigned long proc_flags;
> -	int gotten;
> +	int gotten, ret;
>  
>  	mf_cpu = container_of(work, struct memory_failure_cpu, work);
>  	for (;;) {
> @@ -2256,22 +2269,31 @@ static void memory_failure_work_func(struct work_struct *work)
>  			break;
>  		if (entry.flags & MF_SOFT_OFFLINE)
>  			soft_offline_page(entry.pfn, entry.flags);
> -		else
> -			memory_failure(entry.pfn, entry.flags);
> +		else {
> +			if (sync && (entry.flags & MF_ACTION_REQUIRED)) {
> +				ret = memory_failure(entry.pfn, entry.flags);
> +				if (ret == -EHWPOISON || ret == -EOPNOTSUPP)
> +					return;
> +
> +				pr_err("Memory error not recovered");
> +				force_sig(SIGBUS);
> +			} else if (!sync && !(entry.flags & MF_ACTION_REQUIRED))
> +				memory_failure(entry.pfn, entry.flags);

So if sync is true and MF_ACTION_REQUIRED is not set, memory_failure() is
not called.  Does that break something?

Thanks,
Naoya Horiguchi

> +		}
>  	}
>  }
>  
> -/*
> - * Process memory_failure work queued on the specified CPU.
> - * Used to avoid return-to-userspace racing with the memory_failure workqueue.
> - */
> +static void memory_failure_work_func(struct work_struct *work)
> +{
> +	__memory_failure_work_func(work, false);
> +}
> +
>  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);
> +	__memory_failure_work_func(&mf_cpu->work, true);
>  }
>  
>  static int __init memory_failure_init(void)
> -- 
> 2.36.1