From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-189.mta1.migadu.com (out-189.mta1.migadu.com [95.215.58.189]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9993B34B42F for ; Wed, 17 Dec 2025 13:37:07 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=95.215.58.189 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765978629; cv=none; b=XCcUEKpmXt9eSCt9LlJo9BAQlpWV5jkDzIoWqXNthKknz0olH4TJMOvQvVWhquIPPuK2+Na2ZuQOEDJZPRlAmhzlKEp10l05Z1nmdxz2y+SNF7J1KFdtGeXEwTMeTzkzIjs9pnYx+31R3fmCqR1CZRZ9ItkRuoZTxbrN3xiiF0A= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765978629; c=relaxed/simple; bh=CDhTTqUkcBCzxGdzRB+MjmWYMovNitZkwjURGU/TuOI=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=folbC3sulBxuGPtxw/rGqSupPRBfUu6jPy/mF+X9GAQKECD9ObPtXggx5kXjA3EVvaU5MR1QKNZV1hV0DE/nV0/JAWhzrkJqEptdDeEQXrhBi6XC9rUZtqTsOKTZKZOBi2Tme4Cp3kkuK5rHCun2XTdTKRVCWBnJr6vEGrdFZTo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=uV6nxDAx; arc=none smtp.client-ip=95.215.58.189 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="uV6nxDAx" Message-ID: <3773726f-f060-47a7-b99d-66ab0aa0d8b5@linux.dev> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1765978625; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=VSuJPZH6fly97gob14xn8CyZf7hCQ7fKXfmMIBk+eS8=; b=uV6nxDAxWRobhaRSUyDCvsAca3RGmynr2hrFjlTuSn5X2AQEe82Dq1Dq2W8rps/8uIeoCB 5g0jHjHDgfd/BIgE02edHwGRHedh3vxM4cT0nk3ZsDN8TORjbW0TiADvCskPwDkDCrre/b y8qRV0w8Ht5cZvKlfoIxlv9bPKxwcWs= Date: Wed, 17 Dec 2025 21:36:56 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Subject: Re: [PATCH v3 2/2] hung_task: Enable runtime reset of hung_task_detect_count Content-Language: en-US To: Petr Mladek Cc: Aaron Tomlin , sean@ashe.io, linux-kernel@vger.kernel.org, mhiramat@kernel.org, akpm@linux-foundation.org, gregkh@linuxfoundation.org References: <20251216030036.1822217-1-atomlin@atomlin.com> <20251216030036.1822217-3-atomlin@atomlin.com> <79e00527-0ada-4c43-915e-1f8f6589d051@linux.dev> X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Lance Yang In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Migadu-Flow: FLOW_OUT On 2025/12/17 21:09, Petr Mladek wrote: > On Wed 2025-12-17 15:31:25, Lance Yang wrote: >> On 2025/12/16 11:00, Aaron Tomlin wrote: >>> Introduce support for writing to /proc/sys/kernel/hung_task_detect_count. >>> >>> Writing any value to this file atomically resets the counter of detected >>> hung tasks to zero. This grants system administrators the ability to clear >>> the cumulative diagnostic history after resolving an incident, simplifying >>> monitoring without requiring a system restart. >> >>> --- a/Documentation/admin-guide/sysctl/kernel.rst >>> +++ b/Documentation/admin-guide/sysctl/kernel.rst >>> @@ -418,7 +418,7 @@ hung_task_detect_count >>> ====================== >>> Indicates the total number of tasks that have been detected as hung since >>> -the system boot. >>> +the system boot. The counter can be reset to zero when written to. >>> This file shows up if ``CONFIG_DETECT_HUNG_TASK`` is enabled. >>> diff --git a/kernel/hung_task.c b/kernel/hung_task.c >>> index 5902573200c0..01ce46a107b0 100644 >>> --- a/kernel/hung_task.c >>> +++ b/kernel/hung_task.c >>> @@ -375,6 +375,31 @@ static long hung_timeout_jiffies(unsigned long last_checked, >>> } >>> #ifdef CONFIG_SYSCTL >>> + >>> +/** >>> + * proc_dohung_task_detect_count - proc handler for hung_task_detect_count >>> + * @table: Pointer to the struct ctl_table definition for this proc entry >>> + * @write: Flag indicating the operation >>> + * @buffer: User space buffer for data transfer >>> + * @lenp: Pointer to the length of the data being transferred >>> + * @ppos: Pointer to the current file offset >>> + * >>> + * This handler is used for reading the current hung task detection count >>> + * and for resetting it to zero when a write operation is performed. >>> + * Returns 0 on success or a negative error code on failure. >>> + */ >>> +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); >>> + >>> + WRITE_ONCE(sysctl_hung_task_detect_count, 0); >> >> The reset uses WRITE_ONCE() but the increment in check_hung_task() >> is plain ++, it's just a stat cunter so fine though ;) > > I was about to wave this away as well. But I am afraid that > it is even more complicated. Good catch! > > The counter is used to decide how many hung tasks were found in > by a single check, see: > > static void check_hung_uninterruptible_tasks(unsigned long timeout) > { > [...] > unsigned long prev_detect_count = sysctl_hung_task_detect_count; > [...] > for_each_process_thread(g, t) { > [...] > check_hung_task(t, timeout, prev_detect_count); > } > [...] > if (!(sysctl_hung_task_detect_count - prev_detect_count)) > return; > > if (need_warning || hung_task_call_panic) { > si_mask |= SYS_INFO_LOCKS; > > if (sysctl_hung_task_all_cpu_backtrace) > si_mask |= SYS_INFO_ALL_BT; > } > > sys_info(si_mask); > > if (hung_task_call_panic) > panic("hung_task: blocked tasks"); > } > > Any race might cause false positives and even panic() !!! > And the race window is rather big (checking all processes). Right, I completely missed the dependency on prev_detect_count. Messing with that counter while the loop is running is really dangerous ... > > 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(). > > On one hand, it might work. The code is called from > a task context... > > But adding a lock into a lockup-detector code triggers > some warning bells in my head. > > Honestly, I am not sure if it is worth it. I understand > the motivation for the reset. But IMHO, even more important > is to make sure that the watchdog works as expected. Given that adding a lock to the detector path is undesirable, the risk clearly outweighs the benefit :) Thanks for saving me from this pitfall, Petr!