From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.9 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 18553C47096 for ; Thu, 3 Jun 2021 11:01:09 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id D4A1260FEE for ; Thu, 3 Jun 2021 11:01:08 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D4A1260FEE Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=knXkFDoDjhvxAHvbgTXAzRJlRgOb8Iisk907o9JwmvI=; b=hC/+JKzLY7U+lx bBUTbK8emQ5lYrSZTKRiCsx8PSn1W7mJgMzKWjOnoP5+jelK0zkENyUwZWvzhET55xsCK4nkie7x1 +vv28kkIYJdrotps3q0tHzZrFpJ1f3GsgGDi8oGepOnByidWmtysylNXkMDdvV0A4rTIf0u2Mr3xp +ozkZrqi6lQfgunizT25DOnIba/6T5fpsrPWQpKf9GcYw6LeVNMUaYzgWxfOo3wWfef80+n8ow3s6 yALGBd8hnU13TvZCvIqTwW4q2yggXLgGy2vqKGH9z/dk7T5WJcdd+XEuAzQnAOsOqN7lwkVh8HBwc g750qrbZqUr4A2XRDfdQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1lol46-008CHY-CI; Thu, 03 Jun 2021 10:59:06 +0000 Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1lol43-008CGs-8C for linux-arm-kernel@lists.infradead.org; Thu, 03 Jun 2021 10:59:04 +0000 Received: by mail.kernel.org (Postfix) with ESMTPSA id B2EA860FEE; Thu, 3 Jun 2021 10:58:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1622717942; bh=KemI2Hd6fvYHivyCBcAgtmkUK8KyXDbKOjArerr5H9Y=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=mOskmeuYODybu+Le9EqSFv8m4vtXmVKZPX3iqKfCZYwJmQMPwjyCcXLrQGCmXYRC1 jRhZ5CmlVW3uJwkW4mjwdGnm77gOJnpm4BwStCecnpNNt4IxvqloKhUYFuE9Xt5oTl efqZJhkeTnXqbdKb0VfUReNCy5EuP2efSntBGzXizL89biXX5DreRvA7vjeMeJV70o IkElmJT4B45BNSEhUQrCvvb5jW4r9egH9irbFQLD4JicQIlmu9OHNHXLpj+1yqGkMy 8DF81nhLwXg6b4k2JjiW9jfHYshGbtGqe269lBYwUxbuOuf1sYVfwQQiqQYqSN792z iLW7l/kGfxn9g== Date: Thu, 3 Jun 2021 11:58:56 +0100 From: Will Deacon To: Peter Zijlstra Cc: linux-arm-kernel@lists.infradead.org, linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org, Catalin Marinas , Marc Zyngier , Greg Kroah-Hartman , Morten Rasmussen , Qais Yousef , Suren Baghdasaryan , Quentin Perret , Tejun Heo , Johannes Weiner , Ingo Molnar , Juri Lelli , Vincent Guittot , "Rafael J. Wysocki" , Dietmar Eggemann , Daniel Bristot de Oliveira , kernel-team@android.com, Oleg Nesterov Subject: Re: [RFC][PATCH] freezer,sched: Rewrite core freezer logic Message-ID: <20210603105856.GB32641@willie-the-truck> References: <20210525151432.16875-1-will@kernel.org> <20210525151432.16875-17-will@kernel.org> <20210602125452.GG30593@willie-the-truck> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210603_035903_358566_CD1825CE X-CRM114-Status: GOOD ( 36.13 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Jun 03, 2021 at 12:35:22PM +0200, Peter Zijlstra wrote: > On Wed, Jun 02, 2021 at 01:54:53PM +0100, Will Deacon wrote: > > > There's also Documentation/power/freezing-of-tasks.rst to update. I'm not > > Since it's .rst, the only update I'm willing to do is delete it > outright. Hah! Well, don't do that. > > sure if fs/proc/array.c should be updated to display frozen tasks; I > > couldn't see how that was useful, but thought I'd mention it anyway. > > Yeah, I considered it too, but I figured that if we're all frozen > there's noone left to observe us being frozen, so I didn't bother. Agreed. > > > diff --git a/include/linux/sched.h b/include/linux/sched.h > > > index 2982cfab1ae9..bfadc1dbcf24 100644 > > > --- a/include/linux/sched.h > > > +++ b/include/linux/sched.h > > > @@ -95,7 +95,12 @@ struct task_group; > > > #define TASK_WAKING 0x0200 > > > #define TASK_NOLOAD 0x0400 > > > #define TASK_NEW 0x0800 > > > -#define TASK_STATE_MAX 0x1000 > > > +#define TASK_FREEZABLE 0x1000 > > > +#define __TASK_FREEZABLE_UNSAFE 0x2000 > > > > Give that this is only needed to avoid lockdep checks, maybe we should avoid > > allocating the bit if lockdep is not enabled? Otherwise, people might start > > to use it for other things. > > Something like > > #define __TASK_FREEZABLE_UNSAFE (0x2000 * IS_ENABLED(CONFIG_LOCKDEP)) > > ? Yup. > > > +#define TASK_FROZEN 0x4000 > > > +#define TASK_STATE_MAX 0x8000 > > > + > > > +#define TASK_FREEZABLE_UNSAFE (TASK_FREEZABLE | __TASK_FREEZABLE_UNSAFE) > > > > We probably want to preserve the "DO NOT ADD ANY NEW CALLERS OF THIS STATE" > > comment for the unsafe stuff. > > Done. Thanks. > > > +/* Recursion relies on tail-call optimization to not blow away the stack */ > > > +static bool __frozen(struct task_struct *p) > > > +{ > > > + if (p->state == TASK_FROZEN) > > > + return true; > > > > READ_ONCE()? > > task_struct::state is volatile -- for now. I've got other patches to > deal with that. Thanks, I missed that and have since reviewed your other series. > > > @@ -116,20 +173,8 @@ bool freeze_task(struct task_struct *p) > > > { > > > unsigned long flags; > > > > > > spin_lock_irqsave(&freezer_lock, flags); > > > + if (!freezing(p) || frozen(p) || __freeze_task(p)) { > > > spin_unlock_irqrestore(&freezer_lock, flags); > > > return false; > > > } > > > > I've been trying to figure out how this serialises with ttwu(), given that > > frozen(p) will go and read p->state. I suppose it works out because only the > > freezer can wake up tasks from the FROZEN state, but it feels a bit brittle. > > p->pi_lock; both ttwu() and __freeze_task() (which is essentially a > variant of set_special_state()) take ->pi_lock. I'll put in a comment. The part I struggled with was freeze_task(), which doesn't take ->pi_lock yet calls frozen(p). > > > > @@ -137,7 +182,7 @@ bool freeze_task(struct task_struct *p) > > > if (!(p->flags & PF_KTHREAD)) > > > fake_signal_wake_up(p); > > > else > > > - wake_up_state(p, TASK_INTERRUPTIBLE); > > > + wake_up_state(p, TASK_INTERRUPTIBLE); // TASK_NORMAL ?!? > > > > > > spin_unlock_irqrestore(&freezer_lock, flags); > > > return true; > > > @@ -148,8 +193,8 @@ void __thaw_task(struct task_struct *p) > > > unsigned long flags; > > > > > > spin_lock_irqsave(&freezer_lock, flags); > > > - if (frozen(p)) > > > - wake_up_process(p); > > > + WARN_ON_ONCE(freezing(p)); > > > + wake_up_state(p, TASK_FROZEN | TASK_NORMAL); > > > > Why do we need TASK_NORMAL here? > > It's a left-over from hacking, but I left it in because anything > TASK_NORMAL should be able to deal with spuriuos wakeups, something > try_to_freeze() now also relies on. I just worry that it might hide bugs if TASK_FROZEN is supposed to be sufficient, as it would imply that we have some unfrozen tasks kicking around. I dunno, maybe just a comment saying that everything _should_ be FROZEN at this point? Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel