[CRIU] [PATCH RFC] compel: Save thread registers before executing parasite code

[Michael Holzheu]

Am Wed, 19 Jul 2017 23:05:38 +0300
schrieb Dmitry Safonov <0x7f454c46 at gmail.com>:

> 2017-07-19 21:54 GMT+03:00 Michael Holzheu <holzheu at linux.vnet.ibm.com>:
> > For dumping threads we execute parasite code before we collect the
> > floating point registers with get_task_regs().
> >
> > The following describes the code path were this is done:
> >
> >  dump_task_threads()
> >      for (i = 0; i < item->nr_threads; i++)
> >          dump_task_thread()
> >            parasite_dump_thread_seized()
> >              compel_prepare_thread()
> >                 prepare_thread()
> >
> >                    ### Get general purpose registers ###
> >                    ptrace_get_regs(ctx->regs)
> >
> >              ### Parasite code is executed in thread ###
> >              compel_run_in_thread(tctl, PARASITE_CMD_DUMP_THREAD)
> >
> >              compel_get_thread_regs()
> >
> >                  ### Get FP and VX registers ###
> >                  get_task_regs()
> >
> > On s390 gcc uses floating point registers to cache general purpose
> > registers. Therefore we use the -msoft-float option to ensure that
> > floating point registers are not clobbered in compel_run_in_thread().
> >
> > There might be a better option:
> >
> > First save the thread registers and then call the parasite code. This
> > allows us to remove the -msoft-float on s390.
> >
> > Signed-off-by: Michael Holzheu <holzheu at linux.vnet.ibm.com>
> > ---
> >  Makefile.compel           |  4 ++--
> >  criu/parasite-syscall.c   | 11 +++++------
> >  criu/pie/Makefile         |  7 -------
> >  criu/pie/Makefile.library |  6 ------
> >  4 files changed, 7 insertions(+), 21 deletions(-)
> >
> > diff --git a/Makefile.compel b/Makefile.compel
> > index 5c854e3..d200cb8 100644
> > --- a/Makefile.compel
> > +++ b/Makefile.compel
> > @@ -77,6 +77,6 @@ compel-install-targets        += $(compel-plugins)
> >  # commit "S/390: Fix 64 bit sibcall".
> >  #
> >  ifeq ($(ARCH),s390)
> > -CFLAGS += -msoft-float -fno-optimize-sibling-calls
> > -HOSTCFLAGS += -msoft-float -fno-optimize-sibling-calls
> > +CFLAGS += -fno-optimize-sibling-calls
> > +HOSTCFLAGS += -fno-optimize-sibling-calls
> >  endif
> > diff --git a/criu/parasite-syscall.c b/criu/parasite-syscall.c
> > index 69dba60..76e943e 100644
> > --- a/criu/parasite-syscall.c
> > +++ b/criu/parasite-syscall.c
> > @@ -198,6 +198,11 @@ int parasite_dump_thread_seized(struct parasite_ctl *ctl, const struct pstree_it
> >
> >         tc->has_blk_sigset = true;
> >         memcpy(&tc->blk_sigset, compel_thread_sigmask(tctl), sizeof(k_rtsigset_t));
> > +       ret = compel_get_thread_regs(tctl, save_task_regs, core);
> > +       if (ret) {
> > +               pr_err("Can't obtain regs for thread %d\n", pid);
> > +               goto err_rth;
> > +       }
> 
> That *looks* not very correct by the following reason:
> 
> ucontext_extended has fpu/vxr registers on it, so for the *task*
> they will be restored on parasite's CMD_FINI.
> And you get them before running parasite in a task - that's ok.
> 
> But from the point of *thread*'s view - you dump them here,
> run parasite inside thread, which can change fpu/vxr, but only
> restore generic registers after (parasite_run()=>ptrace_set_regs()).
> 
> That means that dumping thread's core may corrupt fpu/vxr, which
> will break as the result --leave-running and err-path recovery curing
> for the parasite.

You are right, I forgot --leave-running and the error path.

So the current CRIU design is that the parasite code must not change
floating point and VX registers?

Are you sure that on the other architectures FP and VX registers are
not changed by auto-vectorization or other fancy optimization stuff
with -O2?
 
> Please, also correct comment near ptrace_get_regs() for s390.

You mean, if I provide an updated patch that removes soft-float?

Michael