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=-8.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,LOTS_OF_MONEY,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,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 56842C2D0E4 for ; Tue, 17 Nov 2020 09:16:29 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (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 BB40A20E65 for ; Tue, 17 Nov 2020 09:16:26 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="w1N6QJd1"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="ATn4XYy1" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org BB40A20E65 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=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject: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=rKx8ipABQeiGRWXPWFcDDqXOQnPYTGIwrQFKsy5CBVY=; b=w1N6QJd1QBJ2u+81PI68SXz7d L4BXCYlb8wu0InCL2iwCY3+wqjaogxeCfusI6of7nGBfflKU07UpRvplOyrcVzkTZO3J8EOvUGN7Z 20ULxEj8L5zcmJKVBOcbvLJp1MRP9Q1u9Yq5fgWSYNjwtHym9tv++VN3AgTj2BtAtOrAqD7M6AJ/v 1t4m0Tm8207WcgsF+ZILNk6i5WiYMHO69Lc3gOIPi5BnnQYotZ/Ki8JX7RjlZltYxV4u+SLh1WWMX J49LyT0Z0POn81tANgcje0fvBwCUjYWJDRWrORIPxLAnppxZkUfJChmoVPg6w/kuXT/Xca2bv3Kp6 3MdL3IJVQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kex5f-00071A-2s; Tue, 17 Nov 2020 09:15:55 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kex5c-00070W-JO for linux-arm-kernel@lists.infradead.org; Tue, 17 Nov 2020 09:15:53 +0000 Received: from willie-the-truck (236.31.169.217.in-addr.arpa [217.169.31.236]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 9952C2417E; Tue, 17 Nov 2020 09:15:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1605604550; bh=U+FYpbgrS49KuCOaHirSy6jXNIfRFAzW9rFOhILS5KY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ATn4XYy1NEKvOCvE0M0Asx1ReNHEc/aBqBdkRXRDuGScqCbsS+fWt51TiulICPfL/ Hs8z/fVGlQUBqMSRmZry70txOGjOKH6wyJeO4fhd/11Jac4CRejULLhO+ZyXwOCA/F l8LSaNO/Eh/i6oM633/NSI4qxgvDOfLcc3iVkDcA= Date: Tue, 17 Nov 2020 09:15:46 +0000 From: Will Deacon To: Peter Zijlstra Subject: Re: [PATCH] sched: Fix data-race in wakeup Message-ID: <20201117091545.GA31837@willie-the-truck> References: <20201116091054.GL3371@techsingularity.net> <20201116131102.GA29992@willie-the-truck> <20201116133721.GQ3371@techsingularity.net> <20201116142005.GE3121392@hirez.programming.kicks-ass.net> <20201116193149.GW3371@techsingularity.net> <20201117083016.GK3121392@hirez.programming.kicks-ass.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20201117083016.GK3121392@hirez.programming.kicks-ass.net> 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-20201117_041553_021380_9D666EEC X-CRM114-Status: GOOD ( 31.31 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Davidlohr Bueso , Mel Gorman , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org 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 Tue, Nov 17, 2020 at 09:30:16AM +0100, Peter Zijlstra wrote: > On Mon, Nov 16, 2020 at 07:31:49PM +0000, Mel Gorman wrote: > > > And this works. > > Yay! > > > sched_psi_wake_requeue can probably stay with the other three fields > > given they are under the rq lock but sched_remote_wakeup needs to move > > out. > > I _think_ we can move the bit into the unserialized section below. > > It's a bit cheecky, but it should work I think because the only time we > actually use this bit, we're guaranteed the task isn't actually running, > so current doesn't exist. > > I suppose the question is wether this is worth saving 31 bits over... > > How's this? > > --- > Subject: sched: Fix data-race in wakeup > From: Peter Zijlstra > Date: Tue Nov 17 09:08:41 CET 2020 > > Mel reported that on some ARM64 platforms loadavg goes bananas and > tracked it down to the following data race: > > CPU0 CPU1 > > schedule() > prev->sched_contributes_to_load = X; > deactivate_task(prev); > > try_to_wake_up() > if (p->on_rq &&) // false > if (smp_load_acquire(&p->on_cpu) && // true > ttwu_queue_wakelist()) > p->sched_remote_wakeup = Y; > > smp_store_release(prev->on_cpu, 0); (nit: I suggested this race over at [1] ;) > where both p->sched_contributes_to_load and p->sched_remote_wakeup are > in the same word, and thus the stores X and Y race (and can clobber > one another's data). > > Whereas prior to commit c6e7bd7afaeb ("sched/core: Optimize ttwu() > spinning on p->on_cpu") the p->on_cpu handoff serialized access to > p->sched_remote_wakeup (just as it still does with > p->sched_contributes_to_load) that commit broke that by calling > ttwu_queue_wakelist() with p->on_cpu != 0. > > However, due to > > p->XXX ttwu() > schedule() if (p->on_rq && ...) // false > smp_mb__after_spinlock() if (smp_load_acquire(&p->on_cpu) && > deactivate_task() ttwu_queue_wakelist()) > p->on_rq = 0; p->sched_remote_wakeup = X; > > We can be sure any 'current' store is complete and 'current' is > guaranteed asleep. Therefore we can move p->sched_remote_wakeup into > the current flags word. > > Note: while the observed failure was loadavg accounting gone wrong due > to ttwu() cobbering p->sched_contributes_to_load, the reverse problem > is also possible where schedule() clobbers p->sched_remote_wakeup, > this could result in enqueue_entity() wrecking ->vruntime and causing > scheduling artifacts. > > Fixes: c6e7bd7afaeb ("sched/core: Optimize ttwu() spinning on p->on_cpu") > Reported-by: Mel Gorman > Signed-off-by: Peter Zijlstra (Intel) > --- > include/linux/sched.h | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -775,7 +775,6 @@ struct task_struct { > unsigned sched_reset_on_fork:1; > unsigned sched_contributes_to_load:1; > unsigned sched_migrated:1; > - unsigned sched_remote_wakeup:1; > #ifdef CONFIG_PSI > unsigned sched_psi_wake_requeue:1; > #endif > @@ -785,6 +784,18 @@ struct task_struct { > > /* Unserialized, strictly 'current' */ > > + /* > + * p->in_iowait = 1; ttwu() > + * schedule() if (p->on_rq && ..) // false > + * smp_mb__after_spinlock(); if (smp_load_acquire(&p->on_cpu) && //true > + * deactivate_task() ttwu_queue_wakelist()) > + * p->on_rq = 0; p->sched_remote_wakeup = X; > + * > + * Guarantees all stores of 'current' are visible before > + * ->sched_remote_wakeup gets used. I'm still not sure this is particularly clear -- don't we want to highlight that the store of p->on_rq is unordered wrt the update to p->sched_contributes_to_load() in deactivate_task()? I dislike bitfields with a passion, but the fix looks good: Acked-by: Will Deacon Now the million dollar question is why KCSAN hasn't run into this. Hrmph. Will [1] https://lore.kernel.org/r/20201116131102.GA29992@willie-the-truck _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel