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

[Michel Dänzer]

On 27/02/17 09:20 PM, Marek Olšák wrote:
> 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?

We cannot, of course — if we could, it would be possible to compress any
file to any smaller size without loss. :) (Reminds me of the hilarious
TV series Silicon Valley)

If there are any other parameters like the size that can easily be
double-checked, that's probably a good idea just in case, but other than
that I think Timothy's followup is quite convincing that this is
extremely unlikely to ever happen in practice.

It might also be worth looking at what (if anything) ccache does to deal
with this though. (Has anyone run into or heard of a ccache issue due to
a hash collision? A quick web search only turns up
https://sourceforge.net/p/free-cad/mailman/message/33250553/ for me, but
it lacks details about the suspected collision)


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