[Mesa-dev] [PATCH 7/7] util/disk_cache: also write key_blob to cache entry

[Timothy Arceri]

On 16/03/17 10:09, Grazvydas Ignotas wrote:
> This can be used to deal with key hash collisions from different
> version (should we find that to actually happen) and to find
> which mesa version produced it the entry.
>
> Signed-off-by: Grazvydas Ignotas <notasas at gmail.com>
> ---
>  src/util/disk_cache.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/src/util/disk_cache.c b/src/util/disk_cache.c
> index ad591be..8ac9e48 100644
> --- a/src/util/disk_cache.c
> +++ b/src/util/disk_cache.c
> @@ -766,6 +766,7 @@ destroy_put_job(void *job, int thread_index)
>  struct cache_entry_file_data {
>     uint32_t crc32;
>     uint32_t uncompressed_size;
> +   uint32_t key_blob_size;
>  };
>
>  static void
> @@ -839,6 +840,7 @@ cache_put(void *job, int thread_index)
>     struct cache_entry_file_data cf_data;
>     cf_data.crc32 = util_hash_crc32(dc_job->data, dc_job->size);
>     cf_data.uncompressed_size = dc_job->size;
> +   cf_data.key_blob_size = dc_job->cache->key_blob_size;
>
>     size_t cf_data_size = sizeof(cf_data);
>     ret = write_all(fd, &cf_data, cf_data_size);
> @@ -847,6 +849,16 @@ cache_put(void *job, int thread_index)
>        goto done;
>     }
>
> +   /* Write the key_blob, this can be used find information about the
> +    * mesa version that produced the entry or deal with hash collisions,
> +    * should that ever become a real problem.
> +    */
> +   ret = write_all(fd, dc_job->cache->key_blob, dc_job->cache->key_blob_size);
> +   if (ret == -1) {
> +      unlink(filename_tmp);
> +      goto done;
> +   }
> +
>     /* Now, finally, write out the contents to the temporary file, then
>      * rename them atomically to the destination filename, and also
>      * perform an atomic increment of the total cache size.
> @@ -859,7 +871,7 @@ cache_put(void *job, int thread_index)
>     }
>     rename(filename_tmp, filename);
>
> -   file_size += cf_data_size;
> +   file_size += cf_data_size + dc_job->cache->key_blob_size;
>     p_atomic_add(dc_job->cache->size, file_size);
>
>   done:
> @@ -966,8 +978,16 @@ disk_cache_get(struct disk_cache *cache, const cache_key key, size_t *size)
>           goto fail;
>     }
>
> +   if (cf_data.key_blob_size != cache->key_blob_size)
> +      goto fail;
> +
> +   /* Right now we don't use the key_blob, just skip it */
> +   ret = lseek(fd, cache->key_blob_size, SEEK_CUR);
> +   if (ret == -1)
> +      goto fail;
> +
>     /* Load the actual cache data. */
> -   size_t cache_data_size = sb.st_size - cf_data_size;
> +   size_t cache_data_size = sb.st_size - cf_data_size - cache->key_blob_size;
>     for (len = 0; len < cache_data_size; len += ret) {
>        ret = read(fd, data + len, cache_data_size - len);
>        if (ret == -1)
>

I'm not sure how much I like writing this as a blob. It makes reading 
the header difficult. For example radeonsi will contain an llvm_id 
string while i965 will not. Can we not just define a cache_key struct to 
be passed to disk_cache_create().

struct cache_driver_keys {
    char *mesa_id;
    char *llvm_id;
    char *gpu_id;
}

It does mean we can't just use cache_entry_file_data for these strings, 
but that's not a big deal. We do need to a least write 0 when llvm_id is 
NULL.

As well as those 3 we would probably want to add ptr size to 
cache_entry_file_data.