[PATCH] drm: Include task->name and master status in debugfs clients info

[David Herrmann]

Hi

On Mon, Sep 1, 2014 at 4:19 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> On Mon, Sep 01, 2014 at 04:11:43PM +0200, David Herrmann wrote:
>> Hi
>>
>> On Sat, Aug 9, 2014 at 8:22 AM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
>> > Showing who is the current master is useful for trying to decypher
>> > errors when trying to acquire master (e.g. a race with X taking over
>> > from plymouth). By including the process name as well as the pid
>> > simplifies the task of grabbing enough information remotely at the point
>> > of error.
>> >
>> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>> > ---
>> >  drivers/gpu/drm/drm_info.c | 17 ++++++++++++-----
>> >  1 file changed, 12 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c
>> > index 15ec9f4..d813430 100644
>> > --- a/drivers/gpu/drm/drm_info.c
>> > +++ b/drivers/gpu/drm/drm_info.c
>> > @@ -184,14 +184,21 @@ int drm_clients_info(struct seq_file *m, void *data)
>> >         struct drm_file *priv;
>> >
>> >         mutex_lock(&dev->struct_mutex);
>> > -       seq_printf(m, "a dev    pid    uid      magic\n\n");
>> > -       list_for_each_entry(priv, &dev->filelist, lhead) {
>> > -               seq_printf(m, "%c %3d %5d %5d %10u\n",
>> > -                          priv->authenticated ? 'y' : 'n',
>> > -                          priv->minor->index,
>> > +       seq_printf(m, "                       pid dev master a   uid      magic\n");
>>
>> Maybe mention "task" here? Or is this supposed to be a logical part of "pid"?
>
> Yeah, I was thinking that comm/pid were essentially the same column. Top
> uses command which would be reasonable to reuse.

Sounds all fine. Just wanted to go sure it's not a typo.

>> > +       list_for_each_entry_reverse(priv, &dev->filelist, lhead) {
>>
>> No idea why you do backwards traversal, but ok..
>
> Clients are added youngest-first. So traversing backwards gives
>   kernel
>   X/display manager
>   clients
>
>> > +               struct task_struct *task;
>> > +
>> > +               rcu_read_lock();
>>
>> What's that rcu-lock for? task->comm is pre-allocated, priv->pid is
>> static, kuid... no idea?
>
> Cargo-culting the locking from the discussion with Tetsuo:
>
> commit 3ec2f427e6f82b9b8f9b18dd2c758b864385df39
> Author: Tetsuo Handa <penguin-kernel at I-love.SAKURA.ne.jp>
> Date:   Fri Jan 3 20:42:18 2014 +0900
>
>     drm/i915: Fix refcount leak and possible NULL pointerdereference.
>
>     Since get_pid_task() grabs a reference on the task_struct, we have to drop the
>     refcount after reading that task's comm name. Use pid_task() with RCU instead.
>
>     Also, avoid directly reading like pid_task()->comm because
>     pid_task() will return NULL if the task have already exit()ed.
>
>     This patch fixes both problems.

Ah, right, this is for pid lookup not task->comm. Should have seen that..

Feel free to add:
Reviewed-by: David Herrmann <dh.herrmann at gmail.com>

Thanks
David