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

[Dmitry Safonov]

2016-08-31 18:45 GMT+03:00 Laurent Dufour <ldufour at linux.vnet.ibm.com>:
> On 31/08/2016 17:17, Dmitry Safonov wrote:
>> 2016-08-31 17:47 GMT+03:00 Laurent Dufour <ldufour at linux.vnet.ibm.com>:
>>> Hi Dimirty,
>>>
>>> Thanks for your prompt review.
>>> I'll send a V2 soon.
>>
>> Heh, no problem. Thanks for a good work!
>> Unfortunately, I'm not good at Power 8 registers/arch manuals,
>> so can only review it from the top.
>>
>>>>> @@ -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 ?
>>
>> Heh, well, I meant excessive new line ;)
>> (3 here: https://blog.codinghorror.com/new-programming-jargon/ )
>
> Where are these "excessive new line" ?

In get_task_regs():
+       }
+       else {

>
>>>
>>>>
>>>>> @@ -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.
>>
>> Ok, just wanted to know if this is an error or bug.
>> I'm fine with message as-is.
>>
>



-- 
             Dmitry