[PATCH] sched: print parent comm in sched_show_task()

[PATCH] sched: print parent comm in sched_show_task()

[Tio Zhang]

Knowing who the parent is might be useful for debugging.
For example, we can sometimes resolve kernel hung tasks by stopping
the person who begins those hung tasks.
With the parent's name printed in sched_show_task(),
it might be helpful to let people know which "service" should be operated.

Signed-off-by: Tio Zhang <tiozhang@didiglobal.com>
---
 kernel/sched/core.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index cb2aa2b54c7a..6f4aef0fed58 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8854,6 +8854,7 @@ void sched_show_task(struct task_struct *p)
 {
 	unsigned long free = 0;
 	int ppid;
+	char pcomm[TASK_COMM_LEN];
 
 	if (!try_get_task_stack(p))
 		return;
@@ -8867,11 +8868,13 @@ void sched_show_task(struct task_struct *p)
 #endif
 	ppid = 0;
 	rcu_read_lock();
-	if (pid_alive(p))
+	if (pid_alive(p)) {
 		ppid = task_pid_nr(rcu_dereference(p->real_parent));
+		get_task_comm(pcomm, rcu_dereference(p->real_parent));
+	}
 	rcu_read_unlock();
-	pr_cont(" stack:%-5lu pid:%-5d ppid:%-6d flags:0x%08lx\n",
-		free, task_pid_nr(p), ppid,
+	pr_cont(" stack:%-5lu pid:%-5d ppid:%-6d parent:%-15.15s flags:0x%08lx\n",
+		free, task_pid_nr(p), ppid, pcomm,
 		read_task_thread_flags(p));
 
 	print_worker_info(KERN_INFO, p);
-- 
2.17.1

Re: [PATCH] sched: print parent comm in sched_show_task()

[Chen Yu]

On 2022-12-28 at 00:14:00 +0800, Tio Zhang wrote:
> Knowing who the parent is might be useful for debugging.
> For example, we can sometimes resolve kernel hung tasks by stopping
> the person who begins those hung tasks.
> With the parent's name printed in sched_show_task(),
> it might be helpful to let people know which "service" should be operated.
> 
> Signed-off-by: Tio Zhang <tiozhang@didiglobal.com>
> ---
>  kernel/sched/core.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index cb2aa2b54c7a..6f4aef0fed58 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -8854,6 +8854,7 @@ void sched_show_task(struct task_struct *p)
>  {
>  	unsigned long free = 0;
>  	int ppid;
> +	char pcomm[TASK_COMM_LEN];
>  
>  	if (!try_get_task_stack(p))
>  		return;
> @@ -8867,11 +8868,13 @@ void sched_show_task(struct task_struct *p)
>  #endif
>  	ppid = 0;
>  	rcu_read_lock();
> -	if (pid_alive(p))
> +	if (pid_alive(p)) {
>  		ppid = task_pid_nr(rcu_dereference(p->real_parent));
> +		get_task_comm(pcomm, rcu_dereference(p->real_parent));
Maybe struct task_struct *parent = rcu_dereference(p->real_parent);
and use parent directly to get its pid and comm?
Maybe off-topic, what if the parent is a kernel thread/worker? It might have extra
name information such as kthread->full_name or worker->desc according to proc_task_name().

thanks,

Re: [PATCH] sched: print parent comm in sched_show_task()

[Petr Mladek]

On Wed 2023-01-04 01:51:30, 张元瀚 wrote:
> Hi Chen,
> Thanks for your advice!
> 
> > Maybe struct task_struct *parent = rcu_dereference(p->real_parent);
> > and use parent directly to get its pid and comm?
> 
> Yes! It is good to write this way.
> 
> > Maybe off-topic, what if the parent is a kernel thread/worker? It might
> have extra
> > name information such as kthread->full_name or worker->desc according to
> proc_task_name().
> 
> I'm not quite sure if it is necessary to fetch that extra information since
> our sched_show_task() prints p->comm ourselves.
> But, assuming we get the parent's name in the same way we get
> proc_task_name(), there are some new issues I'd like to discuss.

I suggest to keep it simple and just print "parent->comm".

Especially, we should avoid taking any lock. shed_show_task() might
be called when there already is a deadlock on the system. I guess
that it even can be called from NMI.

For example, see print_worker_info(). It uses
copy_from_kernel_nofault() to be safe without taking any lock.

Best Regards,
Petr

Re: [PATCH] sched: print parent comm in sched_show_task()

[Chen Yu]

Hi Tio,
On 2023-01-04 at 01:51:30 +0800, 张元瀚 wrote:
> Hi Chen,
> Thanks for your advice!
> 
> > Maybe struct task_struct *parent = rcu_dereference(p->real_parent);
> > and use parent directly to get its pid and comm?
> 
> Yes! It is good to write this way.
> 
> > Maybe off-topic, what if the parent is a kernel thread/worker? It might
> have extra
> > name information such as kthread->full_name or worker->desc according to
> proc_task_name().
> 
> I'm not quite sure if it is necessary to fetch that extra information since
> our sched_show_task() prints p->comm ourselves.
> But, assuming we get the parent's name in the same way we get
> proc_task_name(), there are some new issues I'd like to discuss.
> For example, if we write like this:
Petr suggested to keep it simple in another thread, so I think we do not
need to let extra information involve in for now.

thanks,
Chenyu

Re: [PATCH] sched: print parent comm in sched_show_task()

[Chen Yu]

Hi,
On 2023-01-10 at 17:35:34 +0800, Yuanhan Zhang wrote:
> Hi Petr and Chen,
> Thanks for your emails! Petr's suggestions sure make a lot of sense.
> Please review the following implementation, it would print a new line like:
>  " parent:kthreadd        ppid:2 "
>  after the origin line.
> 
> ---
> 
>  kernel/sched/core.c | 15 ++++++++-------
> 
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> 
> index cb2aa2b54c7a..86ee90404ddb 100644
> 
> --- a/kernel/sched/core.c
> 
> +++ b/kernel/sched/core.c
> 
> @@ -8853,7 +8853,7 @@ SYSCALL_DEFINE2(sched_rr_get_interval_time32, pid_t,
> pid,
> 
>  void sched_show_task(struct task_struct *p)
> 
>  {
> 
>   unsigned long free = 0;
> 
> - int ppid;
> 
> + struct task_struct *parent;
> 
Above line could be moved inside: if (pid_alive(p)).
And the format of this patch seems to be broken, please resend after fixing the issues
reported by scripts/checkpatch.pl

thanks,
Chenyu

Re: [PATCH] sched: print parent comm in sched_show_task()

[Petr Mladek]

On Wed 2022-12-28 00:14:00, Tio Zhang wrote:
> Knowing who the parent is might be useful for debugging.
> For example, we can sometimes resolve kernel hung tasks by stopping
> the person who begins those hung tasks.
> With the parent's name printed in sched_show_task(),
> it might be helpful to let people know which "service" should be operated.

> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -8854,6 +8854,7 @@ void sched_show_task(struct task_struct *p)
>  {
>  	unsigned long free = 0;
>  	int ppid;
> +	char pcomm[TASK_COMM_LEN];
>  
>  	if (!try_get_task_stack(p))
>  		return;
> @@ -8867,11 +8868,13 @@ void sched_show_task(struct task_struct *p)
>  #endif
>  	ppid = 0;

We need to intialized pcomm here:

	pcomm[0] = '\0';

Otherwise, it would include a garbage when pid_alive(p) returns false below..

>  	rcu_read_lock();
> -	if (pid_alive(p))
> +	if (pid_alive(p)) {
>  		ppid = task_pid_nr(rcu_dereference(p->real_parent));
> +		get_task_comm(pcomm, rcu_dereference(p->real_parent));
> +	}
>  	rcu_read_unlock();
> -	pr_cont(" stack:%-5lu pid:%-5d ppid:%-6d flags:0x%08lx\n",
> -		free, task_pid_nr(p), ppid,
> +	pr_cont(" stack:%-5lu pid:%-5d ppid:%-6d parent:%-15.15s
> flags:0x%08lx\n",

It would print:  .... parent:xxx flags:0000

Some people might be confused whether the flags are from
the task or from the parent.

A solution would be to move the parent value to another line.
It would even better solve the situation when the task
is not alive and we could not get information about the parent:

	if (pid_alive(p)) {
		struct parent = rcu_dereference(p->real_parent);

		pr_info("parent:%-15.15s ppid:%-6d\n",
			parent->comm, task_pid_nr(parent));
	}

> +		free, task_pid_nr(p), ppid, pcomm,
>  		read_task_thread_flags(p));
>  
>  	print_worker_info(KERN_INFO, p);

Best Regards,
Petr

[PATCH] sched: print parent comm in sched_show_task()

[Tio Zhang]

Knowing who the parent is might be useful for debugging.
For example, we can sometimes resolve kernel hung tasks by stopping
the person who begins those hung tasks.
With the parent's name printed in sched_show_task(),
it might be helpful to let people know which "service" should be operated.
Also, we move the parent info to a following new line.
It would better solve the situation when the task
is not alive and we could not get information about the parent.

Signed-off-by: Tio Zhang <tiozhang@didiglobal.com>
---
 kernel/sched/core.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index cb2aa2b54c7a..5be3f476ee5b 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8853,7 +8853,6 @@ SYSCALL_DEFINE2(sched_rr_get_interval_time32, pid_t, pid,
 void sched_show_task(struct task_struct *p)
 {
 	unsigned long free = 0;
-	int ppid;
 
 	if (!try_get_task_stack(p))
 		return;
@@ -8865,14 +8864,16 @@ void sched_show_task(struct task_struct *p)
 #ifdef CONFIG_DEBUG_STACK_USAGE
 	free = stack_not_used(p);
 #endif
-	ppid = 0;
+	pr_cont(" stack:%-5lu pid:%-5d flags:0x%08lx\n",
+		free, task_pid_nr(p), read_task_thread_flags(p));
+
 	rcu_read_lock();
-	if (pid_alive(p))
-		ppid = task_pid_nr(rcu_dereference(p->real_parent));
+	if (pid_alive(p)) {
+		struct task_struct *parent = rcu_dereference(p->real_parent);
+
+		pr_info("parent:%-15.15s ppid:%-6d", parent->comm, task_pid_nr(parent));
+	}
 	rcu_read_unlock();
-	pr_cont(" stack:%-5lu pid:%-5d ppid:%-6d flags:0x%08lx\n",
-		free, task_pid_nr(p), ppid,
-		read_task_thread_flags(p));
 
 	print_worker_info(KERN_INFO, p);
 	print_stop_info(KERN_INFO, p);
-- 
2.17.1

Re: [PATCH] sched: print parent comm in sched_show_task()

[Chen Yu]

Hi Tio,
On 2023-01-13 at 18:54:13 +0800, Tio Zhang wrote:
> Knowing who the parent is might be useful for debugging.
> For example, we can sometimes resolve kernel hung tasks by stopping
> the person who begins those hung tasks.
> With the parent's name printed in sched_show_task(),
> it might be helpful to let people know which "service" should be operated.
> Also, we move the parent info to a following new line.
> It would better solve the situation when the task
> is not alive and we could not get information about the parent.
>
Maybe generate the patch via git format-patch -v2 because it is a second
one. Also Cced corresponding sched maintainers as they will make the decision.
> Signed-off-by: Tio Zhang <tiozhang@didiglobal.com>
> ---
>  kernel/sched/core.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index cb2aa2b54c7a..5be3f476ee5b 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -8853,7 +8853,6 @@ SYSCALL_DEFINE2(sched_rr_get_interval_time32, pid_t, pid,
>  void sched_show_task(struct task_struct *p)
>  {
>  	unsigned long free = 0;
> -	int ppid;
>  
>  	if (!try_get_task_stack(p))
>  		return;
> @@ -8865,14 +8864,16 @@ void sched_show_task(struct task_struct *p)
>  #ifdef CONFIG_DEBUG_STACK_USAGE
>  	free = stack_not_used(p);
>  #endif
> -	ppid = 0;
> +	pr_cont(" stack:%-5lu pid:%-5d flags:0x%08lx\n",
> +		free, task_pid_nr(p), read_task_thread_flags(p));
> +
>  	rcu_read_lock();
> -	if (pid_alive(p))
> -		ppid = task_pid_nr(rcu_dereference(p->real_parent));
> +	if (pid_alive(p)) {
> +		struct task_struct *parent = rcu_dereference(p->real_parent);
> +
> +		pr_info("parent:%-15.15s ppid:%-6d", parent->comm, task_pid_nr(parent));
This might change the original scenario: if !pid_alive(p), the sched_show_task()
will print ppid = 0(init task?).

thanks,
Chenyu

Re: [PATCH] sched: print parent comm in sched_show_task()

[张元瀚 Tio Zhang]

Hi Chen,
Thanks for your reply! I implement this according to Petr's suggestion here:

> A solution would be to move the parent value to another line.
> It would even better solve the situation when the task
> is not alive and we could not get information about the parent:
>
>        if (pid_alive(p)) {
>                struct parent = rcu_dereference(p->real_parent);
>
>                pr_info("parent:%-15.15s ppid:%-6d\n",
>                        parent->comm, task_pid_nr(parent));
>        }

It seems do break the original format, but I guess printing 0 as ppid when the task is not alive
would also confuse people sometimes.

For example, when people (and also most system monitor software) see ppid, they read the value in /proc/PID/status. According to task_tgid_nr_ns(), when the task is in a container with its parent outside the
namespace, we will also see that ppid is 0 inside the container. And in our sched_show_task() here, we are calling task_pid_nr(), so the inconsistency maybe would confuse people under this scenario.

So maybe this new line style would be a better choice? Or we just keep the original format and
move the parent's info (and should we print the parent's pid again here) to a new line.   

Thanks,
Tio

在 2023/1/16 下午5:26，“Chen Yu”<yu.c.chen@intel.com <mailto:yu.c.chen@intel.com>> 写入:


Hi Tio,
On 2023-01-13 at 18:54:13 +0800, Tio Zhang wrote:
> Knowing who the parent is might be useful for debugging.
> For example, we can sometimes resolve kernel hung tasks by stopping
> the person who begins those hung tasks.
> With the parent's name printed in sched_show_task(),
> it might be helpful to let people know which "service" should be operated.
> Also, we move the parent info to a following new line.
> It would better solve the situation when the task
> is not alive and we could not get information about the parent.
>
Maybe generate the patch via git format-patch -v2 because it is a second
one. Also Cced corresponding sched maintainers as they will make the decision.
> Signed-off-by: Tio Zhang <tiozhang@didiglobal.com <mailto:tiozhang@didiglobal.com>>
> ---
> kernel/sched/core.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index cb2aa2b54c7a..5be3f476ee5b 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -8853,7 +8853,6 @@ SYSCALL_DEFINE2(sched_rr_get_interval_time32, pid_t, pid,
> void sched_show_task(struct task_struct *p)
> {
> unsigned long free = 0;
> - int ppid;
> 
> if (!try_get_task_stack(p))
> return;
> @@ -8865,14 +8864,16 @@ void sched_show_task(struct task_struct *p)
> #ifdef CONFIG_DEBUG_STACK_USAGE
> free = stack_not_used(p);
> #endif
> - ppid = 0;
> + pr_cont(" stack:%-5lu pid:%-5d flags:0x%08lx\n",
> + free, task_pid_nr(p), read_task_thread_flags(p));
> +
> rcu_read_lock();
> - if (pid_alive(p))
> - ppid = task_pid_nr(rcu_dereference(p->real_parent));
> + if (pid_alive(p)) {
> + struct task_struct *parent = rcu_dereference(p->real_parent);
> +
> + pr_info("parent:%-15.15s ppid:%-6d", parent->comm, task_pid_nr(parent));
This might change the original scenario: if !pid_alive(p), the sched_show_task()
will print ppid = 0(init task?).


thanks,
Chenyu

Re: [PATCH] sched: print parent comm in sched_show_task()

[Petr Mladek]

On Tue 2023-01-17 08:33:56, 张元瀚 Tio Zhang wrote:
> Hi Chen,
> Thanks for your reply! I implement this according to Petr's suggestion here:
> 
> > A solution would be to move the parent value to another line.
> > It would even better solve the situation when the task
> > is not alive and we could not get information about the parent:
> >
> >        if (pid_alive(p)) {
> >                struct parent = rcu_dereference(p->real_parent);
> >
> >                pr_info("parent:%-15.15s ppid:%-6d\n",
> >                        parent->comm, task_pid_nr(parent));
> >        }
> 
> It seems do break the original format, but I guess printing 0 as ppid when the task is not alive
> would also confuse people sometimes.

Well, a task with pid 0 does not exist so it is not that bad.
But I agree that we could do better.

> For example, when people (and also most system monitor software) see ppid, they read the value in /proc/PID/status. According to task_tgid_nr_ns(), when the task is in a container with its parent outside the
> namespace, we will also see that ppid is 0 inside the container. And in our sched_show_task() here, we are calling task_pid_nr(), so the inconsistency maybe would confuse people under this scenario.
> 
> So maybe this new line style would be a better choice? Or we just keep the original format and
> move the parent's info (and should we print the parent's pid again here) to a new line.   

What about printing something like:

		pr_info("parent:unknown\n");

or

		pr_info("parent:unknown         ppid:<NULL>;

or

		pr_info("parent:???\n");

or

		pr_info("parent:unknown (task is exiting)\n");


I slightly prefer the 2nd variant. The <NULL> string makes it rather
clear that the information is not accessible. And pid_alive() actually
does:

	return p->thread_pid != NULL;

Best Regards,
Petr