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

[Ernst Sjöstrand]

Notice that git worries about collisions between the 7 char abbreviated
sha1s in large repositories, and sometime have to use 10 or 12 chars, etc:
https://github.com/blog/2288-git-2-11-has-been-released

"This is exactly what happened in the Linux kernel repository; it now has
over 5 million objects, meaning we'd expect collisions with names shorter
than 12 hexadecimal characters."

I think that speaks for itself.

Also, SHA1 is very good at generating a completely different hash for small
changes. So to generate a collision with X, you can't have X +/- a few
bytes.

Regards
//Ernst


2017-02-27 13:20 GMT+01:00 Marek Olšák <maraeo at gmail.com>:

> 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
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170227/d5408e2a/attachment.html>