On Fri, Apr 19, 2019 at 06:26:00PM +0200, Oleg Nesterov wrote:
> 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.
Yeah, totally agree: it's not pretty. But honestly I've no better ideas,
so let's fix it 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?
Right, but __wake_up is supposed to wake threads blocked on a waitqueue:
/**
* __wake_up - wake up threads blocked on a waitqueue.
* @wq_head: the waitqueue
* @mode: which threads
* @nr_exclusive: how many wake-one or wake-many threads to wake up
* @key: is directly passed to the wakeup function
*
* If this function wakes up a task, it executes a full memory barrier before
* accessing the task state.
*/
void __wake_up(struct wait_queue_head *wq_head, unsigned int mode,
int nr_exclusive, void *key)
What should I pass as wq_head?
>
> > > > 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?
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.
Thank you!
