| Message ID | 1407565350-13098-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Sat, Aug 9, 2014 at 8:22 AM, Chris Wilson <chris@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@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"? > + list_for_each_entry_reverse(priv, &dev->filelist, lhead) { No idea why you do backwards traversal, but ok.. > + 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? Anyway, patch looks good otherwise. Especially task->comm sounds really handy in that list. Thanks David > + task = pid_task(priv->pid, PIDTYPE_PID); > + seq_printf(m, "%20s %5d %3d %c %c %5d %10u\n", > + task ? task->comm : "<unknown>", > pid_vnr(priv->pid), > + priv->minor->index, > + priv->is_master ? 'y' : 'n', > + priv->authenticated ? 'y' : 'n', > from_kuid_munged(seq_user_ns(m), priv->uid), > priv->magic); > + rcu_read_unlock(); > } > mutex_unlock(&dev->struct_mutex); > return 0; > -- > 1.9.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
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@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@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. > > + 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@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. > 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
Hi On Mon, Sep 1, 2014 at 4:19 PM, Chris Wilson <chris@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@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@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@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@gmail.com> Thanks David
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"); + list_for_each_entry_reverse(priv, &dev->filelist, lhead) { + struct task_struct *task; + + rcu_read_lock(); + task = pid_task(priv->pid, PIDTYPE_PID); + seq_printf(m, "%20s %5d %3d %c %c %5d %10u\n", + task ? task->comm : "<unknown>", pid_vnr(priv->pid), + priv->minor->index, + priv->is_master ? 'y' : 'n', + priv->authenticated ? 'y' : 'n', from_kuid_munged(seq_user_ns(m), priv->uid), priv->magic); + rcu_read_unlock(); } mutex_unlock(&dev->struct_mutex); return 0;
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@chris-wilson.co.uk> --- drivers/gpu/drm/drm_info.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)