Message-ID: <b0867f5d-6f82-6754-2eb4-137a14940d31@codeaurora.org>
Date:   Tue, 22 Aug 2017 11:33:31 +0530
From:   Imran Khan <kimran@...eaurora.org>
To:     "Zhang, Shile (NSB - CN/Hangzhou)" <shile.zhang@...ia-sbell.com>,
        "mingo@...nel.org" <mingo@...nel.org>
Cc:     "imrank140517@...il.com" <imrank140517@...il.com>,
        "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Matt Fleming <matt@...eblueprint.co.uk>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Vegard Nossum <vegard.nossum@...cle.com>,
        Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
        John Siddle <jsiddle@...hat.com>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:PROC SYSCTL" <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] RFC: hung task: Check specific tasks for long
 uninterruptible sleep state

On 8/22/2017 7:24 AM, Zhang, Shile (NSB - CN/Hangzhou) wrote:

> Hi, Imran,
> 
Hi Shile,
Thanks for getting back on this.
> I think a "unmonitored list" is better than "monitor list", because we want khungtaskd can find out the "unexpected" hung task, but not few in a list.
> Then, for the fg tasks, which can put it in the "unmonitored list", for the bg tasks, I think we can tweak the timeout to control the duration.
> 
Here fg and bg tasks were just an example and may not be applicable for all environments (non-android). But main intention here is to have
some mechanism that would allow to monitor only specific tasks. As we have a sysctl option to enable/disable this mechanism we can at any time
fall back to default mechanism of monitoring all tasks.

Alternatively this can be done on task priority basis also, where we have a minimum priority and only tasks with priorities higher than
this min priority would be monitored and if we keep this min priority to the least priority , all tasks would be monitored like the 
default case. Please let me know your feedback regarding this approach.

Thanks and Regards,
Imran 
> Thanks!
> 
> BR, Shile
> 
> -----Original Message-----
> From: Imran Khan [mailto:kimran@...eaurora.org] 
> Sent: Monday, August 21, 2017 6:26 PM
> To: mingo@...nel.org
> Cc: imrank140517@...il.com; Imran Khan <kimran@...eaurora.org>; Luis R. Rodriguez <mcgrof@...nel.org>; Kees Cook <keescook@...omium.org>; Peter Zijlstra (Intel) <peterz@...radead.org>; Zhang, Shile (NSB - CN/Hangzhou) <shile.zhang@...ia-sbell.com>; Matt Fleming <matt@...eblueprint.co.uk>; Andrew Morton <akpm@...ux-foundation.org>; Vegard Nossum <vegard.nossum@...cle.com>; Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>; John Siddle <jsiddle@...hat.com>; open list <linux-kernel@...r.kernel.org>; open list:PROC SYSCTL <linux-fsdevel@...r.kernel.org>
> Subject: [PATCH] RFC: hung task: Check specific tasks for long uninterruptible sleep state
> 
> khungtask by default monitors either all tasks or no tasks at all
> for long unterruptible sleeps.
> For Android like environments this arrangement is not optimal because
> on one hand it may be permissible to have some background(bg) task
> in uninterruptible sleep state for long duration while on the other
> hand it may not be permissible to have some foreground(fg) task like
> surfaceflinger in uninterruptible sleep state for long duration.
> So it would be good to have some arrangement so that few specified tasks
> can be monitored by khungtaskd, on a need basis.
> This change introduces a sysctl option, /proc/sys/kernel/
> hung_task_check_selected, to enable monitoring of selected tasks using
> khungtask daemon. If this sysctl option is enabled then only the tasks
> specified in /proc/hung_task_monitor_list are monitored otherwise all
> tasks are monitored, just like the default case.
> 
> Signed-off-by: Imran Khan <kimran@...eaurora.org>
> ---
>  include/linux/sched/sysctl.h |   1 +
>  kernel/hung_task.c           | 121 ++++++++++++++++++++++++++++++++++++++++++-
>  kernel/sysctl.c              |   8 +++
>  3 files changed, 128 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/sched/sysctl.h b/include/linux/sched/sysctl.h
> index 0f5ecd4..05892f1 100644
> --- a/include/linux/sched/sysctl.h
> +++ b/include/linux/sched/sysctl.h
> @@ -10,6 +10,7 @@
>  extern unsigned int  sysctl_hung_task_panic;
>  extern unsigned long sysctl_hung_task_timeout_secs;
>  extern int sysctl_hung_task_warnings;
> +extern int sysctl_hung_task_check_selected;
>  extern int proc_dohung_task_timeout_secs(struct ctl_table *table, int write,
>  					 void __user *buffer,
>  					 size_t *lenp, loff_t *ppos);
> diff --git a/kernel/hung_task.c b/kernel/hung_task.c
> index 751593e..49f13fb 100644
> --- a/kernel/hung_task.c
> +++ b/kernel/hung_task.c
> @@ -16,12 +16,28 @@
>  #include <linux/export.h>
>  #include <linux/sysctl.h>
>  #include <linux/utsname.h>
> +#include <linux/uaccess.h>
> +#include <linux/slab.h>
> +#include <linux/proc_fs.h>
>  #include <linux/sched/signal.h>
>  #include <linux/sched/debug.h>
>  
>  #include <trace/events/sched.h>
>  
>  /*
> + * Hung task that needs monitoring
> + */
> +struct hung_task {
> +	struct list_head list;
> +	char comm[TASK_COMM_LEN];
> +};
> +
> +static struct hung_task	*monitor_list;
> +int sysctl_hung_task_check_selected;
> +
> +
> +
> +/*
>   * The number of tasks checked:
>   */
>  int __read_mostly sysctl_hung_task_check_count = PID_MAX_LIMIT;
> @@ -76,6 +92,92 @@ static int __init hung_task_panic_setup(char *str)
>  	.notifier_call = hung_task_panic,
>  };
>  
> +static void hung_task_monitor_setup(void)
> +{
> +	monitor_list = kmalloc(sizeof(*monitor_list), GFP_KERNEL);
> +	if (monitor_list) {
> +		INIT_LIST_HEAD(&monitor_list->list);
> +		memset(monitor_list->comm, 0, TASK_COMM_LEN);
> +	}
> +}
> +
> +
> +static  int hung_task_info_show(struct seq_file *m, void *v)
> +{
> +	struct hung_task *ht;
> +
> +	ht = list_entry(v, struct hung_task, list);
> +	seq_puts(m, ht->comm);
> +
> +	return 0;
> +}
> +
> +static void *hung_task_info_start(struct seq_file *m, loff_t *pos)
> +{
> +	return seq_list_start_head(&monitor_list->list, *pos);
> +}
> +
> +static void *hung_task_info_next(struct seq_file *m, void *v, loff_t *pos)
> +{
> +	return seq_list_next(v, &monitor_list->list, pos);
> +}
> +
> +static void hung_task_info_stop(struct seq_file *m, void *v)
> +{
> +}
> +
> +const struct seq_operations hung_task_info_op = {
> +	.start	= hung_task_info_start,
> +	.next	= hung_task_info_next,
> +	.stop	= hung_task_info_stop,
> +	.show	= hung_task_info_show
> +};
> +
> +static int hung_task_info_open(struct inode *inode, struct file *file)
> +{
> +	return seq_open(file, &hung_task_info_op);
> +}
> +
> +static ssize_t
> +hung_task_info_write(struct file *file, const char __user *buf, size_t count,
> +	     loff_t *offs)
> +{
> +	struct task_struct *g, *t;
> +	struct hung_task *ht = kmalloc(sizeof(*ht), GFP_KERNEL);
> +
> +	if (!ht)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(ht->comm, buf, count))
> +		return -EFAULT;
> +	ht->comm[count] = '\0';
> +
> +	for_each_process_thread(g, t) {
> +		if (!strncmp(t->comm, ht->comm, strlen(t->comm))) {
> +			list_add_tail(&ht->list, &monitor_list->list);
> +			return count;
> +		}
> +	}
> +
> +	pr_err("Non-existing task: %s can't be monitored\n", ht->comm);
> +	return count;
> +}
> +
> +static const struct file_operations hung_task_info_operations = {
> +	.open		= hung_task_info_open,
> +	.read		= seq_read,
> +	.write		= hung_task_info_write,
> +	.llseek		= seq_lseek,
> +	.release	= seq_release,
> +};
> +
> +static int __init proc_hung_task_info_init(void)
> +{
> +	proc_create("hung_task_monitor_list", 0644, NULL,
> +		    &hung_task_info_operations);
> +	return 0;
> +}
> +
>  static void check_hung_task(struct task_struct *t, unsigned long timeout)
>  {
>  	unsigned long switch_count = t->nvcsw + t->nivcsw;
> @@ -167,6 +269,7 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)
>  	int max_count = sysctl_hung_task_check_count;
>  	int batch_count = HUNG_TASK_BATCHING;
>  	struct task_struct *g, *t;
> +	struct hung_task *ht, *tmp;
>  
>  	/*
>  	 * If the system crashed already then all bets are off,
> @@ -186,9 +289,21 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)
>  				goto unlock;
>  		}
>  		/* use "==" to skip the TASK_KILLABLE tasks waiting on NFS */
> -		if (t->state == TASK_UNINTERRUPTIBLE)
> -			check_hung_task(t, timeout);
> +		if (t->state == TASK_UNINTERRUPTIBLE) {
> +			if (sysctl_hung_task_check_selected) {
> +				list_for_each_entry_safe(ht, tmp,
> +							 &monitor_list->list,
> +							 list)
> +					if (!strncmp(ht->comm, t->comm,
> +					    strlen(t->comm)))
> +					/* Task belongs to the selected group */
> +						check_hung_task(t, timeout);
> +			} else {
> +				check_hung_task(t, timeout);
> +			}
> +		}
>  	}
> +
>   unlock:
>  	rcu_read_unlock();
>  	if (hung_task_show_lock)
> @@ -259,6 +374,8 @@ static int watchdog(void *dummy)
>  static int __init hung_task_init(void)
>  {
>  	atomic_notifier_chain_register(&panic_notifier_list, &panic_block);
> +	hung_task_monitor_setup();
> +	proc_hung_task_info_init();
>  	watchdog_task = kthread_run(watchdog, NULL, "khungtaskd");
>  
>  	return 0;
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 6648fbb..ab45774 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1096,6 +1096,14 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write,
>  		.proc_handler	= proc_dointvec_minmax,
>  		.extra1		= &neg_one,
>  	},
> +	{
> +		.procname	= "hung_task_check_selected",
> +		.data		= &sysctl_hung_task_check_selected,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0644,
> +		.proc_handler	= proc_dointvec_minmax,
> +		.extra1		= &neg_one,
> +	},
>  #endif
>  #ifdef CONFIG_RT_MUTEXES
>  	{
> 


-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation

Powered by blists - more mailing lists