[Mesa-dev] [PATCH 2/3] Revert "disk cache: initialize cache path and index only when used"
Marek Olšák
maraeo at gmail.com
Mon Feb 12 15:53:44 UTC 2018
radeonsi uses the shader cache from multiple threads.
Marek
On Fri, Feb 9, 2018 at 4:59 AM, Timothy Arceri <tarceri at itsqueeze.com> wrote:
> This reverts commit 6a651b6b77b68db71a027c826abccc843ace88ef.
>
> This change was not thread safe. Applications are allowed to compile
> multiple programs attached to the same context in different threads
> which means we can end up with multiple threads inside
> disk_cache_path_init() or thinking disk_cache_path_init() has
> completed when it hasn't causing various problems.
>
> This change was also causing piglit runs to deadlock with radeonsi
> on a Ryzen 1800X (8 cores), I don't believe shader runner does
> threaded compiles so I'm unsure of the exact root of this additional
> problem.
> ---
> src/util/disk_cache.c | 129 +++++++++++++++++++-------------------------------
> 1 file changed, 49 insertions(+), 80 deletions(-)
>
> diff --git a/src/util/disk_cache.c b/src/util/disk_cache.c
> index dec5a67a79..2884d3c9c1 100644
> --- a/src/util/disk_cache.c
> +++ b/src/util/disk_cache.c
> @@ -77,7 +77,6 @@
> struct disk_cache {
> /* The path to the cache directory. */
> char *path;
> - bool path_init_failed;
>
> /* Thread queue for compressing and writing cache entries to disk */
> struct util_queue cache_queue;
> @@ -179,20 +178,37 @@ concatenate_and_mkdir(void *ctx, const char *path, const char *name)
> return NULL;
> }
>
> -static bool
> -disk_cache_path_init(struct disk_cache *cache)
> +#define DRV_KEY_CPY(_dst, _src, _src_size) \
> +do { \
> + memcpy(_dst, _src, _src_size); \
> + _dst += _src_size; \
> +} while (0);
> +
> +struct disk_cache *
> +disk_cache_create(const char *gpu_name, const char *timestamp,
> + uint64_t driver_flags)
> {
> - void *local = NULL;
> - char *path;
> + void *local;
> + struct disk_cache *cache = NULL;
> + char *path, *max_size_str;
> + uint64_t max_size;
> int fd = -1;
> struct stat sb;
> size_t size;
>
> + /* If running as a users other than the real user disable cache */
> + if (geteuid() != getuid())
> + return NULL;
> +
> /* A ralloc context for transient data during this invocation. */
> local = ralloc_context(NULL);
> if (local == NULL)
> goto fail;
>
> + /* At user request, disable shader cache entirely. */
> + if (env_var_as_boolean("MESA_GLSL_CACHE_DISABLE", false))
> + goto fail;
> +
> /* Determine path for cache based on the first defined name as follows:
> *
> * $MESA_GLSL_CACHE_DIR
> @@ -257,6 +273,10 @@ disk_cache_path_init(struct disk_cache *cache)
> goto fail;
> }
>
> + cache = ralloc(NULL, struct disk_cache);
> + if (cache == NULL)
> + goto fail;
> +
> cache->path = ralloc_strdup(cache, path);
> if (cache->path == NULL)
> goto fail;
> @@ -305,58 +325,6 @@ disk_cache_path_init(struct disk_cache *cache)
> cache->size = (uint64_t *) cache->index_mmap;
> cache->stored_keys = cache->index_mmap + sizeof(uint64_t);
>
> - /* 1 thread was chosen because we don't really care about getting things
> - * to disk quickly just that it's not blocking other tasks.
> - *
> - * The queue will resize automatically when it's full, so adding new jobs
> - * doesn't stall.
> - */
> - util_queue_init(&cache->cache_queue, "disk_cache", 32, 1,
> - UTIL_QUEUE_INIT_RESIZE_IF_FULL |
> - UTIL_QUEUE_INIT_USE_MINIMUM_PRIORITY);
> -
> - ralloc_free(local);
> -
> - return true;
> -
> - fail:
> - if (fd != -1)
> - close(fd);
> -
> - if (local)
> - ralloc_free(local);
> -
> - cache->path_init_failed = true;
> -
> - return false;
> -}
> -
> -#define DRV_KEY_CPY(_dst, _src, _src_size) \
> -do { \
> - memcpy(_dst, _src, _src_size); \
> - _dst += _src_size; \
> -} while (0);
> -
> -struct disk_cache *
> -disk_cache_create(const char *gpu_name, const char *timestamp,
> - uint64_t driver_flags)
> -{
> - struct disk_cache *cache = NULL;
> - char *max_size_str;
> - uint64_t max_size;
> -
> - /* If running as a users other than the real user disable cache */
> - if (geteuid() != getuid())
> - return NULL;
> -
> - /* At user request, disable shader cache entirely. */
> - if (env_var_as_boolean("MESA_GLSL_CACHE_DISABLE", false))
> - return NULL;
> -
> - cache = rzalloc(NULL, struct disk_cache);
> - if (cache == NULL)
> - return NULL;
> -
> max_size = 0;
>
> max_size_str = getenv("MESA_GLSL_CACHE_MAX_SIZE");
> @@ -392,6 +360,16 @@ disk_cache_create(const char *gpu_name, const char *timestamp,
>
> cache->max_size = max_size;
>
> + /* 1 thread was chosen because we don't really care about getting things
> + * to disk quickly just that it's not blocking other tasks.
> + *
> + * The queue will resize automatically when it's full, so adding new jobs
> + * doesn't stall.
> + */
> + util_queue_init(&cache->cache_queue, "disk_cache", 32, 1,
> + UTIL_QUEUE_INIT_RESIZE_IF_FULL |
> + UTIL_QUEUE_INIT_USE_MINIMUM_PRIORITY);
> +
> uint8_t cache_version = CACHE_VERSION;
> size_t cv_size = sizeof(cache_version);
> cache->driver_keys_blob_size = cv_size;
> @@ -414,10 +392,8 @@ disk_cache_create(const char *gpu_name, const char *timestamp,
>
> cache->driver_keys_blob =
> ralloc_size(cache, cache->driver_keys_blob_size);
> - if (!cache->driver_keys_blob) {
> - ralloc_free(cache);
> - return NULL;
> - }
> + if (!cache->driver_keys_blob)
> + goto fail;
>
> uint8_t *drv_key_blob = cache->driver_keys_blob;
> DRV_KEY_CPY(drv_key_blob, &cache_version, cv_size)
> @@ -429,13 +405,24 @@ disk_cache_create(const char *gpu_name, const char *timestamp,
> /* Seed our rand function */
> s_rand_xorshift128plus(cache->seed_xorshift128plus, true);
>
> + ralloc_free(local);
> +
> return cache;
> +
> + fail:
> + if (fd != -1)
> + close(fd);
> + if (cache)
> + ralloc_free(cache);
> + ralloc_free(local);
> +
> + return NULL;
> }
>
> void
> disk_cache_destroy(struct disk_cache *cache)
> {
> - if (cache && cache->index_mmap) {
> + if (cache) {
> util_queue_destroy(&cache->cache_queue);
> munmap(cache->index_mmap, cache->index_mmap_size);
> }
> @@ -454,9 +441,6 @@ get_cache_file(struct disk_cache *cache, const cache_key key)
> char buf[41];
> char *filename;
>
> - if (!cache->path)
> - return NULL;
> -
> _mesa_sha1_format(buf, key);
> if (asprintf(&filename, "%s/%c%c/%s", cache->path, buf[0],
> buf[1], buf + 2) == -1)
> @@ -1012,11 +996,6 @@ disk_cache_put(struct disk_cache *cache, const cache_key key,
> const void *data, size_t size,
> struct cache_item_metadata *cache_item_metadata)
> {
> - /* Initialize path if not initialized yet. */
> - if (cache->path_init_failed ||
> - (!cache->path && !disk_cache_path_init(cache)))
> - return;
> -
> struct disk_cache_put_job *dc_job =
> create_put_job(cache, key, data, size, cache_item_metadata);
>
> @@ -1194,11 +1173,6 @@ disk_cache_put_key(struct disk_cache *cache, const cache_key key)
> int i = CPU_TO_LE32(*key_chunk) & CACHE_INDEX_KEY_MASK;
> unsigned char *entry;
>
> - if (!cache->path) {
> - assert(!"disk_cache_put_key called with no path set");
> - return;
> - }
> -
> entry = &cache->stored_keys[i * CACHE_KEY_SIZE];
>
> memcpy(entry, key, CACHE_KEY_SIZE);
> @@ -1218,11 +1192,6 @@ disk_cache_has_key(struct disk_cache *cache, const cache_key key)
> int i = CPU_TO_LE32(*key_chunk) & CACHE_INDEX_KEY_MASK;
> unsigned char *entry;
>
> - /* Initialize path if not initialized yet. */
> - if (cache->path_init_failed ||
> - (!cache->path && !disk_cache_path_init(cache)))
> - return false;
> -
> entry = &cache->stored_keys[i * CACHE_KEY_SIZE];
>
> return memcmp(entry, key, CACHE_KEY_SIZE) == 0;
> --
> 2.14.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list