In drm_info.c: drm_bufs_info() and drm_vm_info() two seq_printf() was done unnecessarily while locking a mutex. This patch flips the order of the print and lock/unlock where applicable.
Signed-off-by: Jonas Lundqvist jonas@gannon.se --- drivers/gpu/drm/drm_info.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c index 51efebd..981afe7 100644 --- a/drivers/gpu/drm/drm_info.c +++ b/drivers/gpu/drm/drm_info.c @@ -81,11 +81,10 @@ int drm_vm_info(struct seq_file *m, void *data) _DRM_SCATTER_GATHER and _DRM_CONSISTENT */ const char *types[] = { "FB", "REG", "SHM", "AGP", "SG", "PCI" }; const char *type; - int i; + int i = 0;
- mutex_lock(&dev->struct_mutex); seq_printf(m, "slot offset size type flags address mtrr\n\n"); - i = 0; + mutex_lock(&dev->struct_mutex); list_for_each_entry(r_list, &dev->maplist, head) { map = r_list->map; if (!map) @@ -147,8 +146,8 @@ int drm_bufs_info(struct seq_file *m, void *data) seq_printf(m, "\n"); seq_printf(m, " %d", dma->buflist[i]->list); } - seq_printf(m, "\n"); mutex_unlock(&dev->struct_mutex); + seq_printf(m, "\n"); return 0; }
Jonas,
On Tue, Dec 30, 2014 at 10:54:26PM +0100, Jonas Lundqvist wrote:
In drm_info.c: drm_bufs_info() and drm_vm_info() two seq_printf() was done unnecessarily while locking a mutex. This patch flips the order of the print and lock/unlock where applicable.
Signed-off-by: Jonas Lundqvist jonas@gannon.se
drivers/gpu/drm/drm_info.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c index 51efebd..981afe7 100644 --- a/drivers/gpu/drm/drm_info.c +++ b/drivers/gpu/drm/drm_info.c @@ -81,11 +81,10 @@ int drm_vm_info(struct seq_file *m, void *data) _DRM_SCATTER_GATHER and _DRM_CONSISTENT */ const char *types[] = { "FB", "REG", "SHM", "AGP", "SG", "PCI" }; const char *type;
- int i;
- int i = 0;
- mutex_lock(&dev->struct_mutex); seq_printf(m, "slot offset size type flags address mtrr\n\n");
- i = 0;
- mutex_lock(&dev->struct_mutex); list_for_each_entry(r_list, &dev->maplist, head) { map = r_list->map; if (!map)
@@ -147,8 +146,8 @@ int drm_bufs_info(struct seq_file *m, void *data) seq_printf(m, "\n"); seq_printf(m, " %d", dma->buflist[i]->list); }
- seq_printf(m, "\n"); mutex_unlock(&dev->struct_mutex);
- seq_printf(m, "\n"); return 0;
}
You changed 'i' but you didn't explain in your log message why you did this.
Does this change really improve anything? It may work the same with the locks moved around. But if you look at the function as a whole, the locks encapsulate the body of this function nicely. I like the original design better.
-- 2.1.4
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Hi Jeremiah,
On 12/30/2014 11:52 PM, Jeremiah Mahler wrote:
You changed 'i' but you didn't explain in your log message why you did this.
I can change the commit message to something more generic. "Move code outside of locked mutex" or similar.
Does this change really improve anything? It may work the same with the locks moved around. But if you look at the function as a whole, the locks encapsulate the body of this function nicely. I like the original design better.
The locking was already done this way, ie after the seq_printf, in the functions drm_clients_info() and drm_gem_name_info() in thr same file. So this change is really more of an alignment.
Best regards Jonas
Jonas,
On Wed, Dec 31, 2014 at 09:55:19AM +0100, Jonas Lundqvist wrote:
Hi Jeremiah,
On 12/30/2014 11:52 PM, Jeremiah Mahler wrote:
You changed 'i' but you didn't explain in your log message why you did this.
I can change the commit message to something more generic. "Move code outside of locked mutex" or similar.
That still doesn't explain why you changed the 'i' variable.
Does this change really improve anything? It may work the same with the locks moved around. But if you look at the function as a whole, the locks encapsulate the body of this function nicely. I like the original design better.
The locking was already done this way, ie after the seq_printf, in the functions drm_clients_info() and drm_gem_name_info() in thr same file. So this change is really more of an alignment.
Your right, those two have have the lock after the seq_printf. But the drm_bufs_info() function has its lock before the seq_printf. So before your change about half are one way and half are the other.
I am still not convinced that either of these ways is better or makes any difference whatsoever.
Best regards Jonas
dri-devel@lists.freedesktop.org
-
Jeremiah Mahler -
Jonas Lundqvist