From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f52.google.com (mail-wm1-f52.google.com [209.85.128.52]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id E2BE53DDDA4 for ; Wed, 25 Mar 2026 14:12:26 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.52 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774447948; cv=none; b=qgqrBXnPrwltIkyAGZ85JkmshEQkEXm3eGgB22io67yRKZyOQRWBvW0z/l/kA0/hNozvJGVpe90ixabadDI03b5AVPuhQZWKNJRPzPA+8ERQC8ZmD/Qqo8L7uYAckwSwWinChNNeMZbZLCUIb5elWOJBCLEqVEL1/b+nHIqoPog= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774447948; c=relaxed/simple; bh=W/uHQ0X8HmeXGc9GFCe511LzFr/SvuXDM6cHPVCg6Wo=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=D9M1xZ5picsVa7G50Dpo95CwkNk3wYRypATENXUumTfcXtlCRLJ7LRdMqVag23wtVL+0wyahLw+HAukGiyp0QOUmqFV99xJ+NmGJ7qIuaPZCHmgfwYTq/0nhS+5KURs9pI6mTNFDluavOjb6PYZ1SjQbukKeuEr7L6Tz0lFB2Gk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=suse.com; spf=pass smtp.mailfrom=suse.com; dkim=pass (2048-bit key) header.d=suse.com header.i=@suse.com header.b=AFlQlz/+; arc=none smtp.client-ip=209.85.128.52 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=suse.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=suse.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=suse.com header.i=@suse.com header.b="AFlQlz/+" Received: by mail-wm1-f52.google.com with SMTP id 5b1f17b1804b1-486fba7ce4cso24772315e9.3 for ; Wed, 25 Mar 2026 07:12:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1774447945; x=1775052745; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=2tErVjWTL8cXrlUcoAc8LF+5oLaUq7L/Q59xrBI+kJI=; b=AFlQlz/++o3t1XC03I3xgKP0AYwDHfF7ZDPriD4TVJIvSbA1kMDxD9cddDHvYgj+Bv tIZDc0LfZPmQkG8WqNszb6WXUBjJTe0od3b7c+Vnfp8K6QwvtQlstTehIQAPqfAbXALq QZGbQMGU3SD4mFuV4xpO34han9lS9BPj4Px1K1A6s+fV+Q78eNImhSS7rBERj4HD/c10 Ylwf8DYlyTN1DKSmO5Xv8AfhsG8LuiwQy67KFTIz9NOqF7Os7AhYhNx9Hbqa2odovFBp vYmwIRoNCrSkJcrjKrfefwiFo72UCipvs0pnTVuRdoLLnIdFp6wr6rHhOmrNGlUVr3nq 0fhg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1774447945; x=1775052745; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=2tErVjWTL8cXrlUcoAc8LF+5oLaUq7L/Q59xrBI+kJI=; b=iIgvEoUBBJFOsjSTDi0QIn/6QfIUTmZqgd/l3RUbUrQJdfLDfIoyXEZIFmFlpKiScT D6P+zKTeH/bcS4dTN6HBzFjWFaGTzcKWufgQEyjc2o+n41s5/pJdgZKz5A0PyH+703HK 2ckIqf6osQP6aOnsEdnCSl18Z1xQlt46UXJs0NLHhIbhPLlYrgZw4MJ7vP1eMlM4Q2ek n71lsiEL9i6DT/HqWTLDf9DmEQPXY8tfn7zMFV8IEAF8we3RfGO80MqvC13vZHDjZ4W/ oXDT2Mq+N4F4fgLmFOPPQZILjJWtZYBcPfANFEZqfl01KVcvIWy2hIaIPa2Abww525tO swIw== X-Forwarded-Encrypted: i=1; AJvYcCU4B7kd+IO2QIlykWLNiRfGdYPX1HfBJNs6jSRvPdmWxjlAI1KZ+cat/JFScJlzdnihvuW71zxS3hdtnho=@vger.kernel.org X-Gm-Message-State: AOJu0YyBLQxEfg43O2SPczApTqLPFPZrBW6qLgPynX+I+QiZD2G2n9Ng 6y5FEbo5/bb31fD+G7Xqjg6JzjttTeSXBbqWGufKfHxdD60SKNqaS/c/ZK/PBA8mBgA= X-Gm-Gg: ATEYQzydd+f1ticftBNPqNhPLagvApxF3gNGPSDMvW3wgcomUEm4v18P03grZDWXQYd 9Sea5IL7QIdSoAUzcvJTM5Ns2kpN/xxDyTg5SmRZDyQTC3fF0yqlAPgvwieewO4yL1Ikx+D1Gm8 8qpxe6CCoIE1wPZvl/l1S4YAJjJmgD3HOvc+ZvszMCN5z5Y+0FrNgwI+o/ZqcXMtcE5O42nfqi1 6oEt6vAvnj8JWRXbrqTvVLqJXieSCkCFICi1oi5Qup/KNTCm40AVxM6yovunp04Vl79St/R2aGV E7PeqaLLiYnrBeqTBbRTKsdzXNzRaryiqlsqWSWc7OwdmrKB+VzB5f3iKDG9MelYvrWcd7uvQHq xLCbUFAlDyAF9I896/PfvX/HEAFuGodE/FkL8siXPOfxuAY0+uPZGdOBfVm5+8ZRSFsbDV4D3cH vG09Co8Da+67CPeXgMS9SkOrH24Q== X-Received: by 2002:a05:600c:4685:b0:47d:8479:78d5 with SMTP id 5b1f17b1804b1-48715fc38e8mr58148955e9.7.1774447945056; Wed, 25 Mar 2026 07:12:25 -0700 (PDT) Received: from pathway.suse.cz ([176.114.240.130]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4871735e89esm20599555e9.22.2026.03.25.07.12.24 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 25 Mar 2026 07:12:24 -0700 (PDT) Date: Wed, 25 Mar 2026 15:12:22 +0100 From: Petr Mladek To: Song Liu Cc: Song Liu , linux-kernel@vger.kernel.org, tj@kernel.org, jiangshanlai@gmail.com, leitao@debian.org, kernel-team@meta.com, puranjay@kernel.org Subject: Re: [PATCH v2] workqueue: Fix false positive stall reports Message-ID: References: <20260322033045.3405807-1-song@kernel.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: On Wed 2026-03-25 14:19:27, Petr Mladek wrote: > On Tue 2026-03-24 11:22:11, Song Liu wrote: > > On Tue, Mar 24, 2026 at 3:01 AM Petr Mladek wrote: > > [...] > > > This explains why taking the lock is needed. > > > > > > > > > + Since > > > > > > + * __queue_work() is a much hotter path than the timer > > > > > > + * function, we handle false positive here by reading > > > > > > + * last_progress_ts again with pool->lock held. > > > > > > But this is confusing. It says that __queue_work() is a much hotter path > > > but it already takes pool->lock. The sentence makes a feeling that > > > the watchdog patch is less hot. Then it is weird why the watchdog > > > path ignores the lock by default. > > > > This comment primarily concerns the cost of making the read lockless. > > To do that, we need to add a memory barrier in __queue_work(), > > which will slow down the hot path. __queue_work() does take > > pool->lock, but in most cases, __queue_work() only takes the lock > > in local pool, which is faster than taking all the pool->locks from the > > watchdog timer. > > > > That said, I am open to other suggestions to get rid of this false > > positive. > > See below a patch which tries to improve the comment. > > Honestly, I am not sure if it is better than the original. It probably > better answers the questions which I had. But it might open another > questions. I feel that I have lost a detached view because I already > know the details. Also it is hard to mention everything and keep it > "short". > > Feel free to take it, ignore it, or squash it into the original > commit. Grrr, I have sent a wrong version of the patch. I forgot to amend last changes and refresh it. It is actually quite different. Anyway, this is what I wanted to send: >From e7ab783f9a1dc5c0e8bd3b1a4bf16dd8856a59e5 Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Wed, 25 Mar 2026 13:34:18 +0100 Subject: [PATCH v2] workqueue: Better describe stall check Try to be more explicit why the workqueue watchdog does not take pool->lock by default. Spin locks are full memory barriers which delay anything. Obviously, they would primary delay operations on the related worker pools. Explain why it is enough to prevent the false positive by re-checking the timestamp under the pool->lock. Finally, make it clear what would be the alternative solution in __queue_work() which is a hotter path. Signed-off-by: Petr Mladek --- kernel/workqueue.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/kernel/workqueue.c b/kernel/workqueue.c index ff97b705f25e..eda756556341 100644 --- a/kernel/workqueue.c +++ b/kernel/workqueue.c @@ -7702,13 +7702,14 @@ static void wq_watchdog_timer_fn(struct timer_list *unused) /* * Did we stall? * - * Do a lockless check first. On weakly ordered - * architectures, the lockless check can observe a - * reordering between worklist insert_work() and - * last_progress_ts update from __queue_work(). Since - * __queue_work() is a much hotter path than the timer - * function, we handle false positive here by reading - * last_progress_ts again with pool->lock held. + * Do a lockless check first to do not disturb the system. + * + * Prevent false positives by double checking the timestamp + * under pool->lock. The lock makes sure that the check reads + * an updated pool->last_progress_ts when this CPU saw + * an already updated pool->worklist above. It seems better + * than adding another barrier into __queue_work() which + * is a hotter path. */ if (time_after(now, ts + thresh)) { scoped_guard(raw_spinlock_irqsave, &pool->lock) { -- 2.53.0