[Intel-gfx] [PATCH] drm: Include task-name and master status in debugfs clients info
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 ch...@chris-wilson.co.uk
Cc: David Herrmann dh.herrm...@gmail.com
Reviewed-by: David Herrmann dh.herrm...@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 devpiduid 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;
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Include task-name and master status in debugfs clients info
Hi
On Sat, Aug 9, 2014 at 8:22 AM, Chris Wilson ch...@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 ch...@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 devpiduid 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-de...@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Include task-name and master status in debugfs clients info
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 ch...@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 ch...@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 devpiduid 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-ker...@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
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Include task-name and master status in debugfs clients info
Hi
On Mon, Sep 1, 2014 at 4:19 PM, Chris Wilson ch...@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 ch...@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 ch...@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 devpiduid 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-ker...@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.herrm...@gmail.com
Thanks
David
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm: Include task-name and master status in debugfs clients info
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 ch...@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 devpiduid 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;
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
