s390: set TIF_RESTARTBLOCK after sys_restart()

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

---
 arch/s390/kernel/entry64.S |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

index 9aff1d4..798ea9c 100644
@@ -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

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.

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

index d8d4b6b..3e8109b 100644
@@ -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);

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