[Mesa-dev] [PATCH 1/3] gallium/hud: fix a problem where objects are free'd while in use.

[Nicolai Hähnle]

Thank you for taking care of this! I have one comment on patch #2, and I 
also agree with Eduardo's comment there. Apart from that, the series is

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>

On 20.10.2016 20:45, Steven Toth wrote:
> Instead of trying to maintain a reference counted list of valid HUD
> objects, and freeing them accordingly, creating race conditions
> between unanticipated multiple threads, simply accept they're
> allocated once and never released until the process terminates.
>
> They're a shared resource between multiple threads, so accept
> they're always available for use.
>
> Signed-off-by: Steven Toth <stoth at kernellabs.com>
> ---
>  src/gallium/auxiliary/hud/hud_cpufreq.c      | 13 -------------
>  src/gallium/auxiliary/hud/hud_diskstat.c     | 13 -------------
>  src/gallium/auxiliary/hud/hud_nic.c          | 13 -------------
>  src/gallium/auxiliary/hud/hud_sensors_temp.c | 16 ----------------
>  4 files changed, 55 deletions(-)
>
> diff --git a/src/gallium/auxiliary/hud/hud_cpufreq.c b/src/gallium/auxiliary/hud/hud_cpufreq.c
> index 4501bbb..bfc748b 100644
> --- a/src/gallium/auxiliary/hud/hud_cpufreq.c
> +++ b/src/gallium/auxiliary/hud/hud_cpufreq.c
> @@ -112,14 +112,6 @@ query_cfi_load(struct hud_graph *gr)
>     }
>  }
>
> -static void
> -free_query_data(void *p)
> -{
> -   struct cpufreq_info *cfi = (struct cpufreq_info *)p;
> -   list_del(&cfi->list);
> -   FREE(cfi);
> -}
> -
>  /**
>    * Create and initialize a new object for a specific CPU.
>    * \param  pane  parent context.
> @@ -162,11 +154,6 @@ hud_cpufreq_graph_install(struct hud_pane *pane, int cpu_index,
>     gr->query_data = cfi;
>     gr->query_new_value = query_cfi_load;
>
> -   /* Don't use free() as our callback as that messes up Gallium's
> -    * memory debugger.  Use simple free_query_data() wrapper.
> -    */
> -   gr->free_query_data = free_query_data;
> -
>     hud_pane_add_graph(pane, gr);
>     hud_pane_set_max_value(pane, 3000000 /* 3 GHz */);
>  }
> diff --git a/src/gallium/auxiliary/hud/hud_diskstat.c b/src/gallium/auxiliary/hud/hud_diskstat.c
> index b248baf..7d4f500 100644
> --- a/src/gallium/auxiliary/hud/hud_diskstat.c
> +++ b/src/gallium/auxiliary/hud/hud_diskstat.c
> @@ -162,14 +162,6 @@ 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);
> -}
> -
>  /**
>    * Create and initialize a new object for a specific block I/O device.
>    * \param  pane  parent context.
> @@ -208,11 +200,6 @@ hud_diskstat_graph_install(struct hud_pane *pane, const char *dev_name,
>     gr->query_data = dsi;
>     gr->query_new_value = query_dsi_load;
>
> -   /* Don't use free() as our callback as that messes up Gallium's
> -    * memory debugger.  Use simple free_query_data() wrapper.
> -    */
> -   gr->free_query_data = free_query_data;
> -
>     hud_pane_add_graph(pane, gr);
>     hud_pane_set_max_value(pane, 100);
>  }
> diff --git a/src/gallium/auxiliary/hud/hud_nic.c b/src/gallium/auxiliary/hud/hud_nic.c
> index fb6b8c0..719dd04 100644
> --- a/src/gallium/auxiliary/hud/hud_nic.c
> +++ b/src/gallium/auxiliary/hud/hud_nic.c
> @@ -234,14 +234,6 @@ query_nic_load(struct hud_graph *gr)
>     }
>  }
>
> -static void
> -free_query_data(void *p)
> -{
> -   struct nic_info *nic = (struct nic_info *) p;
> -   list_del(&nic->list);
> -   FREE(nic);
> -}
> -
>  /**
>    * Create and initialize a new object for a specific network interface dev.
>    * \param  pane  parent context.
> @@ -284,11 +276,6 @@ hud_nic_graph_install(struct hud_pane *pane, const char *nic_name,
>     gr->query_data = nic;
>     gr->query_new_value = query_nic_load;
>
> -   /* Don't use free() as our callback as that messes up Gallium's
> -    * memory debugger.  Use simple free_query_data() wrapper.
> -    */
> -   gr->free_query_data = free_query_data;
> -
>     hud_pane_add_graph(pane, gr);
>     hud_pane_set_max_value(pane, 100);
>  }
> diff --git a/src/gallium/auxiliary/hud/hud_sensors_temp.c b/src/gallium/auxiliary/hud/hud_sensors_temp.c
> index e41b847..4a8a4fc 100644
> --- a/src/gallium/auxiliary/hud/hud_sensors_temp.c
> +++ b/src/gallium/auxiliary/hud/hud_sensors_temp.c
> @@ -189,17 +189,6 @@ query_sti_load(struct hud_graph *gr)
>     }
>  }
>
> -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();
> -}
> -
>  /**
>    * Create and initialize a new object for a specific sensor interface dev.
>    * \param  pane  parent context.
> @@ -237,11 +226,6 @@ hud_sensors_temp_graph_install(struct hud_pane *pane, const char *dev_name,
>     gr->query_data = sti;
>     gr->query_new_value = query_sti_load;
>
> -   /* Don't use free() as our callback as that messes up Gallium's
> -    * memory debugger.  Use simple free_query_data() wrapper.
> -    */
> -   gr->free_query_data = free_query_data;
> -
>     hud_pane_add_graph(pane, gr);
>     switch (sti->mode) {
>     case SENSORS_TEMP_CURRENT:
>