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"); + 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;
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
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.
v2: Add the command column header and flesh out a couple of comments. (David Herrmann)
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: David Herrmann dh.herrmann@gmail.com Reviewed-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/drm_info.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c index 15ec9f471ece..36aba980ea8b 100644 --- a/drivers/gpu/drm/drm_info.c +++ b/drivers/gpu/drm/drm_info.c @@ -183,15 +183,32 @@ int drm_clients_info(struct seq_file *m, void *data) struct drm_device *dev = node->minor->dev; struct drm_file *priv;
+ seq_printf(m, + "%20s %5s %3s master a %5s %10s\n", + "command", + "pid", + "dev", + "uid", + "magic"); + + /* dev->filelist is sorted youngest first, but we want to present + * oldest first (i.e. kernel, servers, clients), so walk backwardss. + */ 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, + list_for_each_entry_reverse(priv, &dev->filelist, lhead) { + struct task_struct *task; + + rcu_read_lock(); /* locks pid_task()->comm */ + 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;
dri-devel@lists.freedesktop.org
-
Chris Wilson -
David Herrmann