[CRIU] [PATCH] Dismantle cgyard in non-detached restore mode.

[Pavel Emelyanov]

On 03/18/2015 11:39 PM, Tycho Andersen wrote:
> Hi Pavel,
> 
> On Wed, Mar 18, 2015 at 11:17:29PM +0300, Pavel Emelyanov wrote:
>> On 03/18/2015 10:01 PM, Tycho Andersen wrote:
>>> Hi Saied,
>>>
>>> On Wed, Mar 18, 2015 at 11:14:31AM -0700, Saied Kazemi wrote:
>>>> Tycho,
>>>>
>>>> Thanks for looking into this.  I knew that we can't fini_cgroup() here but
>>>> because I am not familiar with the internals of cgroup restoration I just
>>>> put it there as a quick hack to get criu successfully dump and restore my
>>>> test container multiple times (didn't care about properties, etc.).
>>>>
>>>> For a permanent fix, I have to defer to your judgement on the best way to
>>>> dismantle the cgyard in the non-detached mode.  I agree with you that it's
>>>> a good idea to always fini_cgroup() as soon as we've fully restored
>>>> cgroups, regardless of detached/non-detached mode.
>>>
>>> Ok, I think the right thing to do is something like this:
>>>
>>> https://github.com/tych0/criu/commit/63e138e7b4b1b183d864fb08923176c9cd9ba545
>>>
>>> I vaguely recall some discussion about why this wasn't possible
>>> before, but I don't remember exactly what it was. 
>>
>> I can remember only one reason -- cgroup props should be restored at the very
>> end not to make restoring processes hit potentially strict limits set for memory,
>> cpu or anything else.
> 
> Oh, duh, of course.
> 
>> But other than this restoring them earlier than it's done now seems correct :)
> 
> The above patch restores the properties just before unlocking the
> network. A comment around there says:
> 
> * Below this line nothing can fail, because network is unlocked
> 
> Which I guess is below this line nothing *should* fail? 

Yup :)

> Since I think this could potentially fail (e.g. due to kernel version mismatch
> or hardware change as we've previously discussed), we have to do it above
> there. However, this means that the network restore would be limited
> by the cgroups.

No, the code in question in the criu context, not tasks', and this context
isn't (shouldn't) be affected by restored cgroups properties. And by that
time tasks are already in the RESTORE_SIGCHILD state, only creds restoration
and timers will be done under cgroups constraints, but these are not
affected by cgroups.

> Do you have any thoughts on where this should live?

I believe that the place in your patch _is_ the correct one. Unless someone
tries and seed that it's not :)

Thanks,
Pavel