Quoting Oren Laadan (orenl@xxxxxxxxxxxxxxx):
>
>
> Serge E. Hallyn wrote:
> >Quoting Martin Schwidefsky (schwidefsky@xxxxxxxxxx):
> >>On Wed, 10 Feb 2010 14:40:19 -0600
> >>"Serge E. Hallyn" <serue@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-s390" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
