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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> > ---
> > 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.
> > + 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@xxxxxxxxxxxxxxxxxxx>
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.
> Anyway, patch looks good otherwise. Especially task->comm sounds
> really handy in that list.
It was. Thanks for the feedback, will add the extra header and comment.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
