[PATCH] RFC: s390: Move get_signal_to_deliver() up in do_signal

[PATCH] RFC: s390: Move get_signal_to_deliver() up in do_signal

[Serge E. Hallyn]

The current placement of get_signal_to_deliver() means that
try_to_freeze() in get_signal_to_deliver() will happen after
regs->psw.addr, regs->svcnr, and regs->gprs[2] may have been
mangled.  Since the app may get checkpointed while frozen and
then restarted, this means we have to attempt a complicated
and subtle re-calculation of the initial conditions.

If we just move the get_signal_to_deliver() above the
immediately preceding block, we enourmously simplify the
arch-specific checkpoint/restart code.

A full ltp run seems to show no regressions do to this move,
though I'm not familiar enough with the entry_64.S code in
particular to be absolutely confident.

Is this a bad idea?

Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 arch/s390/kernel/signal.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/s390/kernel/signal.c b/arch/s390/kernel/signal.c
index 1675c48..7dbd618 100644
--- a/arch/s390/kernel/signal.c
+++ b/arch/s390/kernel/signal.c
@@ -442,6 +442,10 @@ void do_signal(struct pt_regs *regs)
 	else
 		oldset = &current->blocked;
 
+	/* Get signal to deliver.  When running under ptrace, at this point
+	   the debugger may change all our registers ... */
+	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
+
 	/* Are we from a system call? */
 	if (regs->svcnr) {
 		continue_addr = regs->psw.addr;
@@ -463,10 +467,6 @@ void do_signal(struct pt_regs *regs)
 		regs->svcnr = 0;	/* Don't deal with this again. */
 	}
 
-	/* Get signal to deliver.  When running under ptrace, at this point
-	   the debugger may change all our registers ... */
-	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
-
 	/* Depending on the signal settings we may need to revert the
 	   decision to restart the system call. */
 	if (signr > 0 && regs->psw.addr == restart_addr) {
-- 
1.6.1

Re: [PATCH] RFC: s390: Move get_signal_to_deliver() up in do_signal

[Martin Schwidefsky]

On Wed, 10 Feb 2010 14:40:19 -0600
"Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> wrote:

> The current placement of get_signal_to_deliver() means that
> try_to_freeze() in get_signal_to_deliver() will happen after
> regs->psw.addr, regs->svcnr, and regs->gprs[2] may have been
> mangled.  Since the app may get checkpointed while frozen and
> then restarted, this means we have to attempt a complicated
> and subtle re-calculation of the initial conditions.
> 
> If we just move the get_signal_to_deliver() above the
> immediately preceding block, we enourmously simplify the
> arch-specific checkpoint/restart code.
> 
> A full ltp run seems to show no regressions do to this move,
> though I'm not familiar enough with the entry_64.S code in
> particular to be absolutely confident.
> 
> Is this a bad idea?

I think it is a bad idea. The comment of get_signal_to_deliver tells
you that the debugger is invoked and may want to change the registers.
If the get_signal_to_deliver calls is moved then the debugger sees
the unmodified registers which is imho wrong. A comparison of the
gdb testsuite with and without the patch will tell us more.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [PATCH] RFC: s390: Move get_signal_to_deliver() up in do_signal

[Serge E. Hallyn]

Quoting Martin Schwidefsky (schwidefsky-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org):
> On Wed, 10 Feb 2010 14:40:19 -0600
> "Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > The current placement of get_signal_to_deliver() means that
> > try_to_freeze() in get_signal_to_deliver() will happen after
> > regs->psw.addr, regs->svcnr, and regs->gprs[2] may have been
> > mangled.  Since the app may get checkpointed while frozen and
> > then restarted, this means we have to attempt a complicated
> > and subtle re-calculation of the initial conditions.
> > 
> > If we just move the get_signal_to_deliver() above the
> > immediately preceding block, we enourmously simplify the
> > arch-specific checkpoint/restart code.
> > 
> > A full ltp run seems to show no regressions do to this move,
> > though I'm not familiar enough with the entry_64.S code in
> > particular to be absolutely confident.
> > 
> > Is this a bad idea?
> 
> I think it is a bad idea. The comment of get_signal_to_deliver tells
> you that the debugger is invoked and may want to change the registers.
> If the get_signal_to_deliver calls is moved then the debugger sees
> the unmodified registers which is imho wrong. A comparison of the
> gdb testsuite with and without the patch will tell us more.

Right, but I guess what's confounding me is exactly why the values
being set for the debugger make more sense to the debugger than the
initial ones.  Note that they're not actually the same as they will
be upon exit, for instance in the -ERESTARTNOHAND case if certain
signals are delivered we'll change psw.addr back after all and set
-EINTR.

So yeah, with this patch, if I send a signal to a program being
debugged and then do 'i r' I see -516 instead of the -4 which I
otherwise see, and a different $pswa.  Results for 'sleep' (which
is ERESTART_RESTARTBLOCK) and 'getchar' (which is not) being
interupted is below.  Frankly I think the info you see with the
patch is more informative, not less, and the debugger certainly
functions as well as it did before.

Of course there is probably fancier userspace tracing/debugging
code out there which depends on the current behavior?  And the
most convincing argument might be that it's all so magical that
changing it is begging for trouble.

But it sure would simplify checkpoint.

thanks,
-serge

================================================================
Results without signals patch
================================================================
sleeping program control-c'd under gdb:

(gdb) i r
r0             0x3ff00000000	4393751543808
r1             0x4d7dbeedb0	332822146480
r2             0xfffffffffffffffc	-4
r3             0x3ffff806608	4398038148616
r4             0x0	0
r5             0x8	8
r6             0x80002b40	2147494720
r7             0x3ffff8068e0	4398038149344
r8             0x80000fcc	2147487692
r9             0x80002b44	2147494724
r10            0x3ffff806508	4398038148360
r11            0x3ffff806618	4398038148632
r12            0x4d7dbea000	332822126592
r13            0x4d7dbb3c40	332821904448
r14            0x4d7db2caaa	332821351082
r15            0x3ffff8063d0	4398038148048
pc             0x4d7db2ccfc	0x4d7db2ccfc <__nanosleep_nocancel+2>
cc             0x0	0
x/i $pswa
0x4d7db2ccfc <__nanosleep_nocancel+2>:	lghi	%r4,-4095

readc.c (getchar)  ctrl-c'd in gdb:
(gdb) i r
r0             0xffffffff00000000       -4294967296
r1             0x4d7dbeedb0     332822146480
r2             0x0      0
r3             0x20000005000    2199023276032
r4             0x400    1024
r5             0x4d7daf8fd8     332821139416
r6             0x80000604       2147485188
r7             0x3ffffba43c0    4398041940928
r8             0x4d7dbeaf90     332822130576
r9             0x4d7dbea908     332822128904
r10            0x4d7da79c38     332820618296
r11            0x4d7dbea9e8     332822129128
r12            0x4d7dbea000     332822126592
r13            0x4d7dbb1b50     332821896016
r14            0x4d7dafa1ac     332821143980
r15            0x3ffffba3f28    4398041939752
pc             0x4d7db52f6a     0x4d7db52f6a <__read_nocancel>
cc             0x0      0


================================================================
Results with signals patch
================================================================

sleeping program control-c'd under gdb:
(gdb) i r
r0             0x3ff00000000    4393751543808
r1             0x4d7dbeedb0     332822146480
r2             0xfffffffffffffdfc       -516
r3             0x3ffffa340c8    4398040432840
r4             0x0      0
r5             0x8      8
r6             0x80002b40       2147494720
r7             0x3ffffa343a0    4398040433568
r8             0x80000fcc       2147487692
r9             0x80002b44       2147494724
r10            0x3ffffa33fc8    4398040432584
r11            0x3ffffa340d8    4398040432856
r12            0x4d7dbea000     332822126592
r13            0x4d7dbb3c40     332821904448
r14            0x4d7db2caaa     332821351082
r15            0x3ffffa33e90    4398040432272
pc             0x4d7db2ccfc     0x4d7db2ccfc <__nanosleep_nocancel+2>
cc             0x0      0
x/i $pswa
0x4d7db2ccfc <__nanosleep_nocancel+2>:  lghi    %r4,-4095


readc.c (getchar), ctrl-c'd in gdb:

(gdb) i r
r0             0xffffffff00000000       -4294967296
r1             0x4d7dbeedb0     332822146480
r2             0xfffffffffffffe00       -512
r3             0x20000005000    2199023276032
r4             0x400    1024
r5             0x4d7daf8fd8     332821139416
r6             0x80000604       2147485188
r7             0x3fffff433c0    4398045737920
r8             0x4d7dbeaf90     332822130576
r9             0x4d7dbea908     332822128904
r10            0x4d7da79c38     332820618296
r11            0x4d7dbea9e8     332822129128
r12            0x4d7dbea000     332822126592
r13            0x4d7dbb1b50     332821896016
r14            0x4d7dafa1ac     332821143980
r15            0x3fffff42f28    4398045736744
pc             0x4d7db52f6c     0x4d7db52f6c <__read_nocancel+2>
cc             0x0      0

Re: [PATCH] RFC: s390: Move get_signal_to_deliver() up in do_signal

[Oren Laadan]

Serge E. Hallyn wrote:
> Quoting Martin Schwidefsky (schwidefsky-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org):
>> On Wed, 10 Feb 2010 14:40:19 -0600
>> "Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> wrote:
>>
>>> The current placement of get_signal_to_deliver() means that
>>> try_to_freeze() in get_signal_to_deliver() will happen after
>>> regs->psw.addr, regs->svcnr, and regs->gprs[2] may have been
>>> mangled.  Since the app may get checkpointed while frozen and
>>> then restarted, this means we have to attempt a complicated
>>> and subtle re-calculation of the initial conditions.
>>>
>>> If we just move the get_signal_to_deliver() above the
>>> immediately preceding block, we enourmously simplify the
>>> arch-specific checkpoint/restart code.
>>>
>>> A full ltp run seems to show no regressions do to this move,
>>> though I'm not familiar enough with the entry_64.S code in
>>> particular to be absolutely confident.
>>>
>>> Is this a bad idea?
>> I think it is a bad idea. The comment of get_signal_to_deliver tells
>> you that the debugger is invoked and may want to change the registers.
>> If the get_signal_to_deliver calls is moved then the debugger sees
>> the unmodified registers which is imho wrong. A comparison of the
>> gdb testsuite with and without the patch will tell us more.
> 
> Right, but I guess what's confounding me is exactly why the values
> being set for the debugger make more sense to the debugger than the
> initial ones.  Note that they're not actually the same as they will
> be upon exit, for instance in the -ERESTARTNOHAND case if certain
> signals are delivered we'll change psw.addr back after all and set
> -EINTR.
> 
> So yeah, with this patch, if I send a signal to a program being
> debugged and then do 'i r' I see -516 instead of the -4 which I
> otherwise see, and a different $pswa.  Results for 'sleep' (which
> is ERESTART_RESTARTBLOCK) and 'getchar' (which is not) being
> interupted is below.  Frankly I think the info you see with the
> patch is more informative, not less, and the debugger certainly
> functions as well as it did before.
> 
> Of course there is probably fancier userspace tracing/debugging
> code out there which depends on the current behavior?  And the
> most convincing argument might be that it's all so magical that
> changing it is begging for trouble.

I suppose it also changes the behavior/ output of strace/ltrace ?

> But it sure would simplify checkpoint.

If this doesn't get through, then an alternative would be to
save the original state -- namely, svcnr, pswa, and gprs[2] --
on somewhere that is accessible to the checkpoint code ?

Oren.

Re: [PATCH] RFC: s390: Move get_signal_to_deliver() up in do_signal

[Serge E. Hallyn]

Quoting Oren Laadan (orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org):
> 
> 
> Serge E. Hallyn wrote:
> >Quoting Martin Schwidefsky (schwidefsky-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org):
> >>On Wed, 10 Feb 2010 14:40:19 -0600
> >>"Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> wrote:
> >>
> >>>The current placement of get_signal_to_deliver() means that
> >>>try_to_freeze() in get_signal_to_deliver() will happen after
> >>>regs->psw.addr, regs->svcnr, and regs->gprs[2] may have been
> >>>mangled.  Since the app may get checkpointed while frozen and
> >>>then restarted, this means we have to attempt a complicated
> >>>and subtle re-calculation of the initial conditions.
> >>>
> >>>If we just move the get_signal_to_deliver() above the
> >>>immediately preceding block, we enourmously simplify the
> >>>arch-specific checkpoint/restart code.
> >>>
> >>>A full ltp run seems to show no regressions do to this move,
> >>>though I'm not familiar enough with the entry_64.S code in
> >>>particular to be absolutely confident.
> >>>
> >>>Is this a bad idea?
> >>I think it is a bad idea. The comment of get_signal_to_deliver tells
> >>you that the debugger is invoked and may want to change the registers.
> >>If the get_signal_to_deliver calls is moved then the debugger sees
> >>the unmodified registers which is imho wrong. A comparison of the
> >>gdb testsuite with and without the patch will tell us more.
> >
> >Right, but I guess what's confounding me is exactly why the values
> >being set for the debugger make more sense to the debugger than the
> >initial ones.  Note that they're not actually the same as they will
> >be upon exit, for instance in the -ERESTARTNOHAND case if certain
> >signals are delivered we'll change psw.addr back after all and set
> >-EINTR.
> >
> >So yeah, with this patch, if I send a signal to a program being
> >debugged and then do 'i r' I see -516 instead of the -4 which I
> >otherwise see, and a different $pswa.  Results for 'sleep' (which
> >is ERESTART_RESTARTBLOCK) and 'getchar' (which is not) being
> >interupted is below.  Frankly I think the info you see with the
> >patch is more informative, not less, and the debugger certainly
> >functions as well as it did before.
> >
> >Of course there is probably fancier userspace tracing/debugging
> >code out there which depends on the current behavior?  And the
> >most convincing argument might be that it's all so magical that
> >changing it is begging for trouble.
> 
> I suppose it also changes the behavior/ output of strace/ltrace ?
> 
> >But it sure would simplify checkpoint.
> 
> If this doesn't get through, then an alternative would be to
> save the original state -- namely, svcnr, pswa, and gprs[2] --
> on somewhere that is accessible to the checkpoint code ?

Well, perhaps I've been making this more complicated than it
needs to be.

If we get frozen+checkpointed, then no matter whether we were
frozen with a real pending signal or not, we won't handle the
signal during restart, so we can treat it as though signr==0.

So, in that case, the only thing we need to change at end of
sys_restart is to handle the case:

	/* Restart a different system call. */
	if (retval == -ERESTART_RESTARTBLOCK
			&& regs->psw.addr == continue_addr) {
		regs->gprs[2] = __NR_restart_syscall;
		set_thread_flag(TIF_RESTART_SVC);
	}

Now that's of course a problem bc we don't know continue_addr
when we're in sys_restart().  So before we go into get_signal_to_deliver(),
we should set a new TIF flag which represents the fact that we are
inside do_signal with those conditions.

Then at end of restore_thread(), if that flag is set, we do the

	regs->gprs[2] = __NR_restart_syscall;
	set_thread_flag(TIF_RESTART_SVC);

(which presumably goes into a helper)

If there was a pending signal which we were intending to handle
when checkpointed, then that will simply be delivered after we
exit sys_restart.  That is no different from the case where we
got another signal delivered while a slow sighandler was executing.

I'll try implementing that idea.

-serge

Re: [PATCH] RFC: s390: Move get_signal_to_deliver() up in do_signal

[Serge E. Hallyn]

Quoting Serge E. Hallyn (serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> Well, perhaps I've been making this more complicated than it
> needs to be.
> 
> If we get frozen+checkpointed, then no matter whether we were
> frozen with a real pending signal or not, we won't handle the
> signal during restart, so we can treat it as though signr==0.
> 
> So, in that case, the only thing we need to change at end of
> sys_restart is to handle the case:
> 
> 	/* Restart a different system call. */
> 	if (retval == -ERESTART_RESTARTBLOCK
> 			&& regs->psw.addr == continue_addr) {
> 		regs->gprs[2] = __NR_restart_syscall;
> 		set_thread_flag(TIF_RESTART_SVC);
> 	}
> 
> Now that's of course a problem bc we don't know continue_addr
> when we're in sys_restart().  So before we go into get_signal_to_deliver(),
> we should set a new TIF flag which represents the fact that we are
> inside do_signal with those conditions.
> 
> Then at end of restore_thread(), if that flag is set, we do the
> 
> 	regs->gprs[2] = __NR_restart_syscall;
> 	set_thread_flag(TIF_RESTART_SVC);
> 
> (which presumably goes into a helper)
> 
> If there was a pending signal which we were intending to handle
> when checkpointed, then that will simply be delivered after we
> exit sys_restart.  That is no different from the case where we
> got another signal delivered while a slow sighandler was executing.
> 
> I'll try implementing that idea.
> 
> -serge

Like so:

From b50eb90da03e82ae6e7a2f1a8362e93c18d52074 Mon Sep 17 00:00:00 2001
From: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Date: Thu, 11 Feb 2010 14:05:16 -0500
Subject: [PATCH 1/1] set TIF_RESTART_SVC when needed after sys_restart

Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 arch/s390/include/asm/checkpoint_hdr.h |    5 +++
 arch/s390/include/asm/thread_info.h    |    2 +
 arch/s390/kernel/checkpoint.c          |   52 +++++++++++++++++++++++++++++++-
 arch/s390/kernel/signal.c              |   16 ++++++++++
 4 files changed, 74 insertions(+), 1 deletions(-)

diff --git a/arch/s390/include/asm/checkpoint_hdr.h b/arch/s390/include/asm/checkpoint_hdr.h
index a8c2a3d..fd8be2a 100644
--- a/arch/s390/include/asm/checkpoint_hdr.h
+++ b/arch/s390/include/asm/checkpoint_hdr.h
@@ -76,6 +76,11 @@ struct ckpt_hdr_cpu {
 	__u8 instruction_fetch;
 };
 
+struct ckpt_hdr_thread {
+	struct ckpt_hdr h;
+	__u64 thread_info_flags;
+};
+
 struct ckpt_hdr_mm_context {
 	struct ckpt_hdr h;
 	unsigned long vdso_base;
diff --git a/arch/s390/include/asm/thread_info.h b/arch/s390/include/asm/thread_info.h
index 66069e7..61e84e5 100644
--- a/arch/s390/include/asm/thread_info.h
+++ b/arch/s390/include/asm/thread_info.h
@@ -99,6 +99,7 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_MEMDIE		18
 #define TIF_RESTORE_SIGMASK	19	/* restore signal mask in do_signal() */
 #define TIF_FREEZE		20	/* thread is freezing for suspend */
+#define TIF_SIG_RESTARTBLOCK	23	/* restart must set TIF_RESTART_SVC */
 
 #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
 #define _TIF_RESTORE_SIGMASK	(1<<TIF_RESTORE_SIGMASK)
@@ -114,6 +115,7 @@ static inline struct thread_info *current_thread_info(void)
 #define _TIF_POLLING_NRFLAG	(1<<TIF_POLLING_NRFLAG)
 #define _TIF_31BIT		(1<<TIF_31BIT)
 #define _TIF_FREEZE		(1<<TIF_FREEZE)
+#define _TIF_SIG_RESTARTBLOCK	(1<<TIF_SIG_RESTARTBLOCK)
 
 #endif /* __KERNEL__ */
 
diff --git a/arch/s390/kernel/checkpoint.c b/arch/s390/kernel/checkpoint.c
index 60ba04d..cc7f341 100644
--- a/arch/s390/kernel/checkpoint.c
+++ b/arch/s390/kernel/checkpoint.c
@@ -12,6 +12,7 @@
 #include <asm/system.h>
 #include <asm/pgtable.h>
 #include <asm/elf.h>
+#include <asm/unistd.h>
 
 #include <linux/checkpoint.h>
 #include <linux/checkpoint_hdr.h>
@@ -65,6 +66,18 @@ static void s390_copy_regs(int op, struct ckpt_hdr_cpu *h,
 		BUG_ON(h->gprs[2] < 0);
 		h->gprs[2] = 0;
 	}
+
+	/*
+	 * if a checkpoint was taken while interrupted from a restartable
+	 * syscall, then treat this as though signr==0 (since we did not
+	 * handle the signal) and finish the last part of do_signal
+	 */
+	if (op == CKPT_RST && test_thread_flag(TIF_SIG_RESTARTBLOCK)) {
+		regs->gprs[2] = __NR_restart_syscall;
+		set_thread_flag(TIF_RESTART_SVC);
+		clear_thread_flag(TIF_SIG_RESTARTBLOCK);
+	}
+
 	CKPT_COPY_ARRAY(op, h->fprs, thr->fp_regs.fprs, NUM_FPRS);
 	CKPT_COPY_ARRAY(op, h->acrs, thr->acrs, NUM_ACRS);
 	CKPT_COPY_ARRAY(op, h->per_control_regs,
@@ -83,12 +96,24 @@ static void s390_mm(int op, struct ckpt_hdr_mm_context *h,
 
 int checkpoint_thread(struct ckpt_ctx *ctx, struct task_struct *t)
 {
+	struct ckpt_hdr_thread *h;
+	int ret;
+
 	/* we will eventually support this, as we do on x86-64 */
 	if (test_tsk_thread_flag(t, TIF_31BIT)) {
 		ckpt_err(ctx, -EINVAL, "checkpoint of 31-bit task\n");
 		return -EINVAL;
 	}
-	return 0;
+
+	h = ckpt_hdr_get_type(ctx, sizeof(*h), CKPT_HDR_THREAD);
+	if (!h)
+		return -ENOMEM;
+
+	h->thread_info_flags = task_thread_info(t)->flags;
+	ret = ckpt_write_obj(ctx, &h->h);
+	ckpt_hdr_put(ctx, h);
+
+	return ret;
 }
 
 /* dump the cpu state and registers of a given task */
@@ -148,11 +173,36 @@ int checkpoint_mm_context(struct ckpt_ctx *ctx, struct mm_struct *mm)
 
 int restore_thread(struct ckpt_ctx *ctx)
 {
+	struct ckpt_hdr_thread *h;
+
 	/* a 31-bit task cannot call sys_restart right now */
 	if (test_thread_flag(TIF_31BIT)) {
 		ckpt_err(ctx, -EINVAL, "restart from 31-bit task\n");
 		return -EINVAL;
 	}
+
+	h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_THREAD);
+	if (IS_ERR(h))
+		return PTR_ERR(h);
+
+	/*
+	 * If we were checkpointed while do_signal() interrupted a
+	 * syscall with restart blocks, then we have some cleanup to
+	 * do at end of restart, in order to finish our pretense of
+	 * having handled signr==0.  (See last part of do_signal).
+	 *
+	 * We can't set gprs[2] here bc we haven't copied registers
+	 * yet, that happens later in restore_cpu().  So re-set the
+	 * TIF_SIG_RESTARTBLOCK thread flag so we can detect it
+	 * at restore_cpu()->s390_copy_regs() and do the right thing.
+	 */
+	if (h->thread_info_flags & _TIF_SIG_RESTARTBLOCK)
+		set_thread_flag(TIF_SIG_RESTARTBLOCK);
+
+	/* will have to handle TIF_RESTORE_SIGMASK as well */
+
+	ckpt_hdr_put(ctx, h);
+
 	return 0;
 }
 
diff --git a/arch/s390/kernel/signal.c b/arch/s390/kernel/signal.c
index 1675c48..9b6ca21 100644
--- a/arch/s390/kernel/signal.c
+++ b/arch/s390/kernel/signal.c
@@ -459,6 +459,16 @@ void do_signal(struct pt_regs *regs)
 			break;
 		case -ERESTART_RESTARTBLOCK:
 			regs->gprs[2] = -EINTR;
+			/*
+			 * This condition is the only one which requires
+			 * special care after handling a signr==0.  So if
+			 * we get frozen and checkpointed at the
+			 * get_signal_to_deliver() below, then we need
+			 * to convey this condition to sys_restart() so it
+			 * can set the restored thread up to run the restart
+			 * block.
+			 */
+			set_thread_flag(TIF_SIG_RESTARTBLOCK);
 		}
 		regs->svcnr = 0;	/* Don't deal with this again. */
 	}
@@ -467,6 +477,12 @@ void do_signal(struct pt_regs *regs)
 	   the debugger may change all our registers ... */
 	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
 
+	/*
+	 * we won't get frozen past this so clear the thread flag hinting
+	 * to sys_restart that TIF_RESTART_SVC must be set.
+	 */
+	clear_thread_flag(TIF_SIG_RESTARTBLOCK);
+
 	/* Depending on the signal settings we may need to revert the
 	   decision to restart the system call. */
 	if (signr > 0 && regs->psw.addr == restart_addr) {
-- 
1.6.1

Re: [PATCH] RFC: s390: Move get_signal_to_deliver() up in do_signal

[Oren Laadan]

Ok, I think I'm convinced ;)
Applied...

Serge E. Hallyn wrote:
> Quoting Serge E. Hallyn (serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
>> Well, perhaps I've been making this more complicated than it
>> needs to be.
>>
>> If we get frozen+checkpointed, then no matter whether we were
>> frozen with a real pending signal or not, we won't handle the
>> signal during restart, so we can treat it as though signr==0.
>>
>> So, in that case, the only thing we need to change at end of
>> sys_restart is to handle the case:
>>
>> 	/* Restart a different system call. */
>> 	if (retval == -ERESTART_RESTARTBLOCK
>> 			&& regs->psw.addr == continue_addr) {
>> 		regs->gprs[2] = __NR_restart_syscall;
>> 		set_thread_flag(TIF_RESTART_SVC);
>> 	}
>>
>> Now that's of course a problem bc we don't know continue_addr
>> when we're in sys_restart().  So before we go into get_signal_to_deliver(),
>> we should set a new TIF flag which represents the fact that we are
>> inside do_signal with those conditions.
>>
>> Then at end of restore_thread(), if that flag is set, we do the
>>
>> 	regs->gprs[2] = __NR_restart_syscall;
>> 	set_thread_flag(TIF_RESTART_SVC);
>>
>> (which presumably goes into a helper)
>>
>> If there was a pending signal which we were intending to handle
>> when checkpointed, then that will simply be delivered after we
>> exit sys_restart.  That is no different from the case where we
>> got another signal delivered while a slow sighandler was executing.
>>
>> I'll try implementing that idea.
>>
>> -serge
> 
> Like so:
> 
> From b50eb90da03e82ae6e7a2f1a8362e93c18d52074 Mon Sep 17 00:00:00 2001
> From: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Date: Thu, 11 Feb 2010 14:05:16 -0500
> Subject: [PATCH 1/1] set TIF_RESTART_SVC when needed after sys_restart
> 
> Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/s390/include/asm/checkpoint_hdr.h |    5 +++
>  arch/s390/include/asm/thread_info.h    |    2 +
>  arch/s390/kernel/checkpoint.c          |   52 +++++++++++++++++++++++++++++++-
>  arch/s390/kernel/signal.c              |   16 ++++++++++
>  4 files changed, 74 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/s390/include/asm/checkpoint_hdr.h b/arch/s390/include/asm/checkpoint_hdr.h
> index a8c2a3d..fd8be2a 100644
> --- a/arch/s390/include/asm/checkpoint_hdr.h
> +++ b/arch/s390/include/asm/checkpoint_hdr.h
> @@ -76,6 +76,11 @@ struct ckpt_hdr_cpu {
>  	__u8 instruction_fetch;
>  };
>  
> +struct ckpt_hdr_thread {
> +	struct ckpt_hdr h;
> +	__u64 thread_info_flags;
> +};
> +
>  struct ckpt_hdr_mm_context {
>  	struct ckpt_hdr h;
>  	unsigned long vdso_base;
> diff --git a/arch/s390/include/asm/thread_info.h b/arch/s390/include/asm/thread_info.h
> index 66069e7..61e84e5 100644
> --- a/arch/s390/include/asm/thread_info.h
> +++ b/arch/s390/include/asm/thread_info.h
> @@ -99,6 +99,7 @@ static inline struct thread_info *current_thread_info(void)
>  #define TIF_MEMDIE		18
>  #define TIF_RESTORE_SIGMASK	19	/* restore signal mask in do_signal() */
>  #define TIF_FREEZE		20	/* thread is freezing for suspend */
> +#define TIF_SIG_RESTARTBLOCK	23	/* restart must set TIF_RESTART_SVC */
>  
>  #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
>  #define _TIF_RESTORE_SIGMASK	(1<<TIF_RESTORE_SIGMASK)
> @@ -114,6 +115,7 @@ static inline struct thread_info *current_thread_info(void)
>  #define _TIF_POLLING_NRFLAG	(1<<TIF_POLLING_NRFLAG)
>  #define _TIF_31BIT		(1<<TIF_31BIT)
>  #define _TIF_FREEZE		(1<<TIF_FREEZE)
> +#define _TIF_SIG_RESTARTBLOCK	(1<<TIF_SIG_RESTARTBLOCK)
>  
>  #endif /* __KERNEL__ */
>  
> diff --git a/arch/s390/kernel/checkpoint.c b/arch/s390/kernel/checkpoint.c
> index 60ba04d..cc7f341 100644
> --- a/arch/s390/kernel/checkpoint.c
> +++ b/arch/s390/kernel/checkpoint.c
> @@ -12,6 +12,7 @@
>  #include <asm/system.h>
>  #include <asm/pgtable.h>
>  #include <asm/elf.h>
> +#include <asm/unistd.h>
>  
>  #include <linux/checkpoint.h>
>  #include <linux/checkpoint_hdr.h>
> @@ -65,6 +66,18 @@ static void s390_copy_regs(int op, struct ckpt_hdr_cpu *h,
>  		BUG_ON(h->gprs[2] < 0);
>  		h->gprs[2] = 0;
>  	}
> +
> +	/*
> +	 * if a checkpoint was taken while interrupted from a restartable
> +	 * syscall, then treat this as though signr==0 (since we did not
> +	 * handle the signal) and finish the last part of do_signal
> +	 */
> +	if (op == CKPT_RST && test_thread_flag(TIF_SIG_RESTARTBLOCK)) {
> +		regs->gprs[2] = __NR_restart_syscall;
> +		set_thread_flag(TIF_RESTART_SVC);
> +		clear_thread_flag(TIF_SIG_RESTARTBLOCK);
> +	}
> +
>  	CKPT_COPY_ARRAY(op, h->fprs, thr->fp_regs.fprs, NUM_FPRS);
>  	CKPT_COPY_ARRAY(op, h->acrs, thr->acrs, NUM_ACRS);
>  	CKPT_COPY_ARRAY(op, h->per_control_regs,
> @@ -83,12 +96,24 @@ static void s390_mm(int op, struct ckpt_hdr_mm_context *h,
>  
>  int checkpoint_thread(struct ckpt_ctx *ctx, struct task_struct *t)
>  {
> +	struct ckpt_hdr_thread *h;
> +	int ret;
> +
>  	/* we will eventually support this, as we do on x86-64 */
>  	if (test_tsk_thread_flag(t, TIF_31BIT)) {
>  		ckpt_err(ctx, -EINVAL, "checkpoint of 31-bit task\n");
>  		return -EINVAL;
>  	}
> -	return 0;
> +
> +	h = ckpt_hdr_get_type(ctx, sizeof(*h), CKPT_HDR_THREAD);
> +	if (!h)
> +		return -ENOMEM;
> +
> +	h->thread_info_flags = task_thread_info(t)->flags;
> +	ret = ckpt_write_obj(ctx, &h->h);
> +	ckpt_hdr_put(ctx, h);
> +
> +	return ret;
>  }
>  
>  /* dump the cpu state and registers of a given task */
> @@ -148,11 +173,36 @@ int checkpoint_mm_context(struct ckpt_ctx *ctx, struct mm_struct *mm)
>  
>  int restore_thread(struct ckpt_ctx *ctx)
>  {
> +	struct ckpt_hdr_thread *h;
> +
>  	/* a 31-bit task cannot call sys_restart right now */
>  	if (test_thread_flag(TIF_31BIT)) {
>  		ckpt_err(ctx, -EINVAL, "restart from 31-bit task\n");
>  		return -EINVAL;
>  	}
> +
> +	h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_THREAD);
> +	if (IS_ERR(h))
> +		return PTR_ERR(h);
> +
> +	/*
> +	 * If we were checkpointed while do_signal() interrupted a
> +	 * syscall with restart blocks, then we have some cleanup to
> +	 * do at end of restart, in order to finish our pretense of
> +	 * having handled signr==0.  (See last part of do_signal).
> +	 *
> +	 * We can't set gprs[2] here bc we haven't copied registers
> +	 * yet, that happens later in restore_cpu().  So re-set the
> +	 * TIF_SIG_RESTARTBLOCK thread flag so we can detect it
> +	 * at restore_cpu()->s390_copy_regs() and do the right thing.
> +	 */
> +	if (h->thread_info_flags & _TIF_SIG_RESTARTBLOCK)
> +		set_thread_flag(TIF_SIG_RESTARTBLOCK);
> +
> +	/* will have to handle TIF_RESTORE_SIGMASK as well */
> +
> +	ckpt_hdr_put(ctx, h);
> +
>  	return 0;
>  }
>  
> diff --git a/arch/s390/kernel/signal.c b/arch/s390/kernel/signal.c
> index 1675c48..9b6ca21 100644
> --- a/arch/s390/kernel/signal.c
> +++ b/arch/s390/kernel/signal.c
> @@ -459,6 +459,16 @@ void do_signal(struct pt_regs *regs)
>  			break;
>  		case -ERESTART_RESTARTBLOCK:
>  			regs->gprs[2] = -EINTR;
> +			/*
> +			 * This condition is the only one which requires
> +			 * special care after handling a signr==0.  So if
> +			 * we get frozen and checkpointed at the
> +			 * get_signal_to_deliver() below, then we need
> +			 * to convey this condition to sys_restart() so it
> +			 * can set the restored thread up to run the restart
> +			 * block.
> +			 */
> +			set_thread_flag(TIF_SIG_RESTARTBLOCK);
>  		}
>  		regs->svcnr = 0;	/* Don't deal with this again. */
>  	}
> @@ -467,6 +477,12 @@ void do_signal(struct pt_regs *regs)
>  	   the debugger may change all our registers ... */
>  	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
>  
> +	/*
> +	 * we won't get frozen past this so clear the thread flag hinting
> +	 * to sys_restart that TIF_RESTART_SVC must be set.
> +	 */
> +	clear_thread_flag(TIF_SIG_RESTARTBLOCK);
> +
>  	/* Depending on the signal settings we may need to revert the
>  	   decision to restart the system call. */
>  	if (signr > 0 && regs->psw.addr == restart_addr) {

Re: [PATCH] RFC: s390: Move get_signal_to_deliver() up in do_signal

[Oren Laadan]

Ok, I think I'm convinced ;)
Applied...

Serge E. Hallyn wrote:
> Quoting Serge E. Hallyn (serue@us.ibm.com):
>> Well, perhaps I've been making this more complicated than it
>> needs to be.
>>
>> If we get frozen+checkpointed, then no matter whether we were
>> frozen with a real pending signal or not, we won't handle the
>> signal during restart, so we can treat it as though signr==0.
>>
>> So, in that case, the only thing we need to change at end of
>> sys_restart is to handle the case:
>>
>> 	/* Restart a different system call. */
>> 	if (retval == -ERESTART_RESTARTBLOCK
>> 			&& regs->psw.addr == continue_addr) {
>> 		regs->gprs[2] = __NR_restart_syscall;
>> 		set_thread_flag(TIF_RESTART_SVC);
>> 	}
>>
>> Now that's of course a problem bc we don't know continue_addr
>> when we're in sys_restart().  So before we go into get_signal_to_deliver(),
>> we should set a new TIF flag which represents the fact that we are
>> inside do_signal with those conditions.
>>
>> Then at end of restore_thread(), if that flag is set, we do the
>>
>> 	regs->gprs[2] = __NR_restart_syscall;
>> 	set_thread_flag(TIF_RESTART_SVC);
>>
>> (which presumably goes into a helper)
>>
>> If there was a pending signal which we were intending to handle
>> when checkpointed, then that will simply be delivered after we
>> exit sys_restart.  That is no different from the case where we
>> got another signal delivered while a slow sighandler was executing.
>>
>> I'll try implementing that idea.
>>
>> -serge
> 
> Like so:
> 
> From b50eb90da03e82ae6e7a2f1a8362e93c18d52074 Mon Sep 17 00:00:00 2001
> From: Serge E. Hallyn <serue@us.ibm.com>
> Date: Thu, 11 Feb 2010 14:05:16 -0500
> Subject: [PATCH 1/1] set TIF_RESTART_SVC when needed after sys_restart
> 
> Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
> ---
>  arch/s390/include/asm/checkpoint_hdr.h |    5 +++
>  arch/s390/include/asm/thread_info.h    |    2 +
>  arch/s390/kernel/checkpoint.c          |   52 +++++++++++++++++++++++++++++++-
>  arch/s390/kernel/signal.c              |   16 ++++++++++
>  4 files changed, 74 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/s390/include/asm/checkpoint_hdr.h b/arch/s390/include/asm/checkpoint_hdr.h
> index a8c2a3d..fd8be2a 100644
> --- a/arch/s390/include/asm/checkpoint_hdr.h
> +++ b/arch/s390/include/asm/checkpoint_hdr.h
> @@ -76,6 +76,11 @@ struct ckpt_hdr_cpu {
>  	__u8 instruction_fetch;
>  };
>  
> +struct ckpt_hdr_thread {
> +	struct ckpt_hdr h;
> +	__u64 thread_info_flags;
> +};
> +
>  struct ckpt_hdr_mm_context {
>  	struct ckpt_hdr h;
>  	unsigned long vdso_base;
> diff --git a/arch/s390/include/asm/thread_info.h b/arch/s390/include/asm/thread_info.h
> index 66069e7..61e84e5 100644
> --- a/arch/s390/include/asm/thread_info.h
> +++ b/arch/s390/include/asm/thread_info.h
> @@ -99,6 +99,7 @@ static inline struct thread_info *current_thread_info(void)
>  #define TIF_MEMDIE		18
>  #define TIF_RESTORE_SIGMASK	19	/* restore signal mask in do_signal() */
>  #define TIF_FREEZE		20	/* thread is freezing for suspend */
> +#define TIF_SIG_RESTARTBLOCK	23	/* restart must set TIF_RESTART_SVC */
>  
>  #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
>  #define _TIF_RESTORE_SIGMASK	(1<<TIF_RESTORE_SIGMASK)
> @@ -114,6 +115,7 @@ static inline struct thread_info *current_thread_info(void)
>  #define _TIF_POLLING_NRFLAG	(1<<TIF_POLLING_NRFLAG)
>  #define _TIF_31BIT		(1<<TIF_31BIT)
>  #define _TIF_FREEZE		(1<<TIF_FREEZE)
> +#define _TIF_SIG_RESTARTBLOCK	(1<<TIF_SIG_RESTARTBLOCK)
>  
>  #endif /* __KERNEL__ */
>  
> diff --git a/arch/s390/kernel/checkpoint.c b/arch/s390/kernel/checkpoint.c
> index 60ba04d..cc7f341 100644
> --- a/arch/s390/kernel/checkpoint.c
> +++ b/arch/s390/kernel/checkpoint.c
> @@ -12,6 +12,7 @@
>  #include <asm/system.h>
>  #include <asm/pgtable.h>
>  #include <asm/elf.h>
> +#include <asm/unistd.h>
>  
>  #include <linux/checkpoint.h>
>  #include <linux/checkpoint_hdr.h>
> @@ -65,6 +66,18 @@ static void s390_copy_regs(int op, struct ckpt_hdr_cpu *h,
>  		BUG_ON(h->gprs[2] < 0);
>  		h->gprs[2] = 0;
>  	}
> +
> +	/*
> +	 * if a checkpoint was taken while interrupted from a restartable
> +	 * syscall, then treat this as though signr==0 (since we did not
> +	 * handle the signal) and finish the last part of do_signal
> +	 */
> +	if (op == CKPT_RST && test_thread_flag(TIF_SIG_RESTARTBLOCK)) {
> +		regs->gprs[2] = __NR_restart_syscall;
> +		set_thread_flag(TIF_RESTART_SVC);
> +		clear_thread_flag(TIF_SIG_RESTARTBLOCK);
> +	}
> +
>  	CKPT_COPY_ARRAY(op, h->fprs, thr->fp_regs.fprs, NUM_FPRS);
>  	CKPT_COPY_ARRAY(op, h->acrs, thr->acrs, NUM_ACRS);
>  	CKPT_COPY_ARRAY(op, h->per_control_regs,
> @@ -83,12 +96,24 @@ static void s390_mm(int op, struct ckpt_hdr_mm_context *h,
>  
>  int checkpoint_thread(struct ckpt_ctx *ctx, struct task_struct *t)
>  {
> +	struct ckpt_hdr_thread *h;
> +	int ret;
> +
>  	/* we will eventually support this, as we do on x86-64 */
>  	if (test_tsk_thread_flag(t, TIF_31BIT)) {
>  		ckpt_err(ctx, -EINVAL, "checkpoint of 31-bit task\n");
>  		return -EINVAL;
>  	}
> -	return 0;
> +
> +	h = ckpt_hdr_get_type(ctx, sizeof(*h), CKPT_HDR_THREAD);
> +	if (!h)
> +		return -ENOMEM;
> +
> +	h->thread_info_flags = task_thread_info(t)->flags;
> +	ret = ckpt_write_obj(ctx, &h->h);
> +	ckpt_hdr_put(ctx, h);
> +
> +	return ret;
>  }
>  
>  /* dump the cpu state and registers of a given task */
> @@ -148,11 +173,36 @@ int checkpoint_mm_context(struct ckpt_ctx *ctx, struct mm_struct *mm)
>  
>  int restore_thread(struct ckpt_ctx *ctx)
>  {
> +	struct ckpt_hdr_thread *h;
> +
>  	/* a 31-bit task cannot call sys_restart right now */
>  	if (test_thread_flag(TIF_31BIT)) {
>  		ckpt_err(ctx, -EINVAL, "restart from 31-bit task\n");
>  		return -EINVAL;
>  	}
> +
> +	h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_THREAD);
> +	if (IS_ERR(h))
> +		return PTR_ERR(h);
> +
> +	/*
> +	 * If we were checkpointed while do_signal() interrupted a
> +	 * syscall with restart blocks, then we have some cleanup to
> +	 * do at end of restart, in order to finish our pretense of
> +	 * having handled signr==0.  (See last part of do_signal).
> +	 *
> +	 * We can't set gprs[2] here bc we haven't copied registers
> +	 * yet, that happens later in restore_cpu().  So re-set the
> +	 * TIF_SIG_RESTARTBLOCK thread flag so we can detect it
> +	 * at restore_cpu()->s390_copy_regs() and do the right thing.
> +	 */
> +	if (h->thread_info_flags & _TIF_SIG_RESTARTBLOCK)
> +		set_thread_flag(TIF_SIG_RESTARTBLOCK);
> +
> +	/* will have to handle TIF_RESTORE_SIGMASK as well */
> +
> +	ckpt_hdr_put(ctx, h);
> +
>  	return 0;
>  }
>  
> diff --git a/arch/s390/kernel/signal.c b/arch/s390/kernel/signal.c
> index 1675c48..9b6ca21 100644
> --- a/arch/s390/kernel/signal.c
> +++ b/arch/s390/kernel/signal.c
> @@ -459,6 +459,16 @@ void do_signal(struct pt_regs *regs)
>  			break;
>  		case -ERESTART_RESTARTBLOCK:
>  			regs->gprs[2] = -EINTR;
> +			/*
> +			 * This condition is the only one which requires
> +			 * special care after handling a signr==0.  So if
> +			 * we get frozen and checkpointed at the
> +			 * get_signal_to_deliver() below, then we need
> +			 * to convey this condition to sys_restart() so it
> +			 * can set the restored thread up to run the restart
> +			 * block.
> +			 */
> +			set_thread_flag(TIF_SIG_RESTARTBLOCK);
>  		}
>  		regs->svcnr = 0;	/* Don't deal with this again. */
>  	}
> @@ -467,6 +477,12 @@ void do_signal(struct pt_regs *regs)
>  	   the debugger may change all our registers ... */
>  	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
>  
> +	/*
> +	 * we won't get frozen past this so clear the thread flag hinting
> +	 * to sys_restart that TIF_RESTART_SVC must be set.
> +	 */
> +	clear_thread_flag(TIF_SIG_RESTARTBLOCK);
> +
>  	/* Depending on the signal settings we may need to revert the
>  	   decision to restart the system call. */
>  	if (signr > 0 && regs->psw.addr == restart_addr) {