[Mesa-dev] [PATCH 3/3] gallium/hud: protect against and initialization race
Eduardo Lima Mitev
elima at igalia.com
Fri Oct 21 16:34:19 UTC 2016
On 10/20/2016 08:45 PM, Steven Toth wrote:
> In the event that multiple threads attempt to install a graph
> concurrently, protect the shared list.
>
> Signed-off-by: Steven Toth <stoth at kernellabs.com>
> ---
> src/gallium/auxiliary/hud/hud_cpufreq.c | 12 ++++++++++--
> src/gallium/auxiliary/hud/hud_diskstat.c | 13 +++++++++++--
> src/gallium/auxiliary/hud/hud_nic.c | 12 ++++++++++--
> src/gallium/auxiliary/hud/hud_sensors_temp.c | 12 ++++++++++--
> 4 files changed, 41 insertions(+), 8 deletions(-)
>
> diff --git a/src/gallium/auxiliary/hud/hud_cpufreq.c b/src/gallium/auxiliary/hud/hud_cpufreq.c
> index e66c3e4..19a6f08 100644
> --- a/src/gallium/auxiliary/hud/hud_cpufreq.c
> +++ b/src/gallium/auxiliary/hud/hud_cpufreq.c
> @@ -36,6 +36,7 @@
> #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 <stdio.h>
> #include <unistd.h>
> @@ -61,6 +62,7 @@ struct cpufreq_info
>
> static int gcpufreq_count = 0;
> static struct list_head gcpufreq_list;
> +pipe_static_mutex(gcpufreq_mutex);
>
> static struct cpufreq_info *
> find_cfi_by_index(int cpu_index, int mode)
> @@ -186,16 +188,21 @@ hud_get_num_cpufreq(bool displayhelp)
> int cpu_index;
>
> /* Return the number of CPU metrics we support. */
> - if (gcpufreq_count)
> + pipe_mutex_lock(gcpufreq_mutex);
> + if (gcpufreq_count) {
> + pipe_mutex_unlock(gcpufreq_mutex);
> return gcpufreq_count;
> + }
Notice that this won't return the correct value. The value of
'gcpufreq_count' might have changed to zero between the unlock and the
return. Maybe what you want is to save the value in a temporary variable
inside the protected region, then return that temporary. Something like:
int tmp_count;
pipe_mutex_lock(gcpufreq_mutex);
if (gcpufreq_count) {
tmp_count = gcpufreq_count;
pipe_mutex_unlock(gcpufreq_mutex);
return tmp_count;
}
Same comment applies to several other similar chunks in this patch.
>
> /* Scan /sys/devices.../cpu, for every object type we support, create
> * and persist an object to represent its different metrics.
> */
> list_inithead(&gcpufreq_list);
> DIR *dir = opendir("/sys/devices/system/cpu");
> - if (!dir)
> + if (!dir) {
> + pipe_mutex_unlock(gcpufreq_mutex);
> return 0;
> + }
>
> while ((dp = readdir(dir)) != NULL) {
>
> @@ -239,6 +246,7 @@ hud_get_num_cpufreq(bool displayhelp)
> }
> }
>
> + 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 5fada1f..f1cc2bb 100644
> --- a/src/gallium/auxiliary/hud/hud_diskstat.c
> +++ b/src/gallium/auxiliary/hud/hud_diskstat.c
> @@ -35,6 +35,7 @@
> #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 <stdio.h>
> #include <unistd.h>
> @@ -81,6 +82,7 @@ struct diskstat_info
> */
> static int gdiskstat_count = 0;
> static struct list_head gdiskstat_list;
> +pipe_static_mutex(gdiskstat_mutex);
>
> static struct diskstat_info *
> find_dsi_by_name(const char *n, int mode)
> @@ -244,16 +246,21 @@ hud_get_num_disks(bool displayhelp)
> char name[64];
>
> /* Return the number of block devices and partitions. */
> - if (gdiskstat_count)
> + pipe_mutex_lock(gdiskstat_mutex);
> + if (gdiskstat_count) {
> + pipe_mutex_unlock(gdiskstat_mutex);
> return gdiskstat_count;
> + }
>
> /* Scan /sys/block, for every object type we support, create and
> * persist an object to represent its different statistics.
> */
> list_inithead(&gdiskstat_list);
> DIR *dir = opendir("/sys/block/");
> - if (!dir)
> + if (!dir) {
> + pipe_mutex_unlock(gdiskstat_mutex);
> return 0;
> + }
>
> while ((dp = readdir(dir)) != NULL) {
>
> @@ -278,6 +285,7 @@ hud_get_num_disks(bool displayhelp)
> struct dirent *dpart;
> DIR *pdir = opendir(basename);
> if (!pdir) {
> + pipe_mutex_unlock(gdiskstat_mutex);
> close(dir);
> return 0;
> }
> @@ -312,6 +320,7 @@ hud_get_num_disks(bool displayhelp)
> puts(line);
> }
> }
> + pipe_mutex_unlock(gdiskstat_mutex);
>
> return gdiskstat_count;
> }
> diff --git a/src/gallium/auxiliary/hud/hud_nic.c b/src/gallium/auxiliary/hud/hud_nic.c
> index 2795c93..f9935de 100644
> --- a/src/gallium/auxiliary/hud/hud_nic.c
> +++ b/src/gallium/auxiliary/hud/hud_nic.c
> @@ -35,6 +35,7 @@
> #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 <stdio.h>
> #include <unistd.h>
> @@ -66,6 +67,7 @@ struct nic_info
> */
> static int gnic_count = 0;
> static struct list_head gnic_list;
> +pipe_static_mutex(gnic_mutex);
>
> static struct nic_info *
> find_nic_by_name(const char *n, int mode)
> @@ -329,16 +331,21 @@ hud_get_num_nics(bool displayhelp)
> char name[64];
>
> /* Return the number if network interfaces. */
> - if (gnic_count)
> + pipe_mutex_lock(gnic_mutex);
> + if (gnic_count) {
> + pipe_mutex_unlock(gnic_mutex);
> return gnic_count;
> + }
>
> /* Scan /sys/block, for every object type we support, create and
> * persist an object to represent its different statistics.
> */
> list_inithead(&gnic_list);
> DIR *dir = opendir("/sys/class/net/");
> - if (!dir)
> + if (!dir) {
> + pipe_mutex_unlock(gnic_mutex);
> return 0;
> + }
>
> while ((dp = readdir(dir)) != NULL) {
>
> @@ -412,6 +419,7 @@ hud_get_num_nics(bool displayhelp)
>
> }
>
> + pipe_mutex_unlock(gnic_mutex);
> return gnic_count;
> }
>
> diff --git a/src/gallium/auxiliary/hud/hud_sensors_temp.c b/src/gallium/auxiliary/hud/hud_sensors_temp.c
> index 4a8a4fc..11b8a4c 100644
> --- a/src/gallium/auxiliary/hud/hud_sensors_temp.c
> +++ b/src/gallium/auxiliary/hud/hud_sensors_temp.c
> @@ -32,6 +32,7 @@
> #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 <stdio.h>
> #include <unistd.h>
> @@ -49,6 +50,7 @@
> */
> static int gsensors_temp_count = 0;
> static struct list_head gsensors_temp_list;
> +pipe_static_mutex(gsensor_temp_mutex);
>
> struct sensors_temp_info
> {
> @@ -322,12 +324,17 @@ int
> hud_get_num_sensors(bool displayhelp)
> {
> /* Return the number of sensors detected. */
> - if (gsensors_temp_count)
> + pipe_mutex_lock(gsensor_temp_mutex);
> + if (gsensors_temp_count) {
> + pipe_mutex_unlock(gsensor_temp_mutex);
> return gsensors_temp_count;
> + }
>
> int ret = sensors_init(NULL);
> - if (ret)
> + if (ret) {
> + pipe_mutex_unlock(gsensor_temp_mutex);
> return 0;
> + }
>
> list_inithead(&gsensors_temp_list);
>
> @@ -361,6 +368,7 @@ hud_get_num_sensors(bool displayhelp)
> }
> }
>
> + pipe_mutex_unlock(gsensor_temp_mutex);
> return gsensors_temp_count;
> }
>
>
More information about the mesa-dev
mailing list