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

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer