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

[Dieter Nützel]

Am 25.02.2017 05:56, schrieb Timothy Arceri:
> 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/

Shouldn't sha1 _completely_ avoided, then?

Greetings,
   Dieter

>> 
>> Marek