From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ej1-f52.google.com (mail-ej1-f52.google.com [209.85.218.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 450693DEAF9 for ; Tue, 24 Mar 2026 10:01:35 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.218.52 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774346496; cv=none; b=sKpma66n+IoxJeaZcuI4aK3j67JwFVCyFFdYeNY7jioVYYRgsm5UPC0JVVbhvIAAkeXvjfj5NlPcDeoZrwD9ESv6pTDzsJpE6div0TpGbKB5kk2NIfAv+zwts1G2wj4iuljna+HZmUtTYkzCqEMamDtowz6zrzskN2cdtVr6OlY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774346496; c=relaxed/simple; bh=VOezTQ/tj19Oshfw0eJI6Ps7r+bIi/9JOgTJRhyTvPk=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Uxw7OyxPYsitORf6ZSn8BD5UDjVgQLxhoyzecVOhJ8wA8uumuzC+n6t/jQx4d00+eO6w4ZexLLJnrNvRrP0Q8TW8ejDHCiX9QoeUZ3gwvGRU+ft6RZRVqxcTfnlOJLjpTfOAQGbGk3Ka9htdn6CcU2NglM0xSBYlkkpZYUxrJyQ= 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=gmgMG6HQ; arc=none smtp.client-ip=209.85.218.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="gmgMG6HQ" Received: by mail-ej1-f52.google.com with SMTP id a640c23a62f3a-b9793fa5371so690936266b.2 for ; Tue, 24 Mar 2026 03:01:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1774346493; x=1774951293; 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=bipwBQB0tnQhi/MBEyh1B99IFPK8UnoS/ILG3D9S25o=; b=gmgMG6HQSHPdyFCSTDS9rhRvmItNQWAtaqnw9x/POXIsa36nauyuQmWbq9/Wun0P10 +UP6pkYeZyQhYpfGhqrkCFsnEi6uExV2del9+Ed/xLKcjNvPfw0vMjdAlgi3SsEoixx6 S2cBPvC5OEOB9GYHu5fy1dn6XLDRr3bIEgp2nQErlUgn0N0czFHQ5ce5Q+J0goaPbOzb Psr6FtOIzWkcQWV6Fee9x45ot+N+QG8Hbx1OekAnSonoM+bw4LbolIXQSRM128ADtrKE d6TkPdkIVkf2senuPdq0T6ZKqFFDv6o6CKJq1lSaEJVF3EqzS031rvseiH9z1K23Di5m +Lew== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1774346493; x=1774951293; 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=bipwBQB0tnQhi/MBEyh1B99IFPK8UnoS/ILG3D9S25o=; b=D6Zn8e6th5n3B5KQQDpLUjMJn0sRrFZE1ynb29rD78FTS4fabtlNQBaoazQzFkw3mq IdQQwSR4q3iAE6mfBhOZpn/4sfL8AS0JRWLyv1mwd7zP9FXnjL/0gQNBG/qkdX6g8/zh eeOUDihc9NU8U1+pFl6Um7g2It6Ng7Ur5H/eBiDOyG80b1EnWywK+yAAYeJtWfJnvkJA yjpQjWRZhzQx3NHcwv8Xf/kVZDcbwaT0zjS2BSUbpn3N8Uuu0kLrUrv2LTYE6jVLo8Vf dVeAMSSuxVVeXI19h6EIDkGxmdJ9Uj2ApzDMRMPBLLxvsZtmx9Ct2CW3JfiRdSEyk0O/ Jnog== X-Gm-Message-State: AOJu0YxH86/jNveP3VUeGA5jgXfZMHMLgiLre+vGoYrd1eJYnsg77U4y MNz0XOB55IH9ERJwy3NWzSxvrTb+w0Q/HBtfNhdlQrrq04FU1sGgo2g4I75d8rj6TzA= X-Gm-Gg: ATEYQzxcjme9Z5M9nDE0aHoUuwqEubhnpsVr/UK6E3QTYwlYJ6QTn53jfU8hBi68xGd Y2NjQLr237mqqbmAus9x16i8iF0l5mM7NeVr+6QKWiKQr/91OZj40JS/UUnW/9X9X+TTDP2E03G 7nnISe6EFYydVq2ZrVWyx7pYyIgz44kqyx/9pIWtM36Qo1O+5Wy2hPP3qNuC2wp3CemHgWehNra of0Fjk/DS1VU+exJ98QD/OKhE9hdTwLkzPYf3fqSr15glIE5ScctTAti1RCXfORF1Xb2zuRxYtk G6jFLQfIgxN1M5nXj1/STTiadUW6J3tjhqOkvhcndwzR7JHfOTvMBxb2L60s+rZA68cMvjTfa/j J6KpI1lUoYT7eQVTveVGs1bFWk1Jl311SK+zzc2O/VtBUU+ViWmtqhA2UDkuh4KiiRa+rxP3KPZ ggmUC19Yuq1sMOdH+HOBlsF+G1M3K2HNfZPrBe X-Received: by 2002:a17:906:f593:b0:b98:458e:2b99 with SMTP id a640c23a62f3a-b98458e42a5mr948810666b.38.1774346492980; Tue, 24 Mar 2026 03:01:32 -0700 (PDT) Received: from pathway.suse.cz ([176.114.240.130]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-43b644ae37dsm37616529f8f.2.2026.03.24.03.01.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 24 Mar 2026 03:01:32 -0700 (PDT) Date: Tue, 24 Mar 2026 11:01:30 +0100 From: Petr Mladek To: Song Liu Cc: 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 Mon 2026-03-23 11:31:14, Song Liu wrote: > On Mon, Mar 23, 2026 at 7:09 AM Petr Mladek wrote: > > > > On Sat 2026-03-21 20:30:45, Song Liu wrote: > > > On weakly ordered architectures (e.g., arm64), the lockless check in > > > wq_watchdog_timer_fn() can observe a reordering between the worklist > > > insertion and the last_progress_ts update. Specifically, the watchdog > > > can see a non-empty worklist (from a list_add) while reading a stale > > > last_progress_ts value, causing a false positive stall report. > > > > > > This was confirmed by reading pool->last_progress_ts again after holding > > > pool->lock in wq_watchdog_timer_fn(): > > > > > > workqueue watchdog: pool 7 false positive detected! > > > lockless_ts=4784580465 locked_ts=4785033728 > > > diff=453263ms worklist_empty=0 > > > > > > To avoid slowing down the hot path (queue_work, etc.), recheck > > > last_progress_ts with pool->lock held. This will eliminate the false > > > positive with minimal overhead. > > > > > > Remove two extra empty lines in wq_watchdog_timer_fn() as we are on it. > > > > > > --- a/kernel/workqueue.c > > > +++ b/kernel/workqueue.c > > > @@ -7699,8 +7699,28 @@ static void wq_watchdog_timer_fn(struct timer_list *unused) > > > else > > > ts = touched; > > > > > > - /* did we stall? */ > > > + /* > > > + * 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(). 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. Instead, it would make sense to explain why taking the lock only around "pool->last_progress_ts" read is safe. You know, the code makes my head spin because it violates the usual locking rules. The lock should serialize 2 or more writes/reads. It works only when all the reads/writes are done under the lock. But it is not the case here. IMHO, the new code works because spin_lock() is a full memory barrier. It makes sure that pool->worklist is read before worker->pool->last_progress_ts Plus, the worker->pool->last_progress_ts read has to wait for any parallel modifications of both variables. As a result, pool->worklist value might be wrong (set or empty) but the timestamp is always more recent. IMHO, it prevents false positives (reporting a stall because of outdated timestamp). While it does not prevent false negatives (missing a stall). But it is not a problem because the stall will get caught in the next watchdog check round. > > > + */ > > > if (time_after(now, ts + thresh)) { > > > + scoped_guard(raw_spinlock_irqsave, &pool->lock) { > > > + pool_ts = pool->last_progress_ts; > > > + if (time_after(pool_ts, touched)) > > > + ts = pool_ts; > > > + else > > > + ts = touched; > > > + } > > > + if (!time_after(now, ts + thresh)) > > > + continue; > > > > The new code is pretty hairy. It might make sense to take the lock > > around the original check and keep it as is. > > > > IMHO, if a contention on a pool->lock has ever been a problem than maybe > > the non-trivial workqueue API was not a good choice for the affected > > use-case. Or am I wrong? > > Given the false positive is really rare (a handful of them per day among a > fleet of thousands of hosts), I think taking hundreds of pool->lock per 30 > second seems not necessary. This optimization might make sense. And it most likely works. My primary problem is that the code violates standard locking rules. And the comment does not explain why it works. I am sorry if I am too meticulous. I just know that it is not trivial to make lockless algorithms right. And the new code is still kind of lockless. Best Regards, Petr