Re: [Mesa-dev] [PATCH] gallium/hud: Sensor extension is segfaulting.

[Brian Paul]

For the subject line we usually say what we're fixing.  I'd suggest 
something like "gallium/hud: fix segfault in sensor extension code".


-Brian

On 10/13/2016 11:29 AM, Steven Toth wrote:

Round two of the patchset, incorporating feedback
from nhaeh...@gmail.com

The fixes in this set address bugfix #68169, HUD crashing
when testing with unigine (heaven).

The bug also manifested itself as a lack of data in
HUD charts when multiple instanced were created and
destroyed in a specific order, a use case not witnessed
with glxgears.

These patches:
1. mutex protect the internal object lists.
2. reference count object access for destruction purposes.

Patchset tested with glxgears/demo and unigine.

Signed-off-by: Steven Toth 



___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] gallium/hud: Sensor extension is segfaulting.

[Nicolai Hähnle]

On 13.10.2016 19:29, Steven Toth wrote:

Round two of the patchset, incorporating feedback
from nhaeh...@gmail.com

The fixes in this set address bugfix #68169, HUD crashing
when testing with unigine (heaven).

The bug also manifested itself as a lack of data in
HUD charts when multiple instanced were created and
destroyed in a specific order, a use case not witnessed
with glxgears.

These patches:
1. mutex protect the internal object lists.
2. reference count object access for destruction purposes.

Patchset tested with glxgears/demo and unigine.

Signed-off-by: Steven Toth 
---
 src/gallium/auxiliary/hud/hud_cpufreq.c  | 33 ++
 src/gallium/auxiliary/hud/hud_diskstat.c | 39 ++
 src/gallium/auxiliary/hud/hud_nic.c  | 37 +
 src/gallium/auxiliary/hud/hud_sensors_temp.c | 41 ++--
 4 files changed, 125 insertions(+), 25 deletions(-)

diff --git a/src/gallium/auxiliary/hud/hud_cpufreq.c 
b/src/gallium/auxiliary/hud/hud_cpufreq.c
index 4501bbb..e193568 100644
--- a/src/gallium/auxiliary/hud/hud_cpufreq.c
+++ b/src/gallium/auxiliary/hud/hud_cpufreq.c
@@ -37,6 +37,8 @@
 #include "util/list.h"
 #include "os/os_time.h"
 #include "util/u_memory.h"
+#include "util/u_inlines.h"
+#include "os/os_thread.h"
 #include 
 #include 
 #include 
@@ -57,21 +59,28 @@ struct cpufreq_info
char sysfs_filename[128];
uint64_t KHz;
uint64_t last_time;
+   struct pipe_reference refcnt;
 };

 static int gcpufreq_count = 0;
 static struct list_head gcpufreq_list;
+static pipe_mutex gcpufreq_mutex;

 static struct cpufreq_info *
 find_cfi_by_index(int cpu_index, int mode)
 {
+   struct cpufreq_info *ret = 0;
+   pipe_mutex_lock(gcpufreq_mutex);
list_for_each_entry(struct cpufreq_info, cfi, _list, list) {
   if (cfi->mode != mode)
  continue;
-  if (cfi->cpu_index == cpu_index)
- return cfi;
+  if (cfi->cpu_index == cpu_index) {
+ ret = cfi;
+ break;
+  }
}
-   return 0;
+   pipe_mutex_unlock(gcpufreq_mutex);
+   return ret;
 }

 static int
@@ -116,8 +125,14 @@ static void
 free_query_data(void *p)
 {
struct cpufreq_info *cfi = (struct cpufreq_info *)p;
-   list_del(>list);
-   FREE(cfi);
+   /* atomic dec */
+   if (pipe_reference(>refcnt, NULL)) {
+  pipe_mutex_lock(gcpufreq_mutex);
+  list_del(>list);
+  gcpufreq_count--;
+  pipe_mutex_unlock(gcpufreq_mutex);
+  FREE(cfi);
+   }
 }

 /**
@@ -159,6 +174,7 @@ hud_cpufreq_graph_install(struct hud_pane *pane, int 
cpu_index,
   return;
}

+   pipe_reference(NULL, >refcnt); /* atomic inc */


There is a race condition here where another thread frees cfi between 
the time that you took it from the linked list and this reference count 
increase.


Anyway, I'm sorry for not having looked more closely before, but the 
whole logic of the cpufreq_info life cycle feels a bit off to me right now.


It looks like you're trying to do a lazy one-time initialization of all 
cpufreq_info objects that are ever required in hud_get_num_cpufreq. 
That's fine as a pattern, but then you simply shouldn't ever free those 
objects in free_query_data.


You'd keep them around until program exit; they will appear in a leak 
check as "still reachable" (and that only if the HUD was used), which is 
fine in my book.


Then the only thing you need to mutex-protect is the initialization, to 
ensure that it happens exactly once and that nobody tries to 
find_cfi_index until the list is initialized.




gr->query_data = cfi;
gr->query_new_value = query_cfi_load;

@@ -180,8 +196,12 @@ add_object(const char *name, const char *fn, int objmode, 
int cpu_index)
strcpy(cfi->sysfs_filename, fn);
cfi->mode = objmode;
cfi->cpu_index = cpu_index;
+   pipe_reference_init(>refcnt, 1);
+
+   pipe_mutex_lock(gcpufreq_mutex);
list_addtail(>list, _list);
gcpufreq_count++;
+   pipe_mutex_unlock(gcpufreq_mutex);
 }

 /**
@@ -205,6 +225,7 @@ hud_get_num_cpufreq(bool displayhelp)
/* Scan /sys/devices.../cpu, for every object type we support, create
 * and persist an object to represent its different metrics.
 */
+   pipe_mutex_init(gcpufreq_mutex);
list_inithead(_list);
DIR *dir = opendir("/sys/devices/system/cpu");


There's a closedir missing.

I'm going to skip the other files because I believe they follow a 
similar pattern. Just one more thing: as it stands, the query_*_load 
functions will also go wonky when multiple contexts are actually used to 
show a HUD at the same time. I don't think it hurts the stability of the 
program, but it might lead to confusing output when the values of e.g. 
dsi->last_stat and dsi->last_time will interfere between multiple HUD 
contexts.


Nicolai
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] gallium/hud: Sensor extension is segfaulting.

[Steven Toth]

Round two of the patchset, incorporating feedback
from nhaeh...@gmail.com

The fixes in this set address bugfix #68169, HUD crashing
when testing with unigine (heaven).

The bug also manifested itself as a lack of data in
HUD charts when multiple instanced were created and
destroyed in a specific order, a use case not witnessed
with glxgears.

These patches:
1. mutex protect the internal object lists.
2. reference count object access for destruction purposes.

Patchset tested with glxgears/demo and unigine.

Signed-off-by: Steven Toth 
---
 src/gallium/auxiliary/hud/hud_cpufreq.c  | 33 ++
 src/gallium/auxiliary/hud/hud_diskstat.c | 39 ++
 src/gallium/auxiliary/hud/hud_nic.c  | 37 +
 src/gallium/auxiliary/hud/hud_sensors_temp.c | 41 ++--
 4 files changed, 125 insertions(+), 25 deletions(-)

diff --git a/src/gallium/auxiliary/hud/hud_cpufreq.c 
b/src/gallium/auxiliary/hud/hud_cpufreq.c
index 4501bbb..e193568 100644
--- a/src/gallium/auxiliary/hud/hud_cpufreq.c
+++ b/src/gallium/auxiliary/hud/hud_cpufreq.c
@@ -37,6 +37,8 @@
 #include "util/list.h"
 #include "os/os_time.h"
 #include "util/u_memory.h"
+#include "util/u_inlines.h"
+#include "os/os_thread.h"
 #include 
 #include 
 #include 
@@ -57,21 +59,28 @@ struct cpufreq_info
char sysfs_filename[128];
uint64_t KHz;
uint64_t last_time;
+   struct pipe_reference refcnt;
 };
 
 static int gcpufreq_count = 0;
 static struct list_head gcpufreq_list;
+static pipe_mutex gcpufreq_mutex;
 
 static struct cpufreq_info *
 find_cfi_by_index(int cpu_index, int mode)
 {
+   struct cpufreq_info *ret = 0;
+   pipe_mutex_lock(gcpufreq_mutex);
list_for_each_entry(struct cpufreq_info, cfi, _list, list) {
   if (cfi->mode != mode)
  continue;
-  if (cfi->cpu_index == cpu_index)
- return cfi;
+  if (cfi->cpu_index == cpu_index) {
+ ret = cfi;
+ break;
+  }
}
-   return 0;
+   pipe_mutex_unlock(gcpufreq_mutex);
+   return ret;
 }
 
 static int
@@ -116,8 +125,14 @@ static void
 free_query_data(void *p)
 {
struct cpufreq_info *cfi = (struct cpufreq_info *)p;
-   list_del(>list);
-   FREE(cfi);
+   /* atomic dec */
+   if (pipe_reference(>refcnt, NULL)) {
+  pipe_mutex_lock(gcpufreq_mutex);
+  list_del(>list);
+  gcpufreq_count--;
+  pipe_mutex_unlock(gcpufreq_mutex);
+  FREE(cfi);
+   }
 }
 
 /**
@@ -159,6 +174,7 @@ hud_cpufreq_graph_install(struct hud_pane *pane, int 
cpu_index,
   return;
}
 
+   pipe_reference(NULL, >refcnt); /* atomic inc */
gr->query_data = cfi;
gr->query_new_value = query_cfi_load;
 
@@ -180,8 +196,12 @@ add_object(const char *name, const char *fn, int objmode, 
int cpu_index)
strcpy(cfi->sysfs_filename, fn);
cfi->mode = objmode;
cfi->cpu_index = cpu_index;
+   pipe_reference_init(>refcnt, 1);
+
+   pipe_mutex_lock(gcpufreq_mutex);
list_addtail(>list, _list);
gcpufreq_count++;
+   pipe_mutex_unlock(gcpufreq_mutex);
 }
 
 /**
@@ -205,6 +225,7 @@ hud_get_num_cpufreq(bool displayhelp)
/* Scan /sys/devices.../cpu, for every object type we support, create
 * and persist an object to represent its different metrics.
 */
+   pipe_mutex_init(gcpufreq_mutex);
list_inithead(_list);
DIR *dir = opendir("/sys/devices/system/cpu");
if (!dir)
@@ -240,6 +261,7 @@ hud_get_num_cpufreq(bool displayhelp)
}
 
if (displayhelp) {
+  pipe_mutex_lock(gcpufreq_mutex);
   list_for_each_entry(struct cpufreq_info, cfi, _list, list) {
  char line[128];
  snprintf(line, sizeof(line), "cpufreq-%s-%s",
@@ -249,6 +271,7 @@ hud_get_num_cpufreq(bool displayhelp)
 
  puts(line);
   }
+  pipe_mutex_unlock(gcpufreq_mutex);
}
 
return gcpufreq_count;
diff --git a/src/gallium/auxiliary/hud/hud_diskstat.c 
b/src/gallium/auxiliary/hud/hud_diskstat.c
index b248baf..e1748c5 100644
--- a/src/gallium/auxiliary/hud/hud_diskstat.c
+++ b/src/gallium/auxiliary/hud/hud_diskstat.c
@@ -35,7 +35,9 @@
 #include "hud/hud_private.h"
 #include "util/list.h"
 #include "os/os_time.h"
+#include "os/os_thread.h"
 #include "util/u_memory.h"
+#include "util/u_inlines.h"
 #include 
 #include 
 #include 
@@ -73,6 +75,7 @@ struct diskstat_info
char sysfs_filename[128];
uint64_t last_time;
struct stat_s last_stat;
+   struct pipe_reference refcnt;
 };
 
 /* TODO: We don't handle dynamic block device / partition
@@ -81,17 +84,23 @@ struct diskstat_info
  */
 static int gdiskstat_count = 0;
 static struct list_head gdiskstat_list;
+static pipe_mutex gdiskstat_mutex;
 
 static struct diskstat_info *
 find_dsi_by_name(const char *n, int mode)
 {
+   struct diskstat_info *ret = 0;
+   pipe_mutex_lock(gdiskstat_mutex);
list_for_each_entry(struct diskstat_info, dsi, _list, list) {
   if (dsi->mode != mode)
  continue;
-  if (strcasecmp(dsi->name, n) == 0)
-