linux-kernel - Re: [PATCH] sched: print parent comm in sched_show

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <37FFCAF0-B0BA-48BD-B688-B1C5E7A10A1A@didiglobal.com>
Date:   Tue, 17 Jan 2023 08:33:56 +0000
From:   张元瀚 Tio Zhang <tiozhang@...iglobal.com>
To:     Chen Yu <yu.c.chen@...el.com>
CC:     Peter Zijlstra <peterz@...radead.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Ingo Molnar <mingo@...hat.com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Petr Mladek <pmladek@...e.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Yuanhan Zhang <zyhtheonly@...il.com>,
        "zwp10758@...il.com" <zwp10758@...il.com>,
        "zyhtheonly@...h.net" <zyhtheonly@...h.net>
Subject: Re: [PATCH] sched: print parent comm in sched_show_task()

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@...el.com <mailto:yu.c.chen@...el.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@...iglobal.com <mailto:tiozhang@...iglobal.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