From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f44.google.com (mail-wm1-f44.google.com [209.85.128.44]) (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 5307431986D for ; Fri, 19 Dec 2025 12:03:30 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.44 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1766145812; cv=none; b=qp8i+RyAR5vFHhq84NwBQ6s2picYOUyWyJU3f4Uv9hsoXYDwpQJYdHRnPo2fF2zJTo8h50Vj8bpfKsmiO69fHd75fVbAS9V3d3jxGjlRSsdWU7dsyV7v7mqLnU27X8FlF+UkgfM17O0ZTtq8OBnk/gu9ILtzVBu90yPZ5J79vas= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1766145812; c=relaxed/simple; bh=MpA01uoFCPDoBBaVs9GnNcShozK71jnD9L28cvtMbNo=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=XLsPb9FnZRCLExldCJ3mvNm+CdVOmGYReHPpGk8mPYLsN31Snd64r5lULAKtjiGu1hbiOBk2GmiTI77bxqBi8aE5nw4achyRVVg5AT4EV9NdiKNy7uUmvtQLWMagMp8docy0oBe50u+jCNyWPS0PZottjIHytt3sfr8fT8thQ9U= 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=c1jShzXx; arc=none smtp.client-ip=209.85.128.44 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="c1jShzXx" Received: by mail-wm1-f44.google.com with SMTP id 5b1f17b1804b1-477b198f4bcso11959815e9.3 for ; Fri, 19 Dec 2025 04:03:30 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1766145808; x=1766750608; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=hgLZo/iiN4t13noQ/iudlNxEW9ByuZiOVR1G4B3jOmc=; b=c1jShzXxDPmBOxFSgEw49Jcf5BV35RBBfLzZkXU/xEBZl9C9IKsc6RxMTJ/YdNhIOO h7D3Rb4KJm8SD1HZBkLVqu5NXHxJ4z8JO3jqrPf/ESSYXUbf8e1NtHxUoiq7/Buxdf/C 9SdZP+ra6Diqol4jCKNDmXkYFmu2j8ZhsOo0Vc2KM54ggNQYWHCA1BrrH9FEi6gJF0BY Ckkn6AZJlQKWxWi+2G49yPpFa0oqRbP0idoaNYJn2PN/xvyuYFF/BCFOpASKhxT7hwLm YjzIbe021VnMewCqme//bh+Xg2sjnr+YdIyp0cmTxdyhD6zoLaCCSOuPn1ROIfFPD48M ywig== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1766145808; x=1766750608; h=in-reply-to: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=hgLZo/iiN4t13noQ/iudlNxEW9ByuZiOVR1G4B3jOmc=; b=I4iN3TLUWVrnP3WcC0DeF1Uzu49U2Yg8OPBXmoesV7eGx5B19e4Mye1X1qNW61LgyC rrbbl2yVGAabqcoKnqVPIMwwTGt3LP8hXv7kQ8LctpHnaHyK382zvpM8v1fMmXseY++i bk/8Lk/zemojlbQe3Osp++kP8wNeYLnesjCPF2y8d0UiGPVtS6aYnrL7QDrvAmMgeQxa Wr5mwSWOzo1WUGgS9zNwfci+7E8OnkwqQMsBkEVzAuJPX4zKe1jDM7xe7hLByQueebae NN3SJhBzxojnlsS52c29W+YC7GGL9U6ezP+pKvP9XyoV+8oWZ6G9X6YW5NFRN3SS0DY/ r5SA== X-Forwarded-Encrypted: i=1; AJvYcCX/dlC+PtHCSDrlZhbKL82vCBvYlh/ol4F7T0IUhKTkg0S2RQq+MPUjBA0AykgfE7RI4WXQwJcV0Jee49c=@vger.kernel.org X-Gm-Message-State: AOJu0YxVaCkoQUk/EvTtqJ06tGYQPTGWMz+nguSW10WOcbD54+NNkKo2 YSpIEkCsQNU/slqiNJtp2eaTKmOE9hDUNn+G5jEPu0fz0CeyysoYjDg72zgoh8m3+mg= X-Gm-Gg: AY/fxX5ANRr4OKv2f4xPthD/Iu9Bmei2aSFCVsKUttw3JiCOxsiQHJfjJ0XRo4d0y95 TMdvGmcFXZMwUVeQ7VPG7A2ov5It6/U+mslczC5YN8dVRt2xY5vHwkrIes6skvFHfhapGRq6R4p 643TeZ5Ll5U1gIBEqtYPQlDKNvO8GCbDEcbaFi0rN9a9bBbtzv67pRZ1C2le04Udk7IIN2frEfd 94135XiNRRJ6fSuMxdlMrbfN7be8PLTznZ5bYTIYnadQbuIvvlkcNlC7wJSydYbI6ytU6b4LI9Z jgOcbFREWiq2Uo4ZWTdgJg+NLAKqzedMcXO+VAsEJ88i1ttgwoFoT3VUiYuHVf+qEfe9TvUFNn3 4YM9xFqhlPB/fwwl02/9ARi+sb6El73Oe8P2ZuiOgnoyGaYuKzcSw4+c3Vy5pqNQyR5E6puQ0J2 qKr+p4BDIrYLv23w== X-Google-Smtp-Source: AGHT+IHrtmeHlkYVSqWyO4W/sTG/Udt8xOWnDMFxcOJFrZsIC+U4SSBaCXzEziNtJnDlujVIwP5y5g== X-Received: by 2002:a05:600c:46c4:b0:471:14af:c715 with SMTP id 5b1f17b1804b1-47d19533472mr26262835e9.3.1766145808444; Fri, 19 Dec 2025 04:03:28 -0800 (PST) Received: from pathway.suse.cz ([176.114.240.130]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-47be3aea77bsm32650815e9.17.2025.12.19.04.03.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 19 Dec 2025 04:03:27 -0800 (PST) Date: Fri, 19 Dec 2025 13:03:25 +0100 From: Petr Mladek To: Aaron Tomlin Cc: Lance Yang , sean@ashe.io, linux-kernel@vger.kernel.org, mhiramat@kernel.org, akpm@linux-foundation.org, gregkh@linuxfoundation.org Subject: Re: [PATCH v3 2/2] hung_task: Enable runtime reset of hung_task_detect_count Message-ID: References: <20251216030036.1822217-1-atomlin@atomlin.com> <20251216030036.1822217-3-atomlin@atomlin.com> <79e00527-0ada-4c43-915e-1f8f6589d051@linux.dev> 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=us-ascii Content-Disposition: inline In-Reply-To: On Thu 2025-12-18 22:09:57, Aaron Tomlin wrote: > On Wed, Dec 17, 2025 at 02:09:14PM +0100, Petr Mladek wrote: > > The counter is used to decide how many hung tasks were found in > > by a single check, see: > > > > Any race might cause false positives and even panic() !!! > > And the race window is rather big (checking all processes). > > > > This race can't be prevented by an atomic read/write. > > IMHO, the only solution would be to add some locking, > > e.g. mutex and take it around the entire > > check_hung_uninterruptible_tasks(). [...] > Thank you for the feedback regarding the potential for false positives and > erroneous panics. You are absolutely correct that the race window is > substantial, given that it spans the entire duration of a full > process-thread iteration. > > I believe we can resolve this race condition and prevent the integer > underflow without the overhead or risk of a mutex by leveraging hardware > atomicity and a slight adjustment to the delta logic. > > Specifically, by converting sysctl_hung_task_detect_count to an > atomic_long_t, we ensure that the increment and reset operations are > indivisible at the CPU level. To handle the "reset mid-scan" issue you > highlighted, we can modify the delta calculation to be "reset-aware": > diff --git a/kernel/hung_task.c b/kernel/hung_task.c > index 01ce46a107b0..74725dc1efd0 100644 > --- a/kernel/hung_task.c > +++ b/kernel/hung_task.c > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -36,7 +37,7 @@ static int __read_mostly sysctl_hung_task_check_count = PID_MAX_LIMIT; > /* > * Total number of tasks detected as hung since boot: > */ > -static unsigned long __read_mostly sysctl_hung_task_detect_count; > +static atomic_long_t atomic_long_t sysctl_hung_task_detect_count = ATOMIC_LONG_INIT(0); > > /* > * Limit number of tasks checked in a batch. > @@ -253,6 +254,7 @@ static void check_hung_task(struct task_struct *t, unsigned long timeout, > unsigned long prev_detect_count) > { > unsigned long total_hung_task; > + unsigned long current_detect; > > if (!task_is_hung(t, timeout)) > return; > @@ -261,9 +263,14 @@ static void check_hung_task(struct task_struct *t, unsigned long timeout, > * This counter tracks the total number of tasks detected as hung > * since boot. > */ > - sysctl_hung_task_detect_count++; > + atomic_long_inc(&sysctl_hung_task_detect_count); > + > + current_detect = atomic_long_read(&sysctl_hung_task_detect_count); The two atomic_long_inc() and atomic_long_read() can be replaced by atomic_long_inc_return(). > + if (current_detect >= prev_detect_count) > + total_hung_task = current_detect - prev_detect_count; > + else > + total_hung_task = current_detect; Interesting trick. The result may not be precise when the counter is reset in the middle of the check. But it would prevent calling panic() because of a wrong computation. It should be acceptable. The panic() is needed when the lockup looks permanent. And if the lockup is permanent then the right total_hung_task number will be computed by the next check. Please, add a comment into the code explaining why it is done this way and why it is OK. > - total_hung_task = sysctl_hung_task_detect_count - prev_detect_count; > trace_sched_process_hang(t); > > if (sysctl_hung_task_panic && total_hung_task >= sysctl_hung_task_panic) { > @@ -322,7 +329,7 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout) > int max_count = sysctl_hung_task_check_count; > unsigned long last_break = jiffies; > struct task_struct *g, *t; > - unsigned long prev_detect_count = sysctl_hung_task_detect_count; > + unsigned long prev_detect_count = atomic_long_read(&sysctl_hung_task_detect_count); I think that it is not super important in this case. But this is a kind of locking, so I would use some proper annotation/barriers. IMHO, we should use here: atomic_long_read_acquire() And the counter part would be atomic_long_inc_return_release() > int need_warning = sysctl_hung_task_warnings; > unsigned long si_mask = hung_task_si_mask; > > @@ -350,7 +357,7 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout) > unlock: > rcu_read_unlock(); > > - if (!(sysctl_hung_task_detect_count - prev_detect_count)) > + if (!(atomic_long_read(&sysctl_hung_task_detect_count) - prev_detect_count)) I guess that we should do the same check here. Otherwise, we might print the sys_info() just because the counter has been reset in the meantime. And from the locking/barriers POV, we should use atomic_long_read_release() here. > return; > > if (need_warning || hung_task_call_panic) { > @@ -391,10 +398,15 @@ static long hung_timeout_jiffies(unsigned long last_checked, > static int proc_dohung_task_detect_count(const struct ctl_table *table, int write, > void *buffer, size_t *lenp, loff_t *ppos) > { > - if (!write) > - return proc_doulongvec_minmax(table, write, buffer, lenp, ppos); > + if (!write) { > + unsigned long count = atomic_long_read(&sysctl_hung_task_detect_count); > + struct ctl_table proxy_table = *table; > + > + proxy_table.data = &count; > + return proc_doulongvec_minmax(&proxy_table, write, buffer, lenp, ppos); If we are going to use the proxy table then we are already half way to add the check of written value. Please, return -EINVAL when user writes a non-zero value as I suggested at https://lore.kernel.org/r/aUKmh0Gs8YH_iLbC@pathway.suse.cz + } > > - WRITE_ONCE(sysctl_hung_task_detect_count, 0); > + atomic_long_set(&sysctl_hung_task_detect_count, 0); > *ppos += *lenp; > > return 0; > > Please note that this updated approach has not been tested yet, but I > believe it addresses the core concerns while maintaining the architectural > integrity of the detector. This approach looks acceptable to me. I though also about using sequence counters, see Documentation/locking/seqlock.rst. It would allow to restart the scan when the counter has been reset in the meantime. But I think that it is not worth it. The counter is important only to decide whether to call panic() or not. And it makes sense to call panic() only when the stall is persistent. So, it is perfectly fine to wait for the next scan. Best Regards, Petr