[Mesa-dev] [PATCH] util/disk_cache: Fix disk_cache_get_function_timestamp with disabled cache.

[Bas Nieuwenhuizen]

On Thu, Jul 26, 2018 at 5:55 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 26 July 2018 at 16:25, Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl> wrote:
>> On Thu, Jul 26, 2018 at 5:15 PM, Bas Nieuwenhuizen
>> <bas at basnieuwenhuizen.nl> wrote:
>>> On Thu, Jul 26, 2018 at 5:01 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>> Hi Bas,
>>>>
>>>> Pardon for joining so late.
>>>>
>>>> On 18 July 2018 at 13:01, Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl> wrote:
>>>>> radv always needs it, so just check the header instead. Also
>>>>> do not declare the function if the variable is not set, so we
>>>>> get a nice compile error instead of failing to open a device
>>>>> at runtime.
>>>>>
>>>> AFAICT the ENABLE_SHADER_CACHE define is set on all POSIX platforms.
>>>> Back in the early days we had --enable-shader-cache which was removed
>>>> with d1efa09d342bff3e5def2978a0bef748d74f9c82
>>>>
>>>> Apart from the reasons listed, we do not want to control this at
>>>> configure time since it will produce varying driver behaviour. Hence
>>>> multiple unhappy users, as they hit the codepath w/o cache.
>>>>
>>>> Skimming through - I would imagine you're building with meson, which
>>>> has the explicit shader cache toggle.
>>>> I'm assuming Dylan had it based on early autconf builds?
>>>>
>>>> We can drop that one and effectively revert this and the follow-up Android fix.
>>>>
>>>> What do you think?
>>>
>>> ChromeOS disables it for reasons I'm currently not sure about:
>>>
>>> https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/media-libs/mesa/mesa-18.2_pre1.ebuild#197
>>>
>>> And since the primary use case for vulkan at the moment seems to be
>>> Android and I can conceive us not being able to use the cache there
>>> for reasons, I'm not sure if disabling the cache is really that
>>> unreasonable, though it is not ideal that they apparently override the
>>> upstream configure.
>>
>> To give a bit more context on why it was disabled:
>>
>> "Starting with commit 96fe36f7 in mesa, the disk shader cache has been
>> enabled. This feature attempts to use multi-threading during GPU
>> initialization while Chromium enforces single threading during the
>> initialization process. The conflict causes the system to hang during
>> boot. This patchset includes the following line in the non-9999 ebuild
>> file, which disables the feature and allows platforms to boot:
>>
>> append-flags "-UENABLE_SHADER_CACHE"
>> "
>> (https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/1056288)
>>
> Huge thanks for the context. Did you get that from the bug report -
> seems to be missing from gerrit.

Nope, gerrit, just need to click on the "SHOW ALL 51 MESSAGES" button.
>
> Regardless, the solution seems like a good first step, although
> something better is needed in the long run.
>
> Perhaps, forcing the system to fail on thread_create and adding error
> handling for util_queue_init() in src/util/disk_cache.c?
> Thus anything spawning threads will get the same consistent treatment
> and all of these workarounds can go.

It is my understanding that it does not fail thread creation, but if
at sandboxing time it detects multiple threads it kills the process.
Which unfortunately turns this into a game of "you are not allowed to
create any threads but we are not telling you", which is somewhat
annoying to fix without configuration.

Not familiar enough yet with the how and why of sandboxing this way or
why we open a GL context before that though.


>
> HTH
> Emil
>
>
>>>
>>> Since most of that stuff seems to be driven by GL, this is pretty much
>>> a "isolate radv from whatever people do on the GL side of things"
>>> patch.
>>>
>>>> Emil