Re: [Mesa-dev] [PATCH] gallium/hud: bugfix: 68169 - Sensor extensions segfaults.
Hello Nicolai >> On inspection, this was due to the 'unigine' application >> created the HUD contexts twice (two screens?). The >> extensions had never been tested in that configuration, >> and the free_query_data() calls were thus trigger. > > > The HUD is actually created per-context. Two general points that apply > equally to all four parts of the patch: Yeah. I didn;t realize this as none of the default use cases ever exercised this approach. Now that we have one, I'm happy to adjust it. > > Instead of open-coding the users variable, I'd prefer that you use the > pipe_reference mechanism. Agreed, done. It would have been nice if a macro was global created for pip_reference_inc/dec(obj), instead of having to call (NULL, obj) and (obj, NULL), and read the code for the implementation, but this does not matter at this point. > > Contexts can be created and destroyed simultaneously from multiple threads. In which case yes, we need some exclusivity, I've implemented this with pipe_mutexes. > You probably need to protect some of the query functions with a mutex as > well. The general rule of thumb is that functions associated to a Yep. Done. > Thanks for looking into this! Thank you for the review. I should have a patch available shortly. -- Steven Toth - Kernel Labs http://www.kernellabs.com ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] gallium/hud: bugfix: 68169 - Sensor extensions segfaults.
Hi Steven, On 12 October 2016 at 18:02, Steven Toth wrote: > https://bugs.freedesktop.org/show_bug.cgi?id=98169 > Just a general comment/suggestion: Don't include bug# in the commit summary. Having it as Bugzilla: https... towards the end of the description. Aside: if you know the commit that is getting fixed, you can also add a kernel-like Fixes: tag. -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] gallium/hud: bugfix: 68169 - Sensor extensions segfaults.
On 12.10.2016 19:02, Steven Toth wrote:
https://bugs.freedesktop.org/show_bug.cgi?id=98169
Really a two part bug.
1. The recent extensions to the HUB framework were tested exclusively
with glxgears/demo/head. None of these tools exercise the
free_query_data() code path. When these codepaths were
tested, thanks to Christoph, using heaven-unigine
application, the sensors extension would segfault.
2. In a second case, same use case, extensions would
return zero values for cpufrequency, network stats etc.
On inspection, this was due to the 'unigine' application
created the HUD contexts twice (two screens?). The
extensions had never been tested in that configuration,
and the free_query_data() calls were thus trigger.
The HUD is actually created per-context. Two general points that apply
equally to all four parts of the patch:
Instead of open-coding the users variable, I'd prefer that you use the
pipe_reference mechanism. This mechanism has the advantage that it
already works in a multi-threaded setting, which brings me to the second
point, which is:
Contexts can be created and destroyed simultaneously from multiple
threads. You need to protect at least the globals with a mutex, i.e. in
the find_*_by_name and free_query_data.
You probably need to protect some of the query functions with a mutex as
well. The general rule of thumb is that functions associated to a
pipe_context can only be called from one thread at a time; i.e., the
graph's callback function (like query_dsi_load) is only called from one
thread at a time _for one particular graph_. However, since you're
sharing the underlying data between graphs in multiple contexts, you
need a mutex e.g. in query_dsi_load when accessing the dsi members.
At least you need to think through the implications of what happens here...
It may actually be cleaner and simpler to just get rid of the sharing of
underlying structures.
Thanks for looking into this!
Cheers,
Nicolai
This patchset ensures that globally allocated
mechanisms for collecting and exposing various
disk, network, cpu frequency and sensors statistics
are retained correctly across multiple uses, and
free'd when no more callers require them.
Patchset was regression tested again with Unigine,
glxgears and glxdemo.
Tested-By: Christoph Haag
Signed-off-by: Steven Toth
---
src/gallium/auxiliary/hud/hud_cpufreq.c | 8 ++--
src/gallium/auxiliary/hud/hud_diskstat.c | 10 +++---
src/gallium/auxiliary/hud/hud_nic.c | 8 ++--
src/gallium/auxiliary/hud/hud_sensors_temp.c | 18 +-
4 files changed, 32 insertions(+), 12 deletions(-)
diff --git a/src/gallium/auxiliary/hud/hud_cpufreq.c
b/src/gallium/auxiliary/hud/hud_cpufreq.c
index 4501bbb..39bdcb0 100644
--- a/src/gallium/auxiliary/hud/hud_cpufreq.c
+++ b/src/gallium/auxiliary/hud/hud_cpufreq.c
@@ -57,6 +57,7 @@ struct cpufreq_info
char sysfs_filename[128];
uint64_t KHz;
uint64_t last_time;
+ int users;
};
static int gcpufreq_count = 0;
@@ -116,8 +117,10 @@ static void
free_query_data(void *p)
{
struct cpufreq_info *cfi = (struct cpufreq_info *)p;
- list_del(&cfi->list);
- FREE(cfi);
+ if (--cfi->users == 0) {
+ list_del(&cfi->list);
+ FREE(cfi);
+ }
}
/**
@@ -159,6 +162,7 @@ hud_cpufreq_graph_install(struct hud_pane *pane, int
cpu_index,
return;
}
+ cfi->users++;
gr->query_data = cfi;
gr->query_new_value = query_cfi_load;
diff --git a/src/gallium/auxiliary/hud/hud_diskstat.c
b/src/gallium/auxiliary/hud/hud_diskstat.c
index b248baf..000d6b3 100644
--- a/src/gallium/auxiliary/hud/hud_diskstat.c
+++ b/src/gallium/auxiliary/hud/hud_diskstat.c
@@ -73,6 +73,7 @@ struct diskstat_info
char sysfs_filename[128];
uint64_t last_time;
struct stat_s last_stat;
+ int users;
};
/* TODO: We don't handle dynamic block device / partition
@@ -165,9 +166,11 @@ query_dsi_load(struct hud_graph *gr)
static void
free_query_data(void *p)
{
- struct diskstat_info *nic = (struct diskstat_info *) p;
- list_del(&nic->list);
- FREE(nic);
+ struct diskstat_info *dsi = (struct diskstat_info *) p;
+ if (--dsi->users == 0) {
+ list_del(&dsi->list);
+ FREE(dsi);
+ }
}
/**
@@ -205,6 +208,7 @@ hud_diskstat_graph_install(struct hud_pane *pane, const
char *dev_name,
else
return;
+ dsi->users++;
gr->query_data = dsi;
gr->query_new_value = query_dsi_load;
diff --git a/src/gallium/auxiliary/hud/hud_nic.c
b/src/gallium/auxiliary/hud/hud_nic.c
index fb6b8c0..e3f5910 100644
--- a/src/gallium/auxiliary/hud/hud_nic.c
+++ b/src/gallium/auxiliary/hud/hud_nic.c
@@ -59,6 +59,7 @@ struct nic_info
char throughput_filename[128];
uint64_t last_time;
uint64_t last_nic_bytes;
+ int users;
};
/* TODO: We don't handle dynamic NIC arrival or removal.
@@ -238,8 +239,10 @@ static void
free_query_data(void *p)
{
struct nic_info *nic = (struct nic_info *) p;
- list_del(&nic->list);
-
[Mesa-dev] [PATCH] gallium/hud: bugfix: 68169 - Sensor extensions segfaults.
https://bugs.freedesktop.org/show_bug.cgi?id=98169
Really a two part bug.
1. The recent extensions to the HUB framework were tested exclusively
with glxgears/demo/head. None of these tools exercise the
free_query_data() code path. When these codepaths were
tested, thanks to Christoph, using heaven-unigine
application, the sensors extension would segfault.
2. In a second case, same use case, extensions would
return zero values for cpufrequency, network stats etc.
On inspection, this was due to the 'unigine' application
created the HUD contexts twice (two screens?). The
extensions had never been tested in that configuration,
and the free_query_data() calls were thus trigger.
This patchset ensures that globally allocated
mechanisms for collecting and exposing various
disk, network, cpu frequency and sensors statistics
are retained correctly across multiple uses, and
free'd when no more callers require them.
Patchset was regression tested again with Unigine,
glxgears and glxdemo.
Tested-By: Christoph Haag
Signed-off-by: Steven Toth
---
src/gallium/auxiliary/hud/hud_cpufreq.c | 8 ++--
src/gallium/auxiliary/hud/hud_diskstat.c | 10 +++---
src/gallium/auxiliary/hud/hud_nic.c | 8 ++--
src/gallium/auxiliary/hud/hud_sensors_temp.c | 18 +-
4 files changed, 32 insertions(+), 12 deletions(-)
diff --git a/src/gallium/auxiliary/hud/hud_cpufreq.c
b/src/gallium/auxiliary/hud/hud_cpufreq.c
index 4501bbb..39bdcb0 100644
--- a/src/gallium/auxiliary/hud/hud_cpufreq.c
+++ b/src/gallium/auxiliary/hud/hud_cpufreq.c
@@ -57,6 +57,7 @@ struct cpufreq_info
char sysfs_filename[128];
uint64_t KHz;
uint64_t last_time;
+ int users;
};
static int gcpufreq_count = 0;
@@ -116,8 +117,10 @@ static void
free_query_data(void *p)
{
struct cpufreq_info *cfi = (struct cpufreq_info *)p;
- list_del(&cfi->list);
- FREE(cfi);
+ if (--cfi->users == 0) {
+ list_del(&cfi->list);
+ FREE(cfi);
+ }
}
/**
@@ -159,6 +162,7 @@ hud_cpufreq_graph_install(struct hud_pane *pane, int
cpu_index,
return;
}
+ cfi->users++;
gr->query_data = cfi;
gr->query_new_value = query_cfi_load;
diff --git a/src/gallium/auxiliary/hud/hud_diskstat.c
b/src/gallium/auxiliary/hud/hud_diskstat.c
index b248baf..000d6b3 100644
--- a/src/gallium/auxiliary/hud/hud_diskstat.c
+++ b/src/gallium/auxiliary/hud/hud_diskstat.c
@@ -73,6 +73,7 @@ struct diskstat_info
char sysfs_filename[128];
uint64_t last_time;
struct stat_s last_stat;
+ int users;
};
/* TODO: We don't handle dynamic block device / partition
@@ -165,9 +166,11 @@ query_dsi_load(struct hud_graph *gr)
static void
free_query_data(void *p)
{
- struct diskstat_info *nic = (struct diskstat_info *) p;
- list_del(&nic->list);
- FREE(nic);
+ struct diskstat_info *dsi = (struct diskstat_info *) p;
+ if (--dsi->users == 0) {
+ list_del(&dsi->list);
+ FREE(dsi);
+ }
}
/**
@@ -205,6 +208,7 @@ hud_diskstat_graph_install(struct hud_pane *pane, const
char *dev_name,
else
return;
+ dsi->users++;
gr->query_data = dsi;
gr->query_new_value = query_dsi_load;
diff --git a/src/gallium/auxiliary/hud/hud_nic.c
b/src/gallium/auxiliary/hud/hud_nic.c
index fb6b8c0..e3f5910 100644
--- a/src/gallium/auxiliary/hud/hud_nic.c
+++ b/src/gallium/auxiliary/hud/hud_nic.c
@@ -59,6 +59,7 @@ struct nic_info
char throughput_filename[128];
uint64_t last_time;
uint64_t last_nic_bytes;
+ int users;
};
/* TODO: We don't handle dynamic NIC arrival or removal.
@@ -238,8 +239,10 @@ static void
free_query_data(void *p)
{
struct nic_info *nic = (struct nic_info *) p;
- list_del(&nic->list);
- FREE(nic);
+ if (--nic->users == 0) {
+ list_del(&nic->list);
+ FREE(nic);
+ }
}
/**
@@ -281,6 +284,7 @@ hud_nic_graph_install(struct hud_pane *pane, const char
*nic_name,
else
return;
+ nic->users++;
gr->query_data = nic;
gr->query_new_value = query_nic_load;
diff --git a/src/gallium/auxiliary/hud/hud_sensors_temp.c
b/src/gallium/auxiliary/hud/hud_sensors_temp.c
index e41b847..c6f7d5b 100644
--- a/src/gallium/auxiliary/hud/hud_sensors_temp.c
+++ b/src/gallium/auxiliary/hud/hud_sensors_temp.c
@@ -68,6 +68,7 @@ struct sensors_temp_info
sensors_chip_name *chip;
const sensors_feature *feature;
double current, min, max, critical;
+ int users;
};
static double
@@ -193,11 +194,17 @@ static void
free_query_data(void *p)
{
struct sensors_temp_info *sti = (struct sensors_temp_info *) p;
- list_del(&sti->list);
- if (sti->chip)
- sensors_free_chip_name(sti->chip);
- FREE(sti);
- sensors_cleanup();
+ if (--sti->users == 0) {
+ list_del(&sti->list);
+ gsensors_temp_count--;
+ if (sti->chip)
+ sensors_free_chip_name(sti->chip);
+ FREE(sti);
+ }
+
+ /* Destroy sensor singleton when after all refs released. */
+ if (!gsensors_temp_count)
+ sensors_clean
