[CRIU] [PATCH 2/2] ppc64: handle transactional memory state

[Laurent Dufour]

On 31/08/2016 14:15, Dmitry Safonov wrote:
> 2016-08-31 13:06 GMT+03:00 Laurent Dufour <ldufour at linux.vnet.ibm.com>:
>> The Power 8 introduces the transactional memory (TM) operations (see
>> Power ISA 3.0 for details).
>>
>> The support for the transactional memory operation during the
>> checkpoint and restart requires extended ptrace API provided by the
>> kernel 4.8.
>>
>> When checkpointing a thread while a transactional memory operation is
>> in progress, the TM checkpointed state is checkpointed through the new
>> ptrace API. If these new APIs are not available, the checkpoint is
>> aborted and an explicit error is reported.
>>
>> At restart time, the TM state is pushed on the stack frame to be
>> reloaded by the kernel when reading the stack frame.
>>
>> Only suspended TM operation could be checkpointed since active one
>> will be aborted once a system call is made. Suspended operation will
>> be aborted as well, and the checkpointed thread is expected to handle
>> the TM failure as usual (retrying is a good option).
>>
>> Signed-off-by: Laurent Dufour <ldufour at linux.vnet.ibm.com>
> 
> Oh, what a brilliant stuff!
> Small nitpicks below.

Hi Dimirty,

Thanks for your prompt review.
I'll send a V2 soon.

>> +static int get_tm_regs(pid_t pid, CoreEntry *core)
>> +{
>> +       struct {
>> +               uint64_t tfhar, texasr, tfiar;
>> +       } tm_spr_regs;
>> +       user_regs_struct_t regs;
>> +       uint64_t fpregs[NFPREG], vmxregs[34][2], vsxregs[32];
> 
> Maybe add definitions like?
> #define NVMXREG 34
> #define NVSXREG 32

I agree.
For Altivec registers (vmx), I'll rely on the NVRREG system define, and
for VSX I'll introduce a new one.

> 
>> +       struct iovec iov;
>> +       UserPpc64TmRegsEntry *tme = NULL;
> 
> Not needed initialization

Right

> [...]
> 
>> +static int put_tm_regs(struct rt_sigframe *f, UserPpc64TmRegsEntry *tme)
>> +{
>> +/* WARNING: As stated in kernel's restore_tm_sigcontexts, TEXASR has to be
>> + * restored by the process itself :
>> + *   TEXASR was set by the signal delivery reclaim, as was TFIAR.
>> + *   Users doing anything abhorrent like thread-switching w/ signals for
>> + *   TM-Suspended code will have to back TEXASR/TFIAR up themselves.
>> + *   For the case of getting a signal and simply returning from it,
>> + *   we don't need to re-copy them here.
>> + */
> 
> Netlayer-alike comment style, don't mind, but AFAIK CRIU uses another style.

Fair enough ;)

> 
>> @@ -415,35 +579,60 @@ int get_task_regs(pid_t pid, user_regs_struct_t regs, CoreEntry *core)
>>         /* Resetting trap since we are now coming from user space. */
>>         regs.trap = 0;
>>
>> -       copy_gp_regs(core->ti_ppc64->gpregs, &regs);
>> +       /* Check for Transactional Memory operation in progress.
>> +        * Until we have support of TM register's state through the ptrace API,
>> +        * we can't checkpoint process with TM operation in progress (almost
>> +        * impossible) or suspended (easy to get).
>> +        */
> 
> And another
> 
>> +       if (MSR_TM_ACTIVE(regs.msr)) {
>> +               pr_debug("Task %d has %s TM operation at 0x%lx\n",
>> +                        pid,
>> +                        (regs.msr & MSR_TMS) ? "a suspended" : "an active",
>> +                        regs.nip);
>> +               if (get_tm_regs(pid, core))
>> +                       return -1;
>> +
>> +               gpregs = core->ti_ppc64->tmstate->gpregs;
>> +               fpstate = &(core->ti_ppc64->tmstate->fpstate);
>> +               vrstate = &(core->ti_ppc64->tmstate->vrstate);
>> +               vsxstate = &(core->ti_ppc64->tmstate->vsxstate);
>> +       }
>> +       else {
> 
> Reverse egyptian braces

What do you mean here ?

> 
>> @@ -515,6 +706,14 @@ int restore_fpu(struct rt_sigframe *sigframe, CoreEntry *core)
>>                 ret = -1;
>>         }
>>
>> +       if (!ret && CORE_THREAD_ARCH_INFO(core)->tmstate)
>> +               ret = put_tm_regs(sigframe,
>> +                                 CORE_THREAD_ARCH_INFO(core)->tmstate);
>> +       else if (MSR_TM_ACTIVE(core->ti_ppc64->gpregs->msr)) {
>> +               pr_err("Internal error\n");
>> +               ret = -1;
> 
> AFAIU, that means TM transaction in progress, right?
> CRIU does not use them, so should we BUG() here, or
> just fail with an error?

That's mostly a bug here since this will mean the checkpointed data read
at restart time are not consistent.
I'll rather change the error message since this is not really a bug.

> 
> 
>> diff --git a/images/core-ppc64.proto b/images/core-ppc64.proto
>> index 5bdec9c07809..794a1b029856 100644
>> --- a/images/core-ppc64.proto
>> +++ b/images/core-ppc64.proto
>> @@ -11,6 +11,10 @@ message user_ppc64_regs_entry {
>>         required uint64 xer             = 7;
>>         required uint64 ccr             = 8;
>>         required uint64 trap            = 9;
>> +       // For Transactional memory support since P8
> 
> C++ alike comment

Fair enough.

> 
>> +       optional uint64 texasr          = 10;
>> +       optional uint64 tfhar           = 11;
>> +       optional uint64 tfiar           = 12;
>>  }
>>
>>  message user_ppc64_fpstate_entry {
>> @@ -41,10 +45,19 @@ message user_ppc64_vsxstate_entry {
>>         repeated uint64 vsxregs         = 1;
>>  }
>>
>> +// Transactional memory operation's state
> 
> Another one.
>