[PATCH] mm/memory-failure.c: send "action optional" signal to an arbitrary thread

[PATCH] mm/memory-failure.c: send "action optional" signal to an arbitrary thread

[Kamil Iskra]

Please find below a trivial patch that changes the sending of BUS_MCEERR_AO
SIGBUS signals so that they can be handled by an arbitrary thread of the
target process.  The current implementation makes it impossible to create a
separate, dedicated thread to handle such errors, as the signal is always
sent to the main 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...

The patch is against the 3.12.4 kernel.

--- mm/memory-failure.c.orig	2013-12-08 10:18:58.000000000 -0600
+++ mm/memory-failure.c	2013-12-12 11:43:03.973334767 -0600
@@ -219,7 +219,7 @@ static int kill_proc(struct task_struct
 		 * to SIG_IGN, but hopefully no one will do that?
 		 */
 		si.si_code = BUS_MCEERR_AO;
-		ret = send_sig_info(SIGBUS, &si, t);  /* synchronous? */
+		ret = group_send_sig_info(SIGBUS, &si, t);  /* synchronous? */
 	}
 	if (ret < 0)
 		printk(KERN_INFO "MCE: Error sending signal to %s:%d: %d\n",

Thanks,

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/memory-failure.c: send action optional signal to an arbitrary thread

[Naoya Horiguchi]

Hi Kamil,

# Cced: Andi

On Thu, Dec 12, 2013 at 04:25:27PM -0600, Kamil Iskra wrote:
> Please find below a trivial patch that changes the sending of BUS_MCEERR_AO
> SIGBUS signals so that they can be handled by an arbitrary thread of the
> target process.  The current implementation makes it impossible to create a
> separate, dedicated thread to handle such errors, as the signal is always
> sent to the main thread.

This can be done in application side by letting the main thread create a
dedicated thread for error handling, or by waking up existing/sleeping one.
It might not be optimal in overhead, but note that an action optional error
does not require to be handled ASAP. And we need only one process to handle
an action optional error, so no need to send SIGBUS(BUS_MCEERR_AO) for every
processes/threads.

> 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...

Yes. Unlike action optional errors, action required faults can happen on
all processes/threads which map the error affected page, so in memory error
aware applications every thread must be able to handle SIGBUS(BUS_MCEERR_AR)
or just be killed.

> The patch is against the 3.12.4 kernel.
> 
> --- mm/memory-failure.c.orig	2013-12-08 10:18:58.000000000 -0600
> +++ mm/memory-failure.c	2013-12-12 11:43:03.973334767 -0600
> @@ -219,7 +219,7 @@ static int kill_proc(struct task_struct
>  		 * to SIG_IGN, but hopefully no one will do that?
>  		 */
>  		si.si_code = BUS_MCEERR_AO;
> -		ret = send_sig_info(SIGBUS, &si, t);  /* synchronous? */
> +		ret = group_send_sig_info(SIGBUS, &si, t);  /* synchronous? */

Personally, I don't think we need this change for the above mentioned reason.
And another concern is if this change can affect/break existing applications.
If it can, maybe you need to add (for example) a prctl attribute to show that
the process expects kernel to send SIGBUS(BUS_MCEERR_AO) only to the main
thread, or to all threads belonging to the process.

Thanks,
Naoya Horiguchi

>  	}
>  	if (ret < 0)
>  		printk(KERN_INFO "MCE: Error sending signal to %s:%d: %d\n",
> 
> Thanks,
> 
> 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
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/memory-failure.c: send action optional signal to an arbitrary thread

[Naoya Horiguchi]

On Fri, Dec 13, 2013 at 02:59:02PM -0500, Naoya Horiguchi wrote:
> Hi Kamil,
> 
> # Cced: Andi
> 
> On Thu, Dec 12, 2013 at 04:25:27PM -0600, Kamil Iskra wrote:
> > Please find below a trivial patch that changes the sending of BUS_MCEERR_AO
> > SIGBUS signals so that they can be handled by an arbitrary thread of the
> > target process.  The current implementation makes it impossible to create a
> > separate, dedicated thread to handle such errors, as the signal is always
> > sent to the main thread.
> 
> This can be done in application side by letting the main thread create a
> dedicated thread for error handling, or by waking up existing/sleeping one.
> It might not be optimal in overhead, but note that an action optional error
> does not require to be handled ASAP.

> And we need only one process to handle
> an action optional error, so no need to send SIGBUS(BUS_MCEERR_AO) for every
> processes/threads.

Sorry, let me correct the above: "We need only one thread (not one process)
to handle an action optional error."

Thanks,
Naoya

> 
> > 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...
> 
> Yes. Unlike action optional errors, action required faults can happen on
> all processes/threads which map the error affected page, so in memory error
> aware applications every thread must be able to handle SIGBUS(BUS_MCEERR_AR)
> or just be killed.
> 
> > The patch is against the 3.12.4 kernel.
> > 
> > --- mm/memory-failure.c.orig	2013-12-08 10:18:58.000000000 -0600
> > +++ mm/memory-failure.c	2013-12-12 11:43:03.973334767 -0600
> > @@ -219,7 +219,7 @@ static int kill_proc(struct task_struct
> >  		 * to SIG_IGN, but hopefully no one will do that?
> >  		 */
> >  		si.si_code = BUS_MCEERR_AO;
> > -		ret = send_sig_info(SIGBUS, &si, t);  /* synchronous? */
> > +		ret = group_send_sig_info(SIGBUS, &si, t);  /* synchronous? */
> 
> Personally, I don't think we need this change for the above mentioned reason.
> And another concern is if this change can affect/break existing applications.
> If it can, maybe you need to add (for example) a prctl attribute to show that
> the process expects kernel to send SIGBUS(BUS_MCEERR_AO) only to the main
> thread, or to all threads belonging to the process.
> 
> Thanks,
> Naoya Horiguchi
> 
> >  	}
> >  	if (ret < 0)
> >  		printk(KERN_INFO "MCE: Error sending signal to %s:%d: %d\n",
> > 
> > Thanks,
> > 
> > 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
> > 
> > --
> > To unsubscribe, send a message with 'unsubscribe linux-mm' in
> > the body to majordomo@kvack.org.  For more info on Linux MM,
> > see: http://www.linux-mm.org/ .
> > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> > 
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/memory-failure.c: send action optional signal to an arbitrary thread

[Kamil Iskra]

On Fri, Dec 13, 2013 at 14:59:02 -0500, Naoya Horiguchi wrote:

Hi Naoya,

> On Thu, Dec 12, 2013 at 04:25:27PM -0600, Kamil Iskra wrote:
> > Please find below a trivial patch that changes the sending of BUS_MCEERR_AO
> > SIGBUS signals so that they can be handled by an arbitrary thread of the
> > target process.  The current implementation makes it impossible to create a
> > separate, dedicated thread to handle such errors, as the signal is always
> > sent to the main thread.
> This can be done in application side by letting the main thread create a
> dedicated thread for error handling, or by waking up existing/sleeping one.
> It might not be optimal in overhead, but note that an action optional error
> does not require to be handled ASAP. And we need only one process to handle
> an action optional error, so no need to send SIGBUS(BUS_MCEERR_AO) for every
> processes/threads.

I'm not sure if I understand.  "letting the main thread create a dedicated
thread for error handling" is exactly what I was trying to do -- the
problem is that SIGBUS(BUS_MCEERR_AO) signals are never sent to that
thread, which is contrary to common expectations.  The signals are sent to
the main thread only, even if SIGBUS is masked there.

Just to make sure that we're on the same page, here's a testcase that
demonstrates the problem I'm trying to fix (I should've sent it the first
time; sorry for being lazy):


#include <pthread.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/prctl.h>
#include <unistd.h>

void sigbus_handler(int sig, siginfo_t* si, void* ucontext)
{
    printf("SIGBUS caught by thread %ld, code %d, addr %p\n",
	   (long)pthread_self(), si->si_code, si->si_addr);
}

void* sigbus_thread(void* arg)
{
    printf("sigbus thread: %ld\n", (long)pthread_self());
    for (;;)
	pause();
}

int main(void)
{
    struct sigaction sa;
    sigset_t mask;
    char* buf;
    pthread_t thread_id;

    prctl(PR_MCE_KILL, PR_MCE_KILL_SET, PR_MCE_KILL_EARLY, 0, 0);

    posix_memalign((void*)&buf, 4096, 4096);
    buf[0] = 0;
    printf("convenient address to hard offline: %p\n", buf);

    sa.sa_sigaction = sigbus_handler;
    sigemptyset(&sa.sa_mask);
    sa.sa_flags = SA_SIGINFO;
    sigaction(SIGBUS, &sa, NULL);

    pthread_create(&thread_id, NULL, sigbus_thread, NULL);

    sigemptyset(&mask);
    sigaddset(&mask, SIGBUS);
    pthread_sigmask(SIG_BLOCK, &mask, NULL);

    printf("main thread: %ld\n", (long)pthread_self());

    for (;;)
	pause();

    return 0;
}


This testcase uses a very common signal handling strategy in multithreaded
programs: masking signals in all threads but one, created specifically for
signal handling.  It works just fine if I send it SIGBUS from another
terminal using "kill".  It does not work if I offline the page: the signal
is routed to the main thread, where it's marked as pending; nothing gets
printed out.

As you were so kind to point out, SIGBUS(BUS_MCEERR_AO) does not need to be
handled ASAP, so why should the kernel handle it differently to other
non-critical signals?  The current behavior seems inconsistent, and there
is no convenient workaround (as a library writer, I have no control over
the actions of the main thread).

> And another concern is if this change can affect/break existing applications.
> If it can, maybe you need to add (for example) a prctl attribute to show that
> the process expects kernel to send SIGBUS(BUS_MCEERR_AO) only to the main
> thread, or to all threads belonging to the process.

I understand your concern.  However, I believe that having
SIGBUS(BUS_MCEERR_AO) behave consistently with established POSIX standards
for signal handling outhweighs the concerns over potential
incompatibilities, especially with a feature that is currently used by a
very small subset of applications.

Thus, I kindly ask you to reconsider.

Regards,

Kamil

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/memory-failure.c: send action optional signal to an arbitrary thread

[Naoya Horiguchi]

Kamil,

# Sorry for late response.

On Fri, Dec 13, 2013 at 05:00:04PM -0600, Kamil Iskra wrote:
> On Fri, Dec 13, 2013 at 14:59:02 -0500, Naoya Horiguchi wrote:
> 
> Hi Naoya,
> 
> > On Thu, Dec 12, 2013 at 04:25:27PM -0600, Kamil Iskra wrote:
> > > Please find below a trivial patch that changes the sending of BUS_MCEERR_AO
> > > SIGBUS signals so that they can be handled by an arbitrary thread of the
> > > target process.  The current implementation makes it impossible to create a
> > > separate, dedicated thread to handle such errors, as the signal is always
> > > sent to the main thread.
> > This can be done in application side by letting the main thread create a
> > dedicated thread for error handling, or by waking up existing/sleeping one.
> > It might not be optimal in overhead, but note that an action optional error
> > does not require to be handled ASAP. And we need only one process to handle
> > an action optional error, so no need to send SIGBUS(BUS_MCEERR_AO) for every
> > processes/threads.
> 
> I'm not sure if I understand.  "letting the main thread create a dedicated
> thread for error handling" is exactly what I was trying to do -- the
> problem is that SIGBUS(BUS_MCEERR_AO) signals are never sent to that
> thread, which is contrary to common expectations.  The signals are sent to
> the main thread only, even if SIGBUS is masked there.

I think that what your patch suggests is that "letting the dedicated thread
get SIGBUS(BUS_MCEERR_AO) directly (not via the main thread) from kernel."
It's a bit different from what I meant in the previous email.

> Just to make sure that we're on the same page, here's a testcase that
> demonstrates the problem I'm trying to fix (I should've sent it the first
> time; sorry for being lazy):

Thanks. And I see your problem.

> 
> #include <pthread.h>
> #include <signal.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <sys/prctl.h>
> #include <unistd.h>
> 
> void sigbus_handler(int sig, siginfo_t* si, void* ucontext)
> {
>     printf("SIGBUS caught by thread %ld, code %d, addr %p\n",
> 	   (long)pthread_self(), si->si_code, si->si_addr);
> }
> 
> void* sigbus_thread(void* arg)
> {
>     printf("sigbus thread: %ld\n", (long)pthread_self());
>     for (;;)
> 	pause();
> }
> 
> int main(void)
> {
>     struct sigaction sa;
>     sigset_t mask;
>     char* buf;
>     pthread_t thread_id;
> 
>     prctl(PR_MCE_KILL, PR_MCE_KILL_SET, PR_MCE_KILL_EARLY, 0, 0);
> 
>     posix_memalign((void*)&buf, 4096, 4096);
>     buf[0] = 0;
>     printf("convenient address to hard offline: %p\n", buf);
> 
>     sa.sa_sigaction = sigbus_handler;
>     sigemptyset(&sa.sa_mask);
>     sa.sa_flags = SA_SIGINFO;
>     sigaction(SIGBUS, &sa, NULL);
> 
>     pthread_create(&thread_id, NULL, sigbus_thread, NULL);
> 
>     sigemptyset(&mask);
>     sigaddset(&mask, SIGBUS);
>     pthread_sigmask(SIG_BLOCK, &mask, NULL);
> 
>     printf("main thread: %ld\n", (long)pthread_self());
> 
>     for (;;)
> 	pause();
> 
>     return 0;
> }
> 
> 
> This testcase uses a very common signal handling strategy in multithreaded
> programs: masking signals in all threads but one, created specifically for
> signal handling.  It works just fine if I send it SIGBUS from another
> terminal using "kill".  It does not work if I offline the page: the signal
> is routed to the main thread, where it's marked as pending; nothing gets
> printed out.
> 
> As you were so kind to point out, SIGBUS(BUS_MCEERR_AO) does not need to be
> handled ASAP, so why should the kernel handle it differently to other
> non-critical signals?  The current behavior seems inconsistent, and there
> is no convenient workaround (as a library writer, I have no control over
> the actions of the main thread).

I'm not sure if current implementation is intentional or not,
but I understand about the inconsistency.

> > And another concern is if this change can affect/break existing applications.
> > If it can, maybe you need to add (for example) a prctl attribute to show that
> > the process expects kernel to send SIGBUS(BUS_MCEERR_AO) only to the main
> > thread, or to all threads belonging to the process.
> 
> I understand your concern.  However, I believe that having
> SIGBUS(BUS_MCEERR_AO) behave consistently with established POSIX standards
> for signal handling outhweighs the concerns over potential
> incompatibilities, especially with a feature that is currently used by a
> very small subset of applications.

OK, and in this case the effect on existing multi-threaded applications seems
to be small (just small degradation of availability, but no kernel panic nor
data lost,) so I think it's acceptable.

I want to agree with your patch, so could you repost the patch with patch
description?  git-format-patch will help you.

Thanks,
Naoya Horiguchi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/memory-failure.c: send action optional signal to an arbitrary thread

[Andi Kleen]

> I'm not sure if I understand.  "letting the main thread create a dedicated
> thread for error handling" is exactly what I was trying to do -- the
> problem is that SIGBUS(BUS_MCEERR_AO) signals are never sent to that
> thread, which is contrary to common expectations.  


Yes handling AO errors like this was the intended way 

I thought I had tested it at some point and intentionally changed the 
signal checking for this case (because normally SIGBUS cannot be
blocked). Anyways if it doesn't work it's definitely a bug.

If you fix it please make sure to add the test case to mce-test.

-Andi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/memory-failure.c: send action optional signal to an arbitrary thread

[Chen, Gong]

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

On Wed, Dec 18, 2013 at 07:45:15AM +0100, Andi Kleen wrote:
> Date: Wed, 18 Dec 2013 07:45:15 +0100
> From: Andi Kleen <andi@firstfloor.org>
> To: Kamil Iskra <iskra@mcs.anl.gov>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>, linux-mm@kvack.org, Andi
>  Kleen <andi@firstfloor.org>
> Subject: Re: [PATCH] mm/memory-failure.c: send action optional signal to an
>  arbitrary thread
> User-Agent: Mutt/1.5.20 (2009-06-14)
> 
> > I'm not sure if I understand.  "letting the main thread create a dedicated
> > thread for error handling" is exactly what I was trying to do -- the
> > problem is that SIGBUS(BUS_MCEERR_AO) signals are never sent to that
> > thread, which is contrary to common expectations.  
> 
Please add this section in your patch commit.

> 
> Yes handling AO errors like this was the intended way 
> 
> I thought I had tested it at some point and intentionally changed the 
> signal checking for this case (because normally SIGBUS cannot be
> blocked). Anyways if it doesn't work it's definitely a bug.
> 
> If you fix it please make sure to add the test case to mce-test.
> 

Yes, I think you can update your test case and add it in mce-test.
If you can't find latest mce-test git tree, here it is:
git://git.kernel.org/pub/scm/linux/kernel/git/gong.chen/mce-test.git

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