[Mesa-dev] [PATCH 2/2] anv: Use build-id for pipeline cache UUID.

[Jason Ekstrand]

On Wed, Feb 15, 2017 at 1:54 PM, Chad Versace <chadversary at chromium.org>
wrote:

> On Wed 15 Feb 2017, Emil Velikov wrote:
> > On 14 February 2017 at 21:02, Chad Versace <chadversary at chromium.org>
> wrote:
> > > On Tue 14 Feb 2017, Kenneth Graunke wrote:
> > >> On Tuesday, February 14, 2017 12:38:45 PM PST Chad Versace wrote:
> > >> > On Tue 14 Feb 2017, Matt Turner wrote:
> > >> >
> > >> >
> > >> > >  static bool
> > >> > > -anv_get_function_timestamp(void *ptr, uint32_t* timestamp)
> > >> > > +anv_device_get_cache_uuid(void *uuid)
> > >> > >  {
> > >> > > -   Dl_info info;
> > >> > > -   struct stat st;
> > >> > > -   if (!dladdr(ptr, &info) || !info.dli_fname)
> > >> > > +   const struct note *note = build_id_find_nhdr("libvulkan_
> intel.so");
> > >> > > +   if (!note)
> > >> > >        return false;
> > >> > >
> > >> > > -   if (stat(info.dli_fname, &st))
> > >> > > +   unsigned len = build_id_length(note);
> > >> > > +   if (len < VK_UUID_SIZE)
> > >> > >        return false;
> > >> > >
> > >> > > -   *timestamp = st.st_mtim.tv_sec;
> > >> > > -   return true;
> > >> > > -}
> > >> > > -
> > >> > > -static bool
> > >> > > -anv_device_get_cache_uuid(void *uuid)
> > >> > > -{
> > >> > > -   uint32_t timestamp;
> > >> > > -
> > >> > > -   memset(uuid, 0, VK_UUID_SIZE);
> > >> > > -   if (!anv_get_function_timestamp(anv_device_get_cache_uuid,
> &timestamp))
> > >> > > +   unsigned char *build_id = malloc(len);
> > >> > > +   if (!build_id)
> > >> > >        return false;
> > >> > >
> > >> > > -   snprintf(uuid, VK_UUID_SIZE, "anv-%d", timestamp);
> > >> > > +   build_id_read(note, build_id);
> > >> > > +
> > >> > > +   memcpy(uuid, build_id, VK_UUID_SIZE);
> > >> > > +   free(build_id);
> > >> >
> > >> > The Vulkan spec frowns on memory allocations when not needed. If you
> > >> > must allocate memory here, then it should be through the VkInstance
> > >> > allocation callbacks. However, it's best to avoid the allocation by
> > >> > adding a size_t parameter, à la snprintf, to build_id_read().
> > >> >
> > >> > Otherwise, the patch looks good to me.
> > >>
> > >> You're worried about the performance of anv_physical_device_init()?
> > >> This doesn't happen on lookup...just driver start up...
> > >
> > > I'm not worried about performance here. That would be foolish.
> > >
> > > I'm concerned about complying the expectations provided to advanced
> > > users by the Vulkan spec.
>
> > Hi Chad, pardon for dropping in uninvited:
> >
> > Spec mandates that only the implementation should use the allocation
> > callbacks, and not any of it's dependencies (say something in the C
> > runtime/other library), correct ?
>
> > Or does it implicitly that "Thou Shalt override the weak
> > [mc]alloc/friends symbols provided by your C runtime" ?
>
> The spec actually says neither. It is vague on this topic, but vague in
> a hyper-detailed fashion, which is typical of Khronos specs.
>
> I interpret "Chapter 10. Memory Allocation" as saying:
>
>     - If the user supplies allocation callbacks, then the spec
>       recommends that the implementation use them. (The spec doesn't say
>       *should*, *suggest*, *must*, *shall*, or anything like that here,
>       as far as I can tell. It's my interpretation that Ch10 is primarily a
>       a recommendation.
>
>     - If the implementation does use an allocation callback, Ch10
>       defines precise constraints on how the implementation must use it.
>
>     - Ch10 admits that the implementation may need to allocate internal
>       memory, without using the callbacks. However, Ch10 seem to say
>       that such internal allocations are soley for executable memory.
>       Again, one could interpret the spec differently here.
>
> > Although the src/intel is [almost] perfect, the i965 compiler still
> > explicitly and implicitly uses [mc]alloc. Not mean to be bashful, just
> > pointing out what may not be obvious at first.
>
> Yep. The compiler doesn't use the allocation callbacks. For that matter,
> even printf calls malloc on some libc implementations.


Yeah, I think you're worrying too much.  Sure, it might be a nicer API if
get_build_id did a copy into a provided array, but I wouldn't make any
malloc arguments.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170215/0c9fcaf3/attachment.html>