Re: [PATCH v9 1/2] hung_task: Reset warning budget when problem gets resolved

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

On 2026/8/28 17:05, Petr Mladek wrote:
> On Thu 2026-08-27 23:30:01, Lance Yang wrote:
>> On Wed, Aug 26, 2026 at 01:17:47PM +0200, Petr Mladek wrote:
>>>> @@ -314,6 +317,11 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)
>>>>   	if (test_taint(TAINT_DIE) || did_panic)
>>>>   		return;
>>>>   
>>>> +	if (atomic_xchg(&reset_hung_task_warnings, 0))
>>>
>>> I would use here atomic_xchg_acquire(). It serializes the ordering
>>> of reset_hung_task_warnings vs sysctl_hung_task_warnings.
>>> It would make it symetric with the barrier in the sysctl handler.
>>
>> Yep, _acquire is enough here. Plain atomic_xchg() is already fully
>> ordered, though, so this looks like making the intent clearer rather
>> than fixing the ordering :)
> 
> Yes, my intention was to make the ordering more clear and symmetric.

Yep, that makes sense. atomic_xchg_acquire() is a better fit here :)

> The original code worked because the barrier was even stronger.
> 
>> The old-value return already makes plain atomic_xchg() fully ordered :)
>>
>> ORDERING  (see memory-barriers.txt)
>> --------
>>
>> The rule of thumb:
>> ...
>> - RMW operations that have a return value are fully ordered;
>> ...
>> Except of course when a successful operation has an explicit ordering
>> like:
>>
>> {}_relaxed: unordered
>> {}_acquire: the R of the RMW (or atomic_read) is an ACQUIRE
>> {}_release: the W of the RMW (or atomic_set)  is a  RELEASE
>>
>>>
>>>> +		hung_task_warnings_printed =
>>>> +			READ_ONCE(sysctl_hung_task_warnings);
>>>
>>> This would work only when "sysctl_hung_task_warnings"
>>> is updated using WRITE_ONCE(). But it seems that this
>>> is not the case. My understading is that it is updated by:
>>
>> Wait, I think proc_dointvec_minmax() already handles this.
> 
> You are right.
> 
>> For proc_dointvec_minmax(), the converter is:
>>
>> int proc_dointvec_minmax(const struct ctl_table *table, int dir,
>> 		  void *buffer, size_t *lenp, loff_t *ppos)
>> {
>> 	return do_proc_dointvec(table, dir, buffer, lenp, ppos,
>> 				do_proc_int_conv_minmax);
>> }
>>
>> Here, i is table->data, while lval is local:
> 
> I have missed this.

No worries at all. This was easy to miss in that call chain ...

> 
>> static int do_proc_dointvec(const struct ctl_table *table, int dir,
>> 		  void *buffer, size_t *lenp, loff_t *ppos,
>> 		  int (*conv)(bool *negp, unsigned long *u_ptr, int *k_ptr,
>> 			      int dir, const struct ctl_table *table))
>> {
>> ...
>> 	i = (int *) table->data;
>> 	vleft = table->maxlen / sizeof(*i);
>> ...
>> 	for (; left && vleft--; i++, first=0) {
>> 		unsigned long lval;
>> 		bool neg;
>>
>> 		if (SYSCTL_USER_TO_KERN(dir)) {
>> 			proc_skip_spaces(&p, &left);
>>
>> 			if (!left)
>> 				break;
>> 			err = proc_get_long(&p, &left, &lval, &neg,
>> 					     proc_wspace_sep,
>> 					     sizeof(proc_wspace_sep), NULL);
> 
> I have missed that proc_get_long() assigns the value to the local
> variable @lval.
> 
>> 			if (err)
>> 				break;
>> 			if (conv(&neg, &lval, i, 1, table)) {
>> 				err = -EINVAL;
>> 				break;
> 
> The real asigment to table->data is done here. And I agree that it
> goes down to proc_int_conv() which does WRITE_ONCE().
> 
> So, we are on the safe side and do _not_ need the proxy table.

Agreed.

> 
> Now, I am not sure whether we need v10. It might be worth it.
> AFAIK, Andrew has not taken this patchset yet...

I think we do. Definitely :) And if Andrew hasn't picked it up
yet, even better. There's still time to fold the changes in :)

> 
> I am sorry for complications.

No need to apologize at all, Petr. I really appreciate you taking
another careful look!

Cheers, Lance