Re: [PATCH v3 2/2] hung_task: Enable runtime reset of hung_task_detect_count

[Lance Yang <lance.yang@linux.dev>]

On 2025/12/18 17:08, Joel Granados wrote:
> On Wed, Dec 17, 2025 at 09:21:01PM +0800, Lance Yang wrote:
>>
>>
>> On 2025/12/17 20:48, Petr Mladek wrote:
>>> Adding Joel into Cc. He is improving the sysctl API...
>>>
>>> On Mon 2025-12-15 22:00:36, 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.
> I see that this might be refactored due to dependency that was not
> considered. I'll write my comments, but will also drop it from my radar
> for now.
> 
>>>>
>>>> --- 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.
> Would it make more sense to write it like this:
> 
>    Indicates the total number of tasks that have been detected as hung
>    since the system boot or since the counter was reset. Counter is
>    zeroed 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);
>>>
>>> There have been some changes in the sysctl API recently, see
>>> https://lore.kernel.org/lkml/20251016-jag-sysctl_conv-v2-0-a2f16529acc4@kernel.org/
>>>
>>> They are backward compatible, so the above code works. But it would be
>>> nice to make it up-to-date, namely:
>>>
>>>     + Replace "write" with "dir"
>>>     + Use SYSCTL_USER_TO_KERN(dir) instead of (!write)
>>>
>>>
>>>> +	WRITE_ONCE(sysctl_hung_task_detect_count, 0);
>>>
>>> I might be too conservative. But it looks weird to allow clearing the
>>> value by any write. It would be better to return -EINVAL for non-zero
>>> values. This would require using a copy of struct ctl_table and read
>>> the value into a temporary variable.
>>
>> That's okay, I think. See vmstat_refresh() for a similar pattern - it
> You are correct but wouldn't it make more sense to zero it out only when
> the write value passed by the user is zero. And return -EINVAL for any
> other value?

Fair point, let's return -EINVAL when user writes a non-zero value :)