On 04/19, Roman Gushchin wrote:
>
> > Once again, suppose we race with CGRP_FREEZE. If JOBCTL_TRAP_FREEZE is already
> > set then signal_pending() must be already T and we do not need recalc_sigpending?
> > If JOBCTL_TRAP_FREEZE is not set yet, how can recalc_sigpending() help?
>
> This is paired with cgroup_task_frozen() check in recalc_sigpending_tsk().
Ooh, I didn't notice this version added cgroup_task_frozen() into
recalc_sigpending_tsk() ...
Honestly, I don't like this. But see another email I sent, we can cleanup
this code later.
> > > +static void cgroup_freeze_task(struct task_struct *task, bool freeze)
> > > +{
> > > + unsigned long flags;
> > > +
> > > + /* If the task is about to die, don't bother with freezing it. */
> > > + if (!lock_task_sighand(task, &flags))
> > > + return;
> > > +
> > > + if (freeze) {
> > > + task->jobctl |= JOBCTL_TRAP_FREEZE;
> > > + signal_wake_up(task, false);
> > > + } else {
> > > + task->jobctl &= ~JOBCTL_TRAP_FREEZE;
> > > + wake_up_process(task);
> >
> > wake_up_interruptible() ?
>
> Wait_up_interruptible() is supposed to work with a workqueue,
> but here there is nothing like this. Probably, I didn't understand your idea.
> Can you, please, elaborate a bit more?
Not sure I understand... We need to wake up the task if it sleeps in
do_freezer_trap(), right? do_freezer_trap() uses TASK_INTERRUPTIBLE, so
why can't wake_up_interruptible() == __wake_up(TASK_INTERRUPTIBLE) work?
> > > static int ptrace_signal(int signr, kernel_siginfo_t *info)
> > > {
> > > /*
> > > @@ -2442,6 +2483,10 @@ bool get_signal(struct ksignal *ksig)
> > > ksig->info.si_signo = signr = SIGKILL;
> > > sigdelset(¤t->pending.signal, SIGKILL);
> > > recalc_sigpending();
> > > + current->jobctl &= ~JOBCTL_TRAP_FREEZE;
> > > + spin_unlock_irq(&sighand->siglock);
> > > + if (unlikely(cgroup_task_frozen(current)))
> > > + cgroup_leave_frozen(true);
> >
> > Oh, and another leave_frozen below...
>
> Yeah, because of this new "goto fatal" shortcut.
I understand, but the code doesn't look nice... but again, I can't suggest
anything better at least right now, so please forget.
> > > + if (unlikely(cgroup_task_frozen(current))) {
> > > spin_unlock_irq(&sighand->siglock);
> > > + cgroup_leave_frozen(true);
> > > goto relock;
> > > }
> >
> > afaics cgroup_leave_frozen(false) makes more sense here.
>
> Why? I don't see any reasons why the task should remain in the frozen
> state after this point.
But cgroup_leave_frozen(false) will equally clear ->frozen if !CGRP_FREEZE ?
OTOH, if CGRP_FREEZE is set again, why do we need to clear ->frozen?
Oleg.
