[Devel] Re: [PATCH RFC] s390: let tasks know to restart syscalls after sys_restart()

[Oren Laadan]

Serge E. Hallyn wrote:
> Quoting Oren Laadan (orenl at cs.columbia.edu):
>>
>> Serge E. Hallyn wrote:
>>> (This is a patch against the checkpoint/restart kernel tree at
>>> http://git.ncl.cs.columbia.edu/?p=linux-cr.git;a=shortlog;h=refs/heads/ckpt-v19-rc2.9)
>>>
>>> On x86, do_signal() leaves -516 in eax while it freezes, which
>>> sys_restart() can use to detect that it should restart the
>>> syscall which was interrupted by a signal (or the freezer).
>>>
>>> On s390, gprs[2] gets tweaked to -EINTR (-4) instead, leaving
>>> us no reliable way to tell whether should be restarted.  If the
>>> task is checkpointed here and then restarted, then the last part
>>> of do_signal() won't be done, and the interrupted syscall won't
>>> be restarted.
>>>
>>> This patch defines TIF_RESTARTBLOCK as a thread flag showing that
>>> the thread expects to be frozen while kicked out of a restartable
>>> syscall by a signal.
>>>
>>> The TIF_RESTARTBLOCK flag is only set for the duration of the
>>> get get_signal_to_deliver() which is where the task may get
>>> frozen.  We also set it in sys_restart() if the checkpointed task
>>> had had TIF_RESTARTBLOCK set.  It will get cleared if upon exiting
>>> sys_restart() we jump to sysc_sigpending.  If instead we jump back
>>> to do_signal(), then TIF_RESTARTBLOCK will stay set again until
>>> after get_signal_to_deliver() so that if it immediately freezes and
>>> is re-checkpointed, the resulting second checkpoint image again
>>> will have TIF_RESTARTBLOCK set.
>> Two comments:
>>
>> 1) This note will be lost once we fold this patch into a clean
>> patchset. Can you please make it part of the code ?
> 
> Sure, good idea.
> 
>> 2) Maybe I'm missing something, but I'm not convinced. Can you
>> elaborate on why this is correct in different cases ?  Also, in
>> particular with respect to the post-signal-sent snippet in the
>> signal handling code:
>>
>>         if (retval == -ERESTART_RESTARTBLOCK
>>             && regs->psw.addr == continue_addr) {
>>
>>                 regs->gprs[2] = __NR_restart_syscall;
>>
>>                 set_thread_flag(TIF_RESTART_SVC);
>>
>>         }
>>
>>
>> Would it do what you expect after a restart ?  (restart modifies
>> the psw.addr)
> 
> I don't understand the question.  After sys_restart(), we won't be
> returning to this kernel code.  We'll either immediately call
> restart_syscall(), or, if a signal was delivered before sys_restart(),
> completed, then do_signal() will start again from the top.

Ok, I re-read the code: let's look at these cases:

case 1: checkpointee wasn't in syscall -- no problem.

case 2: checkpointee was in syscall, no signal pending; when it was
frozen, regs->svcnr became 0, and that's what we save, so on restart
we won't enter that snippet again. Again, no problem.

case 3: checkpointee was in syscall, signal pending;
case 4: checkpointee was in syscall, signal received at restart;
look at this snippet:

        if (signr > 0 && regs->psw.addr == restart_addr) {
                if (retval == -ERESTARTNOHAND
                    || (retval == -ERESTARTSYS
                         && !(current->sighand->action[signr-1].sa.sa_flags
                              & SA_RESTART))) {
                        regs->gprs[2] = -EINTR;
                        regs->psw.addr = continue_addr;
                }
        }

Because svcnr is/was 0, neither restart_addr nor continue_addr
were setup, so this condition is always false, which I think is
wrong. In particular if the original return value was one of
these two. Also, if the signal arrives _after_ the restart
completes... ?

case 5: receives a signal during restart -- restart should fail.

Oren.

> 
> In the first case we're doing exactly what we wanted to.
> 
> In that second case, we enter do_signal with very different
> initial conditions than the checkpointed case:  regs->svcnr is 0,
> so none of the gprs[2] or svcnr or psw-addr tweaking that
> would have happened the first time will happen.  We'll just
> handle the signal (if any), then, upon exit of do_signal,
> proceed again with regs->gprs[2] == __NR_restart_syscall.
> 
> But, since thread_info_flags->TIF_RESTARTBLOCK is set,
> if we get frozen and checkpointed again during the
> get_signal_to_deliver(), a restart of that image should
> be exactly the same as a restart of the current image.
> 
> (That, at least, is my intent and understanding :)
> 
> -serge
> 
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers