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

[Lance Yang]

On Wed, Aug 26, 2026 at 01:17:47PM +0200, Petr Mladek wrote:
[...]
>/*
> * The number of hung tasks which still can be reported.
> * The budget gets restored to the original limit when
> * the previous stall is resolved.
> */
>static int hung_task_warnings_budget = 10;

Yeah, hung_task_warnings_budget is better. Comments too.

></proposal>
>
>> +static atomic_t reset_hung_task_warnings = ATOMIC_INIT(0);
>> +
>>  static int __read_mostly did_panic;
>>  static bool hung_task_call_panic;
>>  
>> @@ -245,11 +248,11 @@ static void hung_task_info(struct task_struct *t, unsigned long timeout,
>>  	/*
>>  	 * The given task did not get scheduled for more than
>>  	 * CONFIG_DEFAULT_HUNG_TASK_TIMEOUT. Therefore, complain
>> -	 * accordingly
>> +	 * accordingly with full details if the budget is not exhausted.
>>  	 */
>> -	if (sysctl_hung_task_warnings || hung_task_call_panic) {
>> -		if (sysctl_hung_task_warnings > 0)
>> -			sysctl_hung_task_warnings--;
>> +	if (hung_task_warnings_printed || hung_task_call_panic) {
>> +		if (hung_task_warnings_printed > 0)
>> +			hung_task_warnings_printed--;
>>  		pr_err("INFO: task %s:%d blocked%s for more than %ld seconds.\n",
>>  		       t->comm, t->pid, t->in_iowait ? " in I/O wait" : "",
>>  		       (jiffies - t->last_switch_time) / HZ);
>> @@ -264,7 +267,7 @@ static void hung_task_info(struct task_struct *t, unsigned long timeout,
>>  		sched_show_task(t);
>>  		debug_show_blocker(t, timeout);
>>  
>> -		if (!sysctl_hung_task_warnings)
>> +		if (!hung_task_warnings_printed)
>>  			pr_info("Future hung task reports are suppressed, see sysctl kernel.hung_task_warnings\n");
>>  	}
>>  
>> @@ -304,7 +307,7 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)
>>  	unsigned long last_break = jiffies;
>>  	struct task_struct *g, *t;
>>  	unsigned long this_round_count;
>> -	int need_warning = sysctl_hung_task_warnings;
>> +	int need_warning;
>>  	unsigned long si_mask = hung_task_si_mask;
>>  
>>  	/*
>> @@ -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 :)

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.

The handler publishes the reset only after proc_dointvec_minmax()
succeeds:

static int proc_dohung_task_warnings(const struct ctl_table *table, int write,
				     void *buffer,
				     size_t *lenp, loff_t *ppos)
{
	int ret;

	ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
	if (!ret && write)
		atomic_set_release(&reset_hung_task_warnings, 1);

	return ret;
}

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:

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);
			if (err)
				break;
			if (conv(&neg, &lval, i, 1, table)) {
				err = -EINVAL;
				break;
			}
...
	*lenp -= left;
out:
	*ppos += *lenp;
	return err;
}

The min/max callback passes true for k_ptr_range_check:

static int do_proc_int_conv_minmax(bool *negp, unsigned long *u_ptr, int *k_ptr,
				   int dir, const struct ctl_table *tbl)
{
	return proc_int_conv(negp, u_ptr, k_ptr, dir, tbl, true,
			     sysctl_user_to_kern_int_conv,
			     sysctl_kern_to_user_int_conv);
}

proc_int_conv() writes i here:

int proc_int_conv(bool *negp, ulong *u_ptr, int *k_ptr, int dir,
		  const struct ctl_table *tbl, bool k_ptr_range_check,
		  int (*user_to_kern)(const bool *negp, const ulong *u_ptr, int *k_ptr),
		  int (*kern_to_user)(bool *negp, ulong *u_ptr, const int *k_ptr))
{
...
	if (k_ptr_range_check) {
		int tmp_k, ret;

		if (!tbl)
			return -EINVAL;
		ret = user_to_kern(negp, u_ptr, &tmp_k);
		if (ret)
			return ret;
		if ((tbl->extra1 && *(int *)tbl->extra1 > tmp_k) ||
		    (tbl->extra2 && *(int *)tbl->extra2 < tmp_k))
			return -EINVAL;
		WRITE_ONCE(*k_ptr, tmp_k);
...
	return 0;
}

So table->data already gets WRITE_ONCE() before atomic_set_release(). No
need for proxy_table here, AFAICS. I'd keep the rename and _acquire
change :)

WDYT?

Cheers, Lance

>
>   + proc_dointvec_minmax()
>     + do_proc_vec()
>       + proc_get_long()
>	 + strtoul_lenient()
>
>which does a plain assigment:
>
>static int strtoul_lenient(const char *cp, char **endp, unsigned int base,
>			   unsigned long *res)
>{
>[...]
>	*res = (unsigned long)result;
>[...]
>}
>
>It can be solved by using temporary variable in proc_dointvec_minmax().
>We have a custom proc_dohung_task_warnings() handler anyway.
>See below.
>
>> +	need_warning = hung_task_warnings_printed;
>> +
>>  	this_round_count = 0;
>>  	rcu_read_lock();
>>  	for_each_process_thread(g, t) {
>> @@ -340,8 +348,11 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)
>>   unlock:
>>  	rcu_read_unlock();
>>  
>> -	if (!this_round_count)
>> +	if (!this_round_count) {
>> +		hung_task_warnings_printed =
>> +			READ_ONCE(sysctl_hung_task_warnings);
>>  		return;
>> +	}
>>  
>>  	if (need_warning || hung_task_call_panic) {
>>  		si_mask |= SYS_INFO_LOCKS;
>> @@ -425,6 +436,19 @@ static int proc_dohung_task_timeout_secs(const struct ctl_table *table, int writ
>>  	return ret;
>>  }
>>  
>> +static int proc_dohung_task_warnings(const struct ctl_table *table, int write,
>> +				     void *buffer,
>> +				     size_t *lenp, loff_t *ppos)
>> +{
>> +	int ret;
>> +
>> +	ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
>> +	if (!ret && write)
>> +		atomic_set_release(&reset_hung_task_warnings, 1);
>> +
>> +	return ret;
>> +}
>
>We should use WRITE_ONCE() when updating proc_dohung_task_warnings.
>So, we need similar trick with proxy_table like in
>proc_dohung_task_detect_count. Something like, on top of this patch:
>
>--- a/kernel/hung_task.c
>+++ b/kernel/hung_task.c
>@@ -444,13 +444,26 @@ static int proc_dohung_task_warnings(const struct ctl_table *table, int write,
> 				     void *buffer,
> 				     size_t *lenp, loff_t *ppos)
> {
>+	struct ctl_table proxy_table;
>+	int warnings;
> 	int ret;
> 
>-	ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
>-	if (!ret && write)
>-		atomic_set_release(&reset_hung_task_warnings, 1);
>+	proxy_table = *table;
>+	proxy_table.data = &warnings;
> 
>-	return ret;
>+	if (SYSCTL_KERN_TO_USER(write))
>+		warnings = READ_ONCE(sysctl_hung_task_warnings);
>+
>+	ret = proc_dointvec_minmax(&proxy_table, write, buffer, lenp, ppos);
>+	if (ret < 0)
>+		return ret;
>+
>+	if (SYSCTL_USER_TO_KERN(write)) {
>+		WRITE_ONCE(sysctl_hung_task_warnings, warnings);
>+		atomic_set_release(&reset_hung_task_warnings, 1);
>+	}
>+
>+	return 0;
> }
> 
> /*
>
>Otherwise, it looks good to me.
>
>Best Regards,
>Petr
>