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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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”<[email protected] <mailto:[email protected]>> 写入:
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 <[email protected] <mailto:[email protected]>>
> ---
> 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
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