[Mesa-dev] [PATCH 1/2] radeonsi: add support for an on-disk shader cache

[Marek Olšák]

On Mon, Feb 27, 2017 at 10:57 AM, Michel Dänzer <michel at daenzer.net> wrote:
> On 25/02/17 01:56 PM, Timothy Arceri wrote:
>> On 24/02/17 21:02, Marek Olšák wrote:
>>> On Fri, Feb 24, 2017 at 3:18 AM, Timothy Arceri
>>> <tarceri at itsqueeze.com> wrote:
>>>> On 24/02/17 08:49, Timothy Arceri wrote:
>>>>> On 24/02/17 05:12, Marek Olšák wrote:
>>>>>> On Thu, Feb 23, 2017 at 3:09 AM, Timothy Arceri
>>>>>> <tarceri at itsqueeze.com> wrote:
>>>>>>>
>>>>>>> From: kdj0c <kdj0c at djinvi.net>
>>>>>>>
>>>>>>> V2 (Timothy Arceri):
>>>>>>> - when loading from disk cache also binary insert into memory cache.
>>>>>>> - check that the binary loaded from disk is the correct size. If not
>>>>>>>   delete the cache item and skip loading from cache.
>>>>>>> ---
>>>>>>>  src/gallium/drivers/radeonsi/si_state_shaders.c | 69
>>>>>>> ++++++++++++++++++++++---
>>>>>>>  1 file changed, 62 insertions(+), 7 deletions(-)
>>>>>>>
>>>>>>> diff --git a/src/gallium/drivers/radeonsi/si_state_shaders.c
>>>>>>> b/src/gallium/drivers/radeonsi/si_state_shaders.c
>>>>>>> index f615aa8..71556f9 100644
>>>>>>> --- a/src/gallium/drivers/radeonsi/si_state_shaders.c
>>>>>>> +++ b/src/gallium/drivers/radeonsi/si_state_shaders.c
>>>>>>> @@ -36,6 +36,9 @@
>>>>>>>  #include "util/u_memory.h"
>>>>>>>  #include "util/u_prim.h"
>>>>>>>
>>>>>>> +#include "util/disk_cache.h"
>>>>>>> +#include "util/mesa-sha1.h"
>>>>>>> +
>>>>>>>  /* SHADER_CACHE */
>>>>>>>
>>>>>>>  /**
>>>>>>> @@ -182,10 +185,12 @@ static bool si_load_shader_binary(struct
>>>>>>> si_shader *shader, void *binary)
>>>>>>>   */
>>>>>>>  static bool si_shader_cache_insert_shader(struct si_screen *sscreen,
>>>>>>>                                           void *tgsi_binary,
>>>>>>> -                                         struct si_shader *shader)
>>>>>>> +                                         struct si_shader *shader,
>>>>>>> +                                         bool
>>>>>>> insert_into_disk_cache)
>>>>>>>  {
>>>>>>>         void *hw_binary;
>>>>>>>         struct hash_entry *entry;
>>>>>>> +       uint8_t key[CACHE_KEY_SIZE];
>>>>>>>
>>>>>>>         entry = _mesa_hash_table_search(sscreen->shader_cache,
>>>>>>> tgsi_binary);
>>>>>>>         if (entry)
>>>>>>> @@ -201,6 +206,12 @@ static bool si_shader_cache_insert_shader(struct
>>>>>>> si_screen *sscreen,
>>>>>>>                 return false;
>>>>>>>         }
>>>>>>>
>>>>>>> +       if (sscreen->b.disk_shader_cache && insert_into_disk_cache) {
>>>>>>> +               _mesa_sha1_compute(tgsi_binary, *((uint32_t
>>>>>>> *)tgsi_binary), key);
>>>>>>
>>>>>>
>>>>>> What happens if we randomly get a sha1 collision?
>>>>>
>>>>>
>>>>> You should stop playing your game which will be rendering incorrectly
>>>>> and by a lotto ticket.
>>>>>
>>>>>> Shouldn't we store the whole key as well?
>>>>>
>>>>>
>>>>> Sure I can add that, its cheap to check here anyway. Although the other
>>>>> cache stages rely on a collision being improbable.
>>>>
>>>>
>>>>
>>>> For some reason I thought the key was simpler than it is. It seems
>>>> excessive
>>>> to store and compare the tgsi again. I don't think git even worries
>>>> about
>>>> the possibility of a collision and we will be dealing with much smaller
>>>> amounts of cache items then commits in a git repository.
>>>>
>>>> Thoughts?
>>>
>>> I'll let others comment on this. If nobody comments, checking only the
>>> key can stay.
>>
>> Seems SVN didn't used to worry about collisions either.
>>
>> https://arstechnica.com/security/2017/02/watershed-sha1-collision-just-broke-the-webkit-repository-others-may-follow/
>
> What scenario are we concerned about here? Getting a genuine SHA1
> collision between two shader objects is still as unlikely as it ever was
> (virtually impossible?), and someone with access to the shader cache
> files can wreak havoc much more easily than via a hash collision.

I'm concerned about the random scenario where 2 shaders can have the
same hash. That is, if the user doesn't do anything wrong and nobody
else corrupts the cache, can we say for sure that a collision will
never ever happen?

The patch can land as-is, but the topic is still open.

Marek