Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 18 Jan 2023 12:06:49 +0100
From:   Petr Mladek <pmladek@suse.com>
To:     =?utf-8?B?5byg5YWD54Ca?= Tio Zhang <tiozhang@didiglobal.com>
Cc:     Chen Yu <yu.c.chen@intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Ingo Molnar <mingo@redhat.com>,
        Juri Lelli <juri.lelli@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Yuanhan Zhang <zyhtheonly@gmail.com>,
        "zwp10758@gmail.com" <zwp10758@gmail.com>,
        "zyhtheonly@yeah.net" <zyhtheonly@yeah.net>
Subject: Re: [PATCH] sched: print parent comm in sched_show_task()
Message-ID: <Y8fSyVvnmoNnxiiD@alley>
References: <Y8UYKzBJeejHCpgO@chenyu5-mobl1>
 <37FFCAF0-B0BA-48BD-B688-B1C5E7A10A1A@didiglobal.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <37FFCAF0-B0BA-48BD-B688-B1C5E7A10A1A@didiglobal.com>
Precedence: bulk

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