[Mesa-dev] [PATCH 3/3] gallium/hud: protect against and initialization race

[Nicolai Hähnle]

On 21.10.2016 18:34, Eduardo Lima Mitev wrote:
> 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.

I actually think the code is fine as is, because gcpufreq_count is only 
set _once_ by hud_get_num_cpufreq while holding the lock. So if we find 
that gcpufreq_count != 0 while holding the lock, we have a guarantee 
that nobody will ever change that variable again.

Nicolai