Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Roman Gushchin <guro@fb.com>
To:     Oleg Nesterov <oleg@redhat.com>
CC:     Roman Gushchin <guroan@gmail.com>, Tejun Heo <tj@kernel.org>,
        Kernel Team <Kernel-team@fb.com>,
        "cgroups@vger.kernel.org" <cgroups@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v10 4/9] cgroup: cgroup v2 freezer
Thread-Topic: [PATCH v10 4/9] cgroup: cgroup v2 freezer
Thread-Index: AQHU69erRVR2Ctc4e0afYj1p+92aPKZDrswA//+ZOACAAHlyAIAACGIA
Date:   Fri, 19 Apr 2019 16:56:04 +0000
Message-ID: <20190419165600.GC23357@tower.DHCP.thefacebook.com>
References: <20190405174708.1010-1-guro@fb.com>
 <20190405174708.1010-5-guro@fb.com> <20190419151912.GA12152@redhat.com>
 <20190419161118.GA23357@tower.DHCP.thefacebook.com>
 <20190419162600.GC12228@redhat.com>
In-Reply-To: <20190419162600.GC12228@redhat.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-messagesentrepresentingtype: 1
x-originating-ip: [2620:10d:c090:200::1:66a3]
x-ms-publictraffictype: Email
x-ms-office365-filtering-correlation-id: be4b065b-51e6-45e0-a90d-08d6c4e7e8f7
x-microsoft-antispam: BCL:0;PCL:0;RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600141)(711020)(4605104)(2017052603328)(7193020);SRVR:BYAPR15MB2389;
x-ms-traffictypediagnostic: BYAPR15MB2389:
x-microsoft-antispam-prvs: <BYAPR15MB2389242FDEA54ED0BEEB6C44BE270@BYAPR15MB2389.namprd15.prod.outlook.com>
x-forefront-prvs: 0012E6D357
x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(396003)(376002)(366004)(39860400002)(136003)(346002)(51234002)(189003)(199004)(6246003)(14444005)(256004)(33656002)(4326008)(25786009)(93886005)(66556008)(66476007)(9686003)(66446008)(64756008)(66946007)(5660300002)(6512007)(1076003)(6116002)(186003)(52116002)(97736004)(478600001)(446003)(76176011)(11346002)(8676002)(99286004)(14454004)(486006)(476003)(46003)(316002)(2906002)(54906003)(102836004)(6506007)(386003)(7736002)(305945005)(73956011)(229853002)(6486002)(71190400001)(6916009)(6436002)(81166006)(68736007)(81156014)(86362001)(8936002)(71200400001)(53936002);DIR:OUT;SFP:1102;SCL:1;SRVR:BYAPR15MB2389;H:BYAPR15MB2631.namprd15.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;MX:1;A:1;
received-spf: None (protection.outlook.com: fb.com does not designate
 permitted sender hosts)
x-ms-exchange-senderadcheck: 1
x-microsoft-antispam-message-info: jYUkY4ZtPEkcNb5ehCO7rr9Tk914lFa5QKketvmcWi0cB72cIGJ98xD/oiyp8+6MaOxPTEQwNK0FCvZs2tW0joDWcogs/QJt8czOSQQLY16M5pfeHfJAuspf0NQU7/Uh/tTXIzLrmYX/W1EppJJnDHMSddwv2lsA6iC959w7rXPH1VYnLSwKxmpL4/P3DjXo+1lSNiIjn+XFDxtNvvllo+Mmemj2joWEU+TDJoDb+xwc127WoZQA/PYeg7uOSDpIs8q5hMrOEISubGp6khjP8P2kOok/57ahE7l6CmPHcCst0i20noCPrx78zmDU816YEvn7LTsuePfLrOVaGGig1j1jwC+68y8FbXu6SHFLQJ/7iIwRE4ODHuyj0K31LhTWXbte9Y7ofr0H1TvqUXtzRnf53WTSutdy2xfYxbVow/o=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <F937F0E16108E8498B9720C7BDC65529@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: be4b065b-51e6-45e0-a90d-08d6c4e7e8f7
X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Apr 2019 16:56:04.7340
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR15MB2389
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-04-19_09:,,
 signatures=0
X-Proofpoint-Spam-Reason: safe
X-FB-Internal: Safe
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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 i=
s 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() hel=
p?
> >
> > This is paired with cgroup_task_frozen() check in recalc_sigpending_tsk=
().
>=20
> Ooh, I didn't notice this version added cgroup_task_frozen() into
> recalc_sigpending_tsk() ...
>=20
> 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.

>=20
> > > > +static void cgroup_freeze_task(struct task_struct *task, bool free=
ze)
> > > > +{
> > > > +	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 |=3D JOBCTL_TRAP_FREEZE;
> > > > +		signal_wake_up(task, false);
> > > > +	} else {
> > > > +		task->jobctl &=3D ~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?
>=20
> 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() =3D=3D __wake_up(TASK_INTERRUPTIBLE) wo=
rk?

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?

>=20
> > > >  static int ptrace_signal(int signr, kernel_siginfo_t *info)
> > > >  {
> > > >  	/*
> > > > @@ -2442,6 +2483,10 @@ bool get_signal(struct ksignal *ksig)
> > > >  		ksig->info.si_signo =3D signr =3D SIGKILL;
> > > >  		sigdelset(&current->pending.signal, SIGKILL);
> > > >  		recalc_sigpending();
> > > > +		current->jobctl &=3D ~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.
>=20
> I understand, but the code doesn't look nice... but again, I can't sugges=
t
> anything better at least right now, so please forget.
>=20
> > > > +		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.
>=20
> But cgroup_leave_frozen(false) will equally clear ->frozen if !CGRP_FREEZ=
E ?
> 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 optio=
n
is to have a single cgroup_leave_frozen(true) in signal.c, it's just simple=
r.
If a user changed the desired state of cgroup twice, there is no need to av=
oid
state transitions. Or maybe I don't see it yet.

Thank you!