Re: [PATCH v2 2/2] ACPI: APEI: handle synchronous exceptions in task work

[Shuai Xue <xueshuai@linux.alibaba.com>]

On 2023/3/17 AM8:29, HORIGUCHI NAOYA(堀口 直也) wrote:
> On Thu, Mar 16, 2023 at 07:10:56PM +0800, Shuai Xue wrote:
>>
>>
>> On 2023/3/16 PM3:21, HORIGUCHI NAOYA(堀口 直也) wrote:
>>> On Mon, Feb 27, 2023 at 01:03:15PM +0800, 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:
>>>>
>>>> - task work for synchronous errors.
>>>> - and workqueue for asynchronous errors.
>>>>
>>>> Then for 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 <xueshuai@linux.alibaba.com>
>>> ...
>>>>  
>>>>  /*
>>>> - * 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 mce_task_work - for synchronous RAS event
>>>
>>> This seems to handle synchronous memory errors, not limited to MCE?
>>> So naming this struct as such (more generally) might be better.
>>
>> Yes. How about `sync_task_work`?
> 
> Sounds better to me.

Fine, I will change it in next version.

>>
>>>
>>>> + *
>>>> + * @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 bool ghes_do_memory_failure(u64 physical_addr, int flags)
>>>> +static void ghes_do_memory_failure(u64 physical_addr, int flags)
>>>>  {
>>>>  	unsigned long pfn;
>>>> +	struct mce_task_work *twcb;
>>>>  
>>>>  	if (!IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))
>>>> -		return false;
>>>> +		return;
>>>>  
>>>>  	pfn = PHYS_PFN(physical_addr);
>>>>  	if (!pfn_valid(pfn) && !arch_is_platform_page(physical_addr)) {
>>>>  		pr_warn_ratelimited(FW_WARN GHES_PFX
>>>>  		"Invalid address in generic error data: %#llx\n",
>>>>  		physical_addr);
>>>> -		return false;
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	if (flags == MF_ACTION_REQUIRED && current->mm) {
>>>> +		twcb = kmalloc(sizeof(*twcb), GFP_ATOMIC);
>>>> +		if (!twcb)
>>>> +			return;
>>>
>>> When this kmalloc() fails, the error event might be silently dropped?
>>> If so, some warning messages could be helpful.
>>
>> Yes, I was going to add a warning messages like:
>>
>>     pr_err("Failed to handle memory failure due to out of memory\n");
>>
>> But got a warning about patch when checked by checkpatch.pl.
>>
>>    WARNING: Possible unnecessary 'out of memory' message
>>
>> I will add it back in next version :)
> 
> Oh, I didn't know about this warning.  I checked the commit log introduced
> this meesages, and the justification makes sense to me. So I'd like to
> withdraw my comment about this (I mean you don't have to add it back).
> 
>   commit ebfdc40969f24fc0cdd1349835d36e8ebae05374                            
>   Author: Joe Perches <joe@perches.com>                                      
>   Date:   Wed Aug 6 16:10:27 2014 -0700                                      
>                                                                              
>       checkpatch: attempt to find unnecessary 'out of memory' messages       
>                                                                              
>       Logging messages that show some type of "out of memory" error are      
>       generally unnecessary as there is a generic message and a stack dump   
>       done by the memory subsystem.                                          
>                                                                              
>       These messages generally increase kernel size without much added value.

Haha, that's exactly the patch I was referring to (Sorry for forgetting to
attach a link in last reply). So I will not add the warning messages back.

> 
> Thanks,
> Naoya Horiguchi

Thank you for comments.

Cheers.
Shuai