Re: [PATCH v10 4/9] cgroup: cgroup v2 freezer

[Roman Gushchin <guro@fb.com>]

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(&current->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!