* [PATCH] sched: print parent comm in sched_show_task()
@ 2023-01-13 10:54 Tio Zhang
2023-01-16 9:26 ` Chen Yu
0 siblings, 1 reply; 10+ messages in thread
From: Tio Zhang @ 2023-01-13 10:54 UTC (permalink / raw)
To: yu.c.chen, pmladek, tiozhang, zyhtheonly; +Cc: linux-kernel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] sched: print parent comm in sched_show_task()
2023-01-13 10:54 [PATCH] sched: print parent comm in sched_show_task() Tio Zhang
@ 2023-01-16 9:26 ` Chen Yu
2023-01-17 8:33 ` 张元瀚 Tio Zhang
0 siblings, 1 reply; 10+ messages in thread
From: Chen Yu @ 2023-01-16 9:26 UTC (permalink / raw)
To: Tio Zhang
Cc: Peter Zijlstra, Vincent Guittot, Ingo Molnar, Juri Lelli,
Petr Mladek, LKML
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] sched: print parent comm in sched_show_task()
2023-01-16 9:26 ` Chen Yu
@ 2023-01-17 8:33 ` 张元瀚 Tio Zhang
2023-01-18 11:06 ` Petr Mladek
0 siblings, 1 reply; 10+ messages in thread
From: 张元瀚 Tio Zhang @ 2023-01-17 8:33 UTC (permalink / raw)
To: Chen Yu
Cc: Peter Zijlstra, Vincent Guittot, Ingo Molnar, Juri Lelli,
Petr Mladek, LKML, Yuanhan Zhang, zwp10758, zyhtheonly
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] sched: print parent comm in sched_show_task()
2023-01-17 8:33 ` 张元瀚 Tio Zhang
@ 2023-01-18 11:06 ` Petr Mladek
0 siblings, 0 replies; 10+ messages in thread
From: Petr Mladek @ 2023-01-18 11:06 UTC (permalink / raw)
To: 张元瀚 Tio Zhang
Cc: Chen Yu, Peter Zijlstra, Vincent Guittot, Ingo Molnar,
Juri Lelli, LKML, Yuanhan Zhang, zwp10758, zyhtheonly
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] sched: print parent comm in sched_show_task()
@ 2022-12-27 16:14 Tio Zhang
2022-12-29 4:25 ` Chen Yu
2023-01-04 13:24 ` Petr Mladek
0 siblings, 2 replies; 10+ messages in thread
From: Tio Zhang @ 2022-12-27 16:14 UTC (permalink / raw)
To: mingo, peterz, juri.lelli, vincent.guittot
Cc: linux-kernel, pmladek, zyhtheonly, zyhtheonly, zwp10758
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] sched: print parent comm in sched_show_task()
2022-12-27 16:14 Tio Zhang
@ 2022-12-29 4:25 ` Chen Yu
[not found] ` <CAEQmJ=gcCx1hMf7HicE5OFeUstipdtr=3JkF1JxLuP-CrG++Pw@mail.gmail.com>
2023-01-04 13:24 ` Petr Mladek
1 sibling, 1 reply; 10+ messages in thread
From: Chen Yu @ 2022-12-29 4:25 UTC (permalink / raw)
To: mingo, peterz, juri.lelli, vincent.guittot, linux-kernel,
pmladek, zyhtheonly, zyhtheonly, zwp10758
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,
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] sched: print parent comm in sched_show_task()
2022-12-27 16:14 Tio Zhang
2022-12-29 4:25 ` Chen Yu
@ 2023-01-04 13:24 ` Petr Mladek
1 sibling, 0 replies; 10+ messages in thread
From: Petr Mladek @ 2023-01-04 13:24 UTC (permalink / raw)
To: mingo, peterz, juri.lelli, vincent.guittot, linux-kernel,
zyhtheonly, zyhtheonly, zwp10758
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-01-18 11:48 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-13 10:54 [PATCH] sched: print parent comm in sched_show_task() Tio Zhang
2023-01-16 9:26 ` Chen Yu
2023-01-17 8:33 ` 张元瀚 Tio Zhang
2023-01-18 11:06 ` Petr Mladek
-- strict thread matches above, loose matches on Subject: below --
2022-12-27 16:14 Tio Zhang
2022-12-29 4:25 ` Chen Yu
[not found] ` <CAEQmJ=gcCx1hMf7HicE5OFeUstipdtr=3JkF1JxLuP-CrG++Pw@mail.gmail.com>
2023-01-04 13:40 ` Petr Mladek
2023-01-06 9:51 ` Chen Yu
[not found] ` <CAEQmJ=gZJL6K1yUPq0hHy5D7Rc6g=5Ri72V_kE=xfqR6gJedWg@mail.gmail.com>
2023-01-12 8:40 ` Chen Yu
2023-01-04 13:24 ` Petr Mladek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.