linux-kernel - Re: [PATCH v2] hung_task:add detecting task in D state milliseconds timeout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sun, 5 Jul 2020 18:16:33 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     yang che <chey84736@...il.com>
Cc:     mcgrof@...nel.org, keescook@...omium.org, yzaikin@...gle.com,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v2] hung_task:add detecting task in D state milliseconds
 timeout

On Sun, Jul 05, 2020 at 08:48:52PM +0800, yang che wrote:
> @@ -16,8 +16,9 @@ extern unsigned int sysctl_hung_task_all_cpu_backtrace;
>  
>  extern int	     sysctl_hung_task_check_count;
>  extern unsigned int  sysctl_hung_task_panic;
> +extern unsigned long  sysctl_hung_task_timeout_millisecs;

I would suggest an '_msec' suffix to go along with ...

> @@ -37,6 +37,7 @@ int __read_mostly sysctl_hung_task_check_count = PID_MAX_LIMIT;
>   * the RCU grace period. So it needs to be upper-bound.
>   */
>  #define HUNG_TASK_LOCK_BREAK (HZ / 10)
> +#define SECONDS 1000

We have #define MSEC_PER_SEC      1000L

> @@ -44,9 +45,14 @@ int __read_mostly sysctl_hung_task_check_count = PID_MAX_LIMIT;
>  unsigned long __read_mostly sysctl_hung_task_timeout_secs = CONFIG_DEFAULT_HUNG_TASK_TIMEOUT;
>  
>  /*
> + * Zero means only use sysctl_hung_task_timeout_secs
> + */
> +unsigned long  __read_mostly sysctl_hung_task_timeout_millisecs;

Why not:

unsigned long  __read_mostly sysctl_hung_task_timeout_msec = \
		CONFIG_DEFAULT_HUNG_TASK_TIMEOUT * MSEC_PER_SEC;

> @@ -108,7 +114,8 @@ static void check_hung_task(struct task_struct *t, unsigned long timeout)
>  		t->last_switch_time = jiffies;
>  		return;
>  	}
> -	if (time_is_after_jiffies(t->last_switch_time + timeout * HZ))
> +
> +	if (time_is_after_jiffies(t->last_switch_time + (timeout * HZ) / SECONDS))
>  		return;

We have msecs_to_jiffies() which handles the rounding properly for you.

> -		pr_err("INFO: task %s:%d blocked for more than %ld seconds.\n",
> -		       t->comm, t->pid, (jiffies - t->last_switch_time) / HZ);
> +
> +		pr_err("INFO: task %s:%d blocked for more than %ld seconds %ld milliseconds.\n",

I'd use "%ld.%0.3ld seconds" ... or whatever the right format string is.
I have to work it out every time.

> +			t->comm, t->pid, (jiffies - t->last_switch_time) / HZ,
> +			(jiffies - t->last_switch_time) % HZ * (SECONDS / HZ));

... and again, use jiffies_to_msec() to get the rounding right.