[PATCH 0/2] Fix some machine check application recovery cases

[PATCH 0/2] Fix some machine check application recovery cases

[Tony Luck]

Tesing recovery in mult-threaded applications showed a couple
of issues in our code.

Tony Luck (2):
  memory-failure: Send right signal code to correct thread
  memory-failure: Don't let collect_procs() skip over processes for
    MF_ACTION_REQUIRED

 mm/memory-failure.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

-- 
1.8.4.1

[PATCH 1/2] memory-failure: Send right signal code to correct thread

[Tony Luck]

When a thread in a multi-threaded application hits a machine
check because of an uncorrectable error in memory - we want to
send the SIGBUS with si.si_code = BUS_MCEERR_AR to that thread.
Currently we fail to do that if the active thread is not the
primary thread in the process. collect_procs() just finds primary
threads and this test:
	if ((flags & MF_ACTION_REQUIRED) && t == current) {
will see that the thread we found isn't the current thread
and so send a si.si_code = BUS_MCEERR_AO to the primary
(and nothing to the active thread at this time).

We can fix this by checking whether "current" shares the same
mm with the process that collect_procs() said owned the page.
If so, we send the SIGBUS to current (with code BUS_MCEERR_AR).

Reported-by: Otto Bruggeman <otto.g.bruggeman@intel.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 mm/memory-failure.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 35ef28acf137..642c8434b166 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -204,9 +204,9 @@ static int kill_proc(struct task_struct *t, unsigned long addr, int trapno,
 #endif
 	si.si_addr_lsb = compound_order(compound_head(page)) + PAGE_SHIFT;
 
-	if ((flags & MF_ACTION_REQUIRED) && t == current) {
+	if ((flags & MF_ACTION_REQUIRED) && t->mm == current->mm) {
 		si.si_code = BUS_MCEERR_AR;
-		ret = force_sig_info(SIGBUS, &si, t);
+		ret = force_sig_info(SIGBUS, &si, current);
 	} else {
 		/*
 		 * Don't use force here, it's convenient if the signal
-- 
1.8.4.1

RE: [PATCH 1/2] memory-failure: Send right signal code to correct thread

[Luck, Tony]

> Looks good to me, thank you.
> Reviewed-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>

Thanks for your time reviewing this

> and I think this is worth going into stable trees.

Good point. I should dig in the git history and make one of those
fancy "Fixes: sha1 title" tags too.

-Tony
 

Re: [PATCH 1/2] memory-failure: Send right signal code to correct thread

[Chen, Gong]

[-- Attachment #1: Type: text/plain, Size: 2206 bytes --]

On Tue, May 20, 2014 at 09:28:00AM -0700, Luck, Tony wrote:
> When a thread in a multi-threaded application hits a machine
> check because of an uncorrectable error in memory - we want to
> send the SIGBUS with si.si_code = BUS_MCEERR_AR to that thread.
> Currently we fail to do that if the active thread is not the
> primary thread in the process. collect_procs() just finds primary
> threads and this test:
> 	if ((flags & MF_ACTION_REQUIRED) && t == current) {
> will see that the thread we found isn't the current thread
> and so send a si.si_code = BUS_MCEERR_AO to the primary
> (and nothing to the active thread at this time).
> 
> We can fix this by checking whether "current" shares the same
> mm with the process that collect_procs() said owned the page.
> If so, we send the SIGBUS to current (with code BUS_MCEERR_AR).
> 
> Reported-by: Otto Bruggeman <otto.g.bruggeman@intel.com>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  mm/memory-failure.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 35ef28acf137..642c8434b166 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -204,9 +204,9 @@ static int kill_proc(struct task_struct *t, unsigned long addr, int trapno,
>  #endif
>  	si.si_addr_lsb = compound_order(compound_head(page)) + PAGE_SHIFT;
>  
> -	if ((flags & MF_ACTION_REQUIRED) && t == current) {
> +	if ((flags & MF_ACTION_REQUIRED) && t->mm == current->mm) {
>  		si.si_code = BUS_MCEERR_AR;
> -		ret = force_sig_info(SIGBUS, &si, t);
> +		ret = force_sig_info(SIGBUS, &si, current);
>  	} else {
>  		/*
>  		 * Don't use force here, it's convenient if the signal
> -- 
> 1.8.4.1
Very interesting. I remembered there was a thread about AO error. Here is
the link: http://www.spinics.net/lists/linux-mm/msg66653.html.
According to this link, I have two concerns:

1) how to handle the similar scenario like it in this link. I mean once
the main thread doesn't handle AR error but a thread does this, if SIGBUS
can't be handled at once.
2) why that patch isn't merged. From that thread, Naoya should mean
"acknowledge" :-).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] memory-failure: Send right signal code to correct thread

[Tony Luck]

Added Kamil (hope I got the right one - the spinics.net archive obfuscates
the e-mail addresses).

>> -     if ((flags & MF_ACTION_REQUIRED) && t == current) {
>> +     if ((flags & MF_ACTION_REQUIRED) && t->mm == current->mm) {
>>               si.si_code = BUS_MCEERR_AR;
>> -             ret = force_sig_info(SIGBUS, &si, t);
>> +             ret = force_sig_info(SIGBUS, &si, current);
>>       } else {
>>               /*
>>                * Don't use force here, it's convenient if the signal
>> --
>> 1.8.4.1
> Very interesting. I remembered there was a thread about AO error. Here is
> the link: http://www.spinics.net/lists/linux-mm/msg66653.html.
> According to this link, I have two concerns:
>
> 1) how to handle the similar scenario like it in this link. I mean once
> the main thread doesn't handle AR error but a thread does this, if SIGBUS
> can't be handled at once.
> 2) why that patch isn't merged. From that thread, Naoya should mean
> "acknowledge" :-).

That's an interesting thread ... and looks like it helps out in a case
where there are only AO signals.

But the "AR" case complicates things. Kamil points out at the start
of the thread:
> Also, do I understand it correctly that "action required" faults *must* be
> handled by the thread that triggered the error?  I guess it makes sense for
> it to be that way, even if it circumvents the "dedicated handling thread"
> idea...
this is absolutely true ... in the BUS_MCEERR_AR case the current
thread is executing an instruction that is attempting to consume poison
data ... and we cannot let that instruction retire, so we have to signal that
thread - if it can fix the problem by mapping a new page to the location
that was lost, and refilling it with the right data - the handler can return
to resume - otherwise it can longjmp() somewhere or exit.

This means that the idea of having a multi-threaded application where
just one thread has a SIGBUS handler and we gently steer the
BUS_MCEERR_AO signals to that thread to be handled is flawed.
Every thread needs to have a SIGBUS handler - so that we can handle
the "AR" case. [Digression: what does happen to a process with a thread
with no SIGBUS handler if we in fact send it a SIGBUS? Does just that
thread die (default action for SIGBUS)? Or does the whole process get
killed?  If just one thread is terminated ... then perhaps someone could
write a recovery aware application that worked like this - though it sounds
like that would be working blindfold with one hand tied behind your back.
How would the remaining threads know why their buddy just died? The
siginfo_t describing the problem isn't available]

If we want steerable AO signals to a dedicated thread - we'd have to
use different signals for AO & AR. So every thread can have an AR
handler, but just one have the AO handler.  Or something more exotic
with prctl to designate the preferred target for AO signals?

Or just live with the fact that every thread needs a handler for AR ...
and have the application internally pass AO activity from the
thread that originally got the SIGBUS to some worker thread.

-Tony

Re: [PATCH 1/2] memory-failure: Send right signal code to correct thread

[Kamil Iskra]

On Fri, May 23, 2014 at 09:48:42 -0700, Tony Luck wrote:

Tony,

> Added Kamil (hope I got the right one - the spinics.net archive obfuscates
> the e-mail addresses).

Yes, you got the right address :-).

> >> -     if ((flags & MF_ACTION_REQUIRED) && t == current) {
> >> +     if ((flags & MF_ACTION_REQUIRED) && t->mm == current->mm) {
> >>               si.si_code = BUS_MCEERR_AR;
> >> -             ret = force_sig_info(SIGBUS, &si, t);
> >> +             ret = force_sig_info(SIGBUS, &si, current);
> >>       } else {
> >>               /*
> >>                * Don't use force here, it's convenient if the signal
> >> --
> >> 1.8.4.1
> > Very interesting. I remembered there was a thread about AO error. Here is
> > the link: http://www.spinics.net/lists/linux-mm/msg66653.html.
> > According to this link, I have two concerns:
> >
> > 1) how to handle the similar scenario like it in this link. I mean once
> > the main thread doesn't handle AR error but a thread does this, if SIGBUS
> > can't be handled at once.
> > 2) why that patch isn't merged. From that thread, Naoya should mean
> > "acknowledge" :-).
> That's an interesting thread ... and looks like it helps out in a case
> where there are only AO signals.

Unfortunately, I got distracted by other pressing work at the time and
didn't follow up on my patch/didn't follow the correct kernel workflow on
patch submission procedures.  I haven't checked any developments in that
area so I don't even know if my patch is still applicable -- do you think
it makes sense for me to revisit the issue at this time, or will the patch
that you are working on make my old patch redundant?

> But the "AR" case complicates things. Kamil points out at the start
> of the thread:
> > Also, do I understand it correctly that "action required" faults *must* be
> > handled by the thread that triggered the error?  I guess it makes sense for
> > it to be that way, even if it circumvents the "dedicated handling thread"
> > idea...
> this is absolutely true ... in the BUS_MCEERR_AR case the current
> thread is executing an instruction that is attempting to consume poison
> data ... and we cannot let that instruction retire, so we have to signal that
> thread - if it can fix the problem by mapping a new page to the location
> that was lost, and refilling it with the right data - the handler can return
> to resume - otherwise it can longjmp() somewhere or exit.

Exactly.

> This means that the idea of having a multi-threaded application where
> just one thread has a SIGBUS handler and we gently steer the
> BUS_MCEERR_AO signals to that thread to be handled is flawed.
> Every thread needs to have a SIGBUS handler - so that we can handle
> the "AR" case. [Digression: what does happen to a process with a thread
> with no SIGBUS handler if we in fact send it a SIGBUS? Does just that
> thread die (default action for SIGBUS)? Or does the whole process get
> killed?  If just one thread is terminated ... then perhaps someone could
> write a recovery aware application that worked like this - though it sounds
> like that would be working blindfold with one hand tied behind your back.
> How would the remaining threads know why their buddy just died? The
> siginfo_t describing the problem isn't available]

I believe I experimented with this and the whole process would get killed.

> If we want steerable AO signals to a dedicated thread - we'd have to
> use different signals for AO & AR. So every thread can have an AR
> handler, but just one have the AO handler.  Or something more exotic
> with prctl to designate the preferred target for AO signals?
> 
> Or just live with the fact that every thread needs a handler for AR ...
> and have the application internally pass AO activity from the
> thread that originally got the SIGBUS to some worker thread.

Yes, you make a very valid point that my patch was not complete... but
then, neither was what was there before it.  So my patch was only an
incremental improvement, enough to play with when artificially injecting
fault events, but not enough to *really* solve the problem.  If you have a
complete solution in mind instead, that would be great.

Kamil

-- 
Kamil Iskra, PhD
Argonne National Laboratory, Mathematics and Computer Science Division
9700 South Cass Avenue, Building 240, Argonne, IL 60439, USA
phone: +1-630-252-7197  fax: +1-630-252-5986

Re: [PATCH 1/2] memory-failure: Send right signal code to correct thread

[Tony Luck]

>  - make sure that every thread in a recovery aware application should have
>    a SIGBUS handler, inside which
>    * code for SIGBUS(BUS_MCEERR_AR) is enabled for every thread
>    * code for SIGBUS(BUS_MCEERR_AO) is enabled only for a dedicated thread

But how does the kernel know which is the special thread that
should see the "AO" signal?  Broadcasting the signal to all
threads seems to be just as likely to cause problems to
an application as the h/w broadcasting MCE to all processors.

-Tony

Re: [PATCH 1/2] memory-failure: Send right signal code to correct thread

[Tony Luck]

I'm exploring options to see what writers of threaded applications might want/need. I'm very doubtful that they would really want "broadcast to all threads". What if there are hundreds or thousands of threads? We send the signals from the context of the thread that hit the error. But that might take a while. Meanwhile any of those threads that were already scheduled on other CPUs are back running again. So there are big races even if we broadcast.

Sent from my iPhone

> On May 27, 2014, at 17:15, Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> wrote:
> 
> On Tue, May 27, 2014 at 03:53:55PM -0700, Tony Luck wrote:
>>> - make sure that every thread in a recovery aware application should have
>>>   a SIGBUS handler, inside which
>>>   * code for SIGBUS(BUS_MCEERR_AR) is enabled for every thread
>>>   * code for SIGBUS(BUS_MCEERR_AO) is enabled only for a dedicated thread
>> 
>> But how does the kernel know which is the special thread that
>> should see the "AO" signal?  Broadcasting the signal to all
>> threads seems to be just as likely to cause problems to
>> an application as the h/w broadcasting MCE to all processors.
> 
> I thought that kernel doesn't have to know about which thread is the
> special one if the AO signal is broadcasted to all threads, because
> in such case the special thread always gets the AO signal.
> 
> The reported problem happens only the application sets PF_MCE_EARLY flag,
> and such application is surely recovery aware, so we can assume that the
> coders must implement SIGBUS handler for all threads. Then all other threads
> but the special one can intentionally ignore AO signal. This is to avoid the
> default behavior for SIGBUS ("kill all threads" as Kamil said in the previous
> email.)
> 
> And I hope that downside of signal broadcasting is smaller than MCE
> broadcasting because the range of broadcasting is limited to a process group,
> not to the whole system.
> 
> # I don't intend to rule out other possibilities like adding another prctl
> # flag, so if you have a patch, that's would be great.
> 
> Thanks,
> Naoya Horiguchi

Re: [PATCH] mm/memory-failure.c: support dedicated thread to handle SIGBUS(BUS_MCEERR_AO) thread

[Tony Luck]

On Wed, May 28, 2014 at 11:47 AM, Naoya Horiguchi
<n-horiguchi@ah.jp.nec.com> wrote:
> Could you take a look?

It looks good - and should be a workable API for
application writers to use.

> @@ -84,6 +84,11 @@ PR_MCE_KILL
>                 PR_MCE_KILL_EARLY: Early kill
>                 PR_MCE_KILL_LATE:  Late kill
>                 PR_MCE_KILL_DEFAULT: Use system global default
> +       Note that if you want to have a dedicated thread which handles
> +       the SIGBUS(BUS_MCEERR_AO) on behalf of the process, you should
> +       call prctl() on the thread. Otherwise, the SIGBUS is sent to
> +       the main thread.

Perhaps be more explicit here that the user should call
prctl(PR_MCE_KILL_EARLY) on the designated thread
to get this behavior?  The user could also mark more than
one thread in this way - in which case the kernel will pick
the first one it sees (is that oldest, or newest?) that is marked.
Not sure if this would ever be useful unless you want to pass
responsibility around in an application that is dynamically
creating and removing threads.

> +               if (t->flags & PF_MCE_PROCESS && t->flags & PF_MCE_EARLY)

This is correct - but made me twitch to add extra brackets:

                  if ((t->flags & PF_MCE_PROCESS) && (t->flags & PF_MCE_EARLY))

or
                  if ((t->flags & (PF_MCE_PROCESS|PF_MCE_EARLY)) ==
PF_MCE_PROCESS|PF_MCE_EARLY)

[oops, no ... that's too long and no clearer]

-Tony

Re: [PATCH] mm/memory-failure.c: support dedicated thread to handle SIGBUS(BUS_MCEERR_AO) thread

[Tony Luck]

> OK, I'll take this.

If you didn't already apply it, then add a "Reviewed-by: Tony Luck
<tony.luck@intel,com>"

I see that this patch is on top of my earlier ones (includes the
"force_early" argument).
That means you have both of those queued too?

Thanks

-Tony

Re: [PATCH] mm/memory-failure.c: support dedicated thread to handle SIGBUS(BUS_MCEERR_AO) thread

[Kamil Iskra]

On Wed, May 28, 2014 at 21:45:41 -0400, Naoya Horiguchi wrote:

> >  The user could also mark more than
> > one thread in this way - in which case the kernel will pick
> > the first one it sees (is that oldest, or newest?) that is marked.
> > Not sure if this would ever be useful unless you want to pass
> > responsibility around in an application that is dynamically
> > creating and removing threads.
> 
> I'm not sure which is better to send signal to first-found marked thread
> or to all marked threads. If we have a good reason to do the latter,
> I'm ok about it. Any idea?

Well, it would be more flexible if the signal were sent to all marked
threads, but I don't know if that constitutes a good enough reason to add
the extra complexity involved.  Sometimes better is the enemy of good, and
in this case the patch you proposed should be good enough for any practical
case I can think of.

Naoya, Tony, thank you for taking the leadership on this issue and seeing
it through, and for the courtesy of keeping me in the loop!

Kamil

-- 
Kamil Iskra, PhD
Argonne National Laboratory, Mathematics and Computer Science Division
9700 South Cass Avenue, Building 240, Argonne, IL 60439, USA
phone: +1-630-252-7197  fax: +1-630-252-5986

[PATCH 2/2] memory-failure: Don't let collect_procs() skip over processes for MF_ACTION_REQUIRED

[Tony Luck]

When Linux sees an "action optional" machine check (where h/w has
reported an error that is not in the current execution path) we
generally do not want to signal a process, since most processes
do not have a SIGBUS handler - we'd just prematurely terminate the
process for a problem that they might never actually see.

task_early_kill() decides whether to consider a process - and it
checks whether this specific process has been marked for early signals
with "prctl", or if the system administrator has requested early
signals for all processes using /proc/sys/vm/memory_failure_early_kill.

But for MF_ACTION_REQUIRED case we must not defer. The error is in
the execution path of the current thread so we must send the SIGBUS
immediatley.

Fix by passing a flag argument through collect_procs*() to
task_early_kill() so it knows whether we can defer or must
take action.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 mm/memory-failure.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 642c8434b166..f0967f72991c 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -380,10 +380,12 @@ static void kill_procs(struct list_head *to_kill, int forcekill, int trapno,
 	}
 }
 
-static int task_early_kill(struct task_struct *tsk)
+static int task_early_kill(struct task_struct *tsk, int force_early)
 {
 	if (!tsk->mm)
 		return 0;
+	if (force_early)
+		return 1;
 	if (tsk->flags & PF_MCE_PROCESS)
 		return !!(tsk->flags & PF_MCE_EARLY);
 	return sysctl_memory_failure_early_kill;
@@ -393,7 +395,7 @@ static int task_early_kill(struct task_struct *tsk)
  * Collect processes when the error hit an anonymous page.
  */
 static void collect_procs_anon(struct page *page, struct list_head *to_kill,
-			      struct to_kill **tkc)
+			      struct to_kill **tkc, int force_early)
 {
 	struct vm_area_struct *vma;
 	struct task_struct *tsk;
@@ -409,7 +411,7 @@ static void collect_procs_anon(struct page *page, struct list_head *to_kill,
 	for_each_process (tsk) {
 		struct anon_vma_chain *vmac;
 
-		if (!task_early_kill(tsk))
+		if (!task_early_kill(tsk, force_early))
 			continue;
 		anon_vma_interval_tree_foreach(vmac, &av->rb_root,
 					       pgoff, pgoff) {
@@ -428,7 +430,7 @@ static void collect_procs_anon(struct page *page, struct list_head *to_kill,
  * Collect processes when the error hit a file mapped page.
  */
 static void collect_procs_file(struct page *page, struct list_head *to_kill,
-			      struct to_kill **tkc)
+			      struct to_kill **tkc, int force_early)
 {
 	struct vm_area_struct *vma;
 	struct task_struct *tsk;
@@ -439,7 +441,7 @@ static void collect_procs_file(struct page *page, struct list_head *to_kill,
 	for_each_process(tsk) {
 		pgoff_t pgoff = page->index << (PAGE_CACHE_SHIFT - PAGE_SHIFT);
 
-		if (!task_early_kill(tsk))
+		if (!task_early_kill(tsk, force_early))
 			continue;
 
 		vma_interval_tree_foreach(vma, &mapping->i_mmap, pgoff,
@@ -465,7 +467,8 @@ static void collect_procs_file(struct page *page, struct list_head *to_kill,
  * First preallocate one tokill structure outside the spin locks,
  * so that we can kill at least one process reasonably reliable.
  */
-static void collect_procs(struct page *page, struct list_head *tokill)
+static void collect_procs(struct page *page, struct list_head *tokill,
+				int force_early)
 {
 	struct to_kill *tk;
 
@@ -476,9 +479,9 @@ static void collect_procs(struct page *page, struct list_head *tokill)
 	if (!tk)
 		return;
 	if (PageAnon(page))
-		collect_procs_anon(page, tokill, &tk);
+		collect_procs_anon(page, tokill, &tk, force_early);
 	else
-		collect_procs_file(page, tokill, &tk);
+		collect_procs_file(page, tokill, &tk, force_early);
 	kfree(tk);
 }
 
@@ -963,7 +966,7 @@ static int hwpoison_user_mappings(struct page *p, unsigned long pfn,
 	 * there's nothing that can be done.
 	 */
 	if (kill)
-		collect_procs(ppage, &tokill);
+		collect_procs(ppage, &tokill, flags & MF_ACTION_REQUIRED);
 
 	ret = try_to_unmap(ppage, ttu);
 	if (ret != SWAP_SUCCESS)
-- 
1.8.4.1