Re: [PATCH v5] mm,hwpoison: send SIGBUS to PF_MCE_EARLY processes on action required events

[Aili Yao <yaoaili@kingsoft.com>]

On Thu, 21 Jan 2021 11:03:05 +0100
Oscar Salvador <osalvador@suse.de> wrote:

> On Wed, Jan 20, 2021 at 04:24:22PM +0800, Aili Yao wrote:
> > When a memory uncorrected error is triggered by process who accessed
> > the address with error, It's Action Required Case for only current
> > process which triggered this; This Action Required case means Action
> > optional to other process who share the same page. Usually killing
> > current process will be sufficient, other processes sharing the same
> > page will get be signaled when they really touch the poisoned page.
> > 
> > But there is another scenario that other processes
> > sharing the same page want to be signaled early with PF_MCE_EARLY set,
> > In this case, we should get them into kill list and signal
> > BUS_MCEERR_AO to them.
> > 
> > So in this patch, task_early_kill will check current process if
> > force_early is set, and if not current,the code will fallback to
> > find_early_kill_thread() to check if there is PF_MCE_EARLY process
> > who cares the error.
> > 
> > In kill_proc(), BUS_MCEERR_AR is only send to current, other processes in
> > kill list will be signaled with BUS_MCEERR_AO.
> > 
> > Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
> > Signed-off-by: Aili Yao <yaoaili@kingsoft.com>  
> 
> Looks good to me, a few nits below.
> 
> Reviewed-by: Oscar Salvador <osalvador@suse.de>
> 
> 
> > @@ -243,9 +243,12 @@ static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags)
> >  			pfn, t->comm, t->pid);
> >  
> >  	if (flags & MF_ACTION_REQUIRED) {
> > -		WARN_ON_ONCE(t != current);
> > -		ret = force_sig_mceerr(BUS_MCEERR_AR,
> > +		if (tk->tsk == current)  
> You can re-use "t" here.

yeah, this look better, I will change to that. Thanks!

> 
> > +			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);  
> 
> I would place a brief comment above explaining why we are sending BUS_MCEER_AO
> to non-current tasks.
> E.g: "Signal other processes sharing the page if they have PF_MCE_EARLY set"

Yes, it's good to have

> 
> > @@ -457,8 +463,6 @@ static struct task_struct *task_early_kill(struct task_struct *tsk,
> >  		 */
> >  		if (tsk->mm == current->mm)
> >  			return current;
> > -		else
> > -			return NULL;  
> 
>  if (force_early && task->mm == current->mm)
>          return current;
>  
> 
This looks more cleaner.

I will modify above in a v6 patch!

Thanks,

-- 
Best Regards!

Aili Yao