[PATCH] s390: set TIF_RESTARTBLOCK after sys_restart()

[PATCH] s390: set TIF_RESTARTBLOCK after sys_restart()

[Serge E. Hallyn]

(this is on top of the patch
	's390: actually restart syscalls after sys_restart'
which I sent last Thursday)

We use TIF_RESTARTBLOCK in do_signal() to tell sys_checkpoint()
to mark the thread as needing a sysc_do_restart to restart an
interrupted syscall after sys_restart().

However, as Oren pointed out, if the task receives a signal
after sys_restart() but before returning to userspace, and
enters do_signal, then conditions will not be met to set
TIF_RESTARTBLOCK.  So if the restarted task freezes here and is
checkpointed, then the resulting checkpoint image will not restart
the interrupted syscall.

So, set TIF_RESTARTBLOCK in sys_restart() if TIF_RESTARTBLOCK was set
at checkpoint.  Clear TIF_RESTARTBLOCK in ssyc_do_restart, so that
if we do not go to do_signal right after sys_restart() we don't
mistakenly keep running with that flag.

To test this, add a dummy handler for SIGUSR1 to cr_tests/sleep/sleeptest.c,
open two terminals, and do:

Terminal 1:                             Terminal :
mount -t cgroup -o freezer freezer /cgroup
mkdir /cgroup/1 /cgroup/2
echo $$ > /cgroup/1/tasks
./sleeptest

                                        echo FROZEN > /cgroup/1/freezer.state
                                        checkpoint `pidof sleeptest` > /tmp/out
                                        thaw

restart -F /cgroup/2 < /tmp/out

                                        kill -USR1 `pidof sleeptest`;
                                        echo THAWED > /cgroup/2/freezer.state ;
                                        sleep 0.3;
                                        echo FROZEN > /cgroup/2/freezer.state;
                                        checkpoint `pidof sleeptest` > /tmp/out2;
                                        echo THAWED > /cgroup/2/freezer.state

restart < /tmp/out2

Without this patch, the final restart will immediately exit.  With
this patch, it will restart the sleep.

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

diff --git a/arch/s390/kernel/entry64.S b/arch/s390/kernel/entry64.S
index 9aff1d4..798ea9c 100644
--- a/arch/s390/kernel/entry64.S
+++ b/arch/s390/kernel/entry64.S
@@ -58,6 +58,7 @@ _TIF_WORK_INT = (_TIF_SIGPENDING | _TIF_NOTIFY_RESUME | _TIF_NEED_RESCHED | \
 		 _TIF_MCCK_PENDING)
 _TIF_SYSCALL = (_TIF_SYSCALL_TRACE>>8 | _TIF_SYSCALL_AUDIT>>8 | \
 		_TIF_SECCOMP>>8 | _TIF_SYSCALL_TRACEPOINT>>8)
+CLR_TIF_RESTARTBLOCK = (255 - (_TIF_RESTARTBLOCK>>16))
 
 #define BASED(name) name-system_call(%r13)
 
@@ -338,6 +339,7 @@ sysc_mcck_pending:
 #
 sysc_sigpending:
 	ni	__TI_flags+7(%r9),255-_TIF_SINGLE_STEP # clear TIF_SINGLE_STEP
+	ni	__TI_flags+6(%r9),CLR_TIF_RESTARTBLOCK # clear TIF_RESTARTBLOCK
 	la	%r2,SP_PTREGS(%r15)	# load pt_regs
 	brasl	%r14,do_signal		# call do_signal
 	tm	__TI_flags+7(%r9),_TIF_RESTART_SVC
-- 
1.6.1

Re: [PATCH] s390: set TIF_RESTARTBLOCK after sys_restart()

[Oren Laadan]

Serge E. Hallyn wrote:
> (this is on top of the patch
> 	's390: actually restart syscalls after sys_restart'
> which I sent last Thursday)
> 
> We use TIF_RESTARTBLOCK in do_signal() to tell sys_checkpoint()
> to mark the thread as needing a sysc_do_restart to restart an
> interrupted syscall after sys_restart().
> 
> However, as Oren pointed out, if the task receives a signal
> after sys_restart() but before returning to userspace, and
> enters do_signal, then conditions will not be met to set
> TIF_RESTARTBLOCK.  So if the restarted task freezes here and is
> checkpointed, then the resulting checkpoint image will not restart
> the interrupted syscall.
> 
> So, set TIF_RESTARTBLOCK in sys_restart() if TIF_RESTARTBLOCK was set
> at checkpoint.  Clear TIF_RESTARTBLOCK in ssyc_do_restart, so that

All I see in this patch (below) is the "clear" part. I don't see
the "set" part.

Also, I'm still not convinced that this fixes all 3 issues that
I mentioned in the other thread.

Oren.


> if we do not go to do_signal right after sys_restart() we don't
> mistakenly keep running with that flag.
> 
> To test this, add a dummy handler for SIGUSR1 to cr_tests/sleep/sleeptest.c,
> open two terminals, and do:
> 
> Terminal 1:                             Terminal :
> mount -t cgroup -o freezer freezer /cgroup
> mkdir /cgroup/1 /cgroup/2
> echo $$ > /cgroup/1/tasks
> ./sleeptest
> 
>                                         echo FROZEN > /cgroup/1/freezer.state
>                                         checkpoint `pidof sleeptest` > /tmp/out
>                                         thaw
> 
> restart -F /cgroup/2 < /tmp/out
> 
>                                         kill -USR1 `pidof sleeptest`;
>                                         echo THAWED > /cgroup/2/freezer.state ;
>                                         sleep 0.3;
>                                         echo FROZEN > /cgroup/2/freezer.state;
>                                         checkpoint `pidof sleeptest` > /tmp/out2;
>                                         echo THAWED > /cgroup/2/freezer.state
> 
> restart < /tmp/out2
> 
> Without this patch, the final restart will immediately exit.  With
> this patch, it will restart the sleep.
> 
> Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/s390/kernel/entry64.S |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/s390/kernel/entry64.S b/arch/s390/kernel/entry64.S
> index 9aff1d4..798ea9c 100644
> --- a/arch/s390/kernel/entry64.S
> +++ b/arch/s390/kernel/entry64.S
> @@ -58,6 +58,7 @@ _TIF_WORK_INT = (_TIF_SIGPENDING | _TIF_NOTIFY_RESUME | _TIF_NEED_RESCHED | \
>  		 _TIF_MCCK_PENDING)
>  _TIF_SYSCALL = (_TIF_SYSCALL_TRACE>>8 | _TIF_SYSCALL_AUDIT>>8 | \
>  		_TIF_SECCOMP>>8 | _TIF_SYSCALL_TRACEPOINT>>8)
> +CLR_TIF_RESTARTBLOCK = (255 - (_TIF_RESTARTBLOCK>>16))
>  
>  #define BASED(name) name-system_call(%r13)
>  
> @@ -338,6 +339,7 @@ sysc_mcck_pending:
>  #
>  sysc_sigpending:
>  	ni	__TI_flags+7(%r9),255-_TIF_SINGLE_STEP # clear TIF_SINGLE_STEP
> +	ni	__TI_flags+6(%r9),CLR_TIF_RESTARTBLOCK # clear TIF_RESTARTBLOCK
>  	la	%r2,SP_PTREGS(%r15)	# load pt_regs
>  	brasl	%r14,do_signal		# call do_signal
>  	tm	__TI_flags+7(%r9),_TIF_RESTART_SVC

Re: [PATCH] s390: set TIF_RESTARTBLOCK after sys_restart()

[Serge E. Hallyn]

Quoting Oren Laadan (orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org):
> 
> 
> Serge E. Hallyn wrote:
> > (this is on top of the patch
> > 	's390: actually restart syscalls after sys_restart'
> > which I sent last Thursday)
> > 
> > We use TIF_RESTARTBLOCK in do_signal() to tell sys_checkpoint()
> > to mark the thread as needing a sysc_do_restart to restart an
> > interrupted syscall after sys_restart().
> > 
> > However, as Oren pointed out, if the task receives a signal
> > after sys_restart() but before returning to userspace, and
> > enters do_signal, then conditions will not be met to set
> > TIF_RESTARTBLOCK.  So if the restarted task freezes here and is
> > checkpointed, then the resulting checkpoint image will not restart
> > the interrupted syscall.
> > 
> > So, set TIF_RESTARTBLOCK in sys_restart() if TIF_RESTARTBLOCK was set
> > at checkpoint.  Clear TIF_RESTARTBLOCK in ssyc_do_restart, so that
> 
> All I see in this patch (below) is the "clear" part. I don't see
> the "set" part.

Drat.  And I seem to have wiped it from my working tree - though
it was just:

diff --git a/arch/s390/mm/checkpoint.c b/arch/s390/mm/checkpoint.c
index d8d4b6b..3e8109b 100644
--- a/arch/s390/mm/checkpoint.c
+++ b/arch/s390/mm/checkpoint.c
@@ -74,6 +74,7 @@ static void s390_copy_regs(int op, struct ckpt_hdr_cpu *h,
	if (op == CKPT_RST && h->should_restart) {
		regs->gprs[2] = __NR_restart_syscall;
		set_thread_flag(TIF_RESTART_SVC);
+		set_thread_flag(TIF_RESTARTBLOCK);
	}
	CKPT_COPY_ARRAY(op, h->fprs, thr->fp_regs.fprs, NUM_FPRS);
	CKPT_COPY_ARRAY(op, h->acrs, thr->acrs, NUM_ACRS);


> Also, I'm still not convinced that this fixes all 3 issues that
> I mentioned in the other thread.

Well, at the end of sys_restart(), we've set up the registers and
flags (almost) exactly as they would have been if a signal 0 had
been received and processed through do_signal (which it was, from
the freezer).  That is for the -ERESTART_RESTARTBLOCK case.  In the
case that we went into the freezing do_signal (the one where we
were originally checkpointed) with -ERESTARTNOHAND, -ERESTARTSYS,
or -ERESTARTNOINTR, then for a signr == 0 we don't do any further
processing in do_signal - it was all done before get_signal_to_deliver()
which is where we froze.  So there is nothing further to emulate
(except sigmask restore which is not handled atm).

The only thing that isn't done is the unsetting of TIF_RESTARTBLOCK.
IIUC the only paths we can follow after the sys_restart() syscall,
are do_sysc_restart or do_signal.  In do_signal we will re-set it,
and after this patch we also do in do_sysc_restart.

I do agree I should still restore certain TIF flags in the
arch/s390/mm/checkpoint.c:arch_restore_thread() function, such
as TIF_RESTORE_SIGMASK.  However that will require still more
emulation work as TIF_RESTORE_SIGMASK gets tweaked at the end of
do_signal().

Anyway, I think a full patch including these two patches plus the
resetting of TIF_RESTORE_SIGMASK should go to the s390 devel list
where I'm sure they'll gladly tear me to tasty little bits.

-serge