On Wed, Apr 24, 2019 at 05:46:19PM +0200, Oleg Nesterov wrote: > On 04/22, Roman Gushchin wrote: > > > > > > Hm, it might work too, but I'm not sure I like it more. IMO, the best option > > > > is to have a single cgroup_leave_frozen(true) in signal.c, it's just simpler. > > > > If a user changed the desired state of cgroup twice, there is no need to avoid > > > > state transitions. Or maybe I don't see it yet. > > > > > > Then why do we need cgroup_leave_frozen(false) in wait_for_vfork_done() ? How > > > does it differ from get_signal() ? > > > > We need it because sleeping in vfork is a special state which we want to > > account as frozen. And if the parent process wakes up while the cgroup is frozen > > (because of the child death, for example), we want to push it into the "proper" > > frozen state without changing the state of the cgroup. > > Again, I do not see how vfork() differs from get_signal() in this respect. > > Let me provide another example. A TASK_STOPPED task reacts to SIGCONT and > returns to get_signal(), current->frozen is true. > > If this races with CGRP_FREEZE, the task should not return to user-space, > just like vfork(). I see no difference. > > They differ in that wait_for_vfork_done() should guarentee TIF_SIGPENDING > in this case, but this is another story... Right, I agree. > > > > > > If nothing else. Suppose that wait_for_vfork_done() calls leave(false) and this > > > races with freezer, CGRP_FREEZE is already set but JOBCTL_TRAP_FREEZE is not. > > > > > > This sets TIF_SIGPENDING to ensure the task won't return to user mode, thus it > > > calls get_signal(). > > > > > > get_signal() doesn't see JOBCTL_TRAP_FREEZE, it notices ->frozen == T and does > > > cgroup_leave_frozen(true) which clears ->frozen. > > > > > > Then the task calls dequeue_signal(), clears TIF_SIGPENDING and returns to user > > > mode? > > > > Got it, a good catch! So if the freezer races with vfork() completion, we might > > have a spurious frozen->unfrozen->frozen transition of the cgroup state. > > > > Switching to cgroup_leave_frozen(false) seems to solve it, but I'm slightly > > concerned that we're basically putting the task in a busy loop between > > the setting CGRP_FREEZE and setting TRAP_FREEZE. > > Yes, yes. Didn't I say I dislike the new ->frozen check in recalc() ? ;) > > OK, how about the ABSOLUTELY UNTESTED patch below? For the start. It looks good to me (and all freezer selftests pass). Just to be sure, is it a solution to avoid the busy loop in the signal handling loop, right? Because it doesn't allow to drop the ->frozen check from recalc(). The JOBCTL_TRAP_FREEZE check without siglock initially looked dangerous to me, but after some thoughts I didn't find any case when it's wrong. Do you prefer me to master a patch or to do it by yourself? Thank you! Roman
