Re: [PATCH v2] mm,hwpoison:Differentiate Action Required error in current and non current process

From: Aili Yao <yaoaili@kingsoft.com>
To: "HORIGUCHI NAOYA(堀口　直也)" <naoya.horiguchi@nec.com>
Cc: "linux-mm@kvack.org" <linux-mm@kvack.org>,
	"osalvador@suse.de" <osalvador@suse.de>,
	"YANGFENG1@kingsoft.com" <YANGFENG1@kingsoft.com>
Subject: Re: [PATCH v2] mm,hwpoison:Differentiate Action Required error in current and non current process
Date: Wed, 20 Jan 2021 14:07:29 +0800	[thread overview]
Message-ID: <20210120140729.66d6cab5.yaoaili@kingsoft.com> (raw)
In-Reply-To: <20210120000614.GA22376@hori.linux.bs1.fc.nec.co.jp>

On Wed, 20 Jan 2021 00:06:14 +0000
HORIGUCHI NAOYA(堀口　直也) <naoya.horiguchi@nec.com> wrote:

> You don't have to add these header info into patch description because
> git can get them from email headers.
> # scripts/checkpatch.pl shows some warning due to these.

yes, i will change this. Thanks.

> >  			if (fail || tk->addr == -EFAULT) {
> > -				pr_err("Memory failure: %#lx: forcibly killing %s:%d because of failure to unmap corrupted page\n",
> > -				       pfn, tk->tsk->comm, tk->tsk->pid);
> > -				do_send_sig_info(SIGKILL, SEND_SIG_PRIV,
> > +				if (tk->tsk == current) {
> > +					pr_err("Memory failure: %#lx: forcibly killing %s:%d because of failure to unmap corrupted page\n",
> > +						pfn, tk->tsk->comm, tk->tsk->pid);
> > +					do_send_sig_info(SIGKILL, SEND_SIG_PRIV,
> >  						 tk->tsk, PIDTYPE_PID);
> > +				} else if (kill_proc(tk, pfn, flags) < 0)
> > +					pr_err("Memory failure: %#lx: Cannot send advisory machine check signal to %s:%d\n",
> > +						pfn, tk->tsk->comm, tk->tsk->pid);  
> 
> This seems to change (maybe beyond the intent of this patch) the behavior of
> action optional events, i.e. without this patch all processes on to_kill list
> receive SIGKILL.  but with patch SIGKILL is sent only to current (if it
> happens to be linked to to_kill list) and any other processes on the list
> receive SIGBUSs. Any justification on this change?
> 
> This code path is for failure in error handling, where we can't do anything
> more.  So I think that just killing all affected processes with SIGKILL as
> we do now is the expected behavior.

you are right, I have some misunderstanding here, we need to keep the original code. Thanks for correction

> >  			/*
> > @@ -457,8 +464,6 @@ static struct task_struct *task_early_kill(struct task_struct *tsk,
> >  		 */
> >  		if (tsk->mm == current->mm)
> >  			return current;
> > -		else
> > -			return NULL;  
> 
> Please update the comment above this function about action required case.

yes, I will add some comments here! I will submit a v3 patch.

Thank you!
-- 
Best Regards!

Aili Yao