Re: [Libunwind-devel] [PATCH V2] Configurable cache size

[Dave Watson]

Hi Bert, 

Thanks for the review!  V3 coming shortly.

On 12/02/16 07:48 AM, Bert Wesarg wrote:
> >  include/dwarf.h               | 21 ++++++++++-----
> >  include/libunwind-common.h.in |  2 ++
> >  src/Makefile.am               |  6 +++--
> >  src/dwarf/Gparser.c           | 62 
> > ++++++++++++++++++++++++++++++++-----------
> >  src/mi/Gset_cache_size.c      | 58 ++++++++++++++++++++++++++++++++++++++++
> >  src/mi/Lset_cache_size.c      |  5 ++++
> >  tests/check-namespace.sh.in   |  2 ++
> >  tests/test-flush-cache.c      |  1 +
> 
> I miss a manual page entry

Will add

> > +HIDDEN int
> > +dwarf_flush_rs_cache (struct dwarf_rs_cache *cache)
> >  {
> >    int i;
> >
> > -  cache->lru_head = DWARF_UNW_CACHE_SIZE - 1;
> > +  if (cache->log_size == DWARF_DEFAULT_LOG_UNW_CACHE_SIZE
> > +      || !cache->hash) {
> > +    cache->hash = cache->default_hash;
> > +    cache->buckets = cache->default_buckets;
> > +    cache->log_size = DWARF_DEFAULT_LOG_UNW_CACHE_SIZE;
> > +  } else {
> > +    if (cache->hash && cache->hash != cache->default_hash)
> > +      munmap(cache->hash, DWARF_UNW_HASH_SIZE(cache->prev_log_size));
> > +    if (cache->buckets && cache->buckets != cache->default_buckets)
> > +      munmap(cache->buckets, DWARF_UNW_CACHE_SIZE(cache->prev_log_size));
> > +    GET_MEMORY(cache->hash, DWARF_UNW_HASH_SIZE(cache->log_size)
> > +                             * sizeof (cache->hash[0]));
> > +    GET_MEMORY(cache->buckets, DWARF_UNW_CACHE_SIZE(cache->log_size)
> > +                                * sizeof (cache->buckets[0]));
> > +    if (!cache->hash || !cache->buckets)
> > +      {
> > +        Debug (5, "Unable to allocate cache memory");
> > +        return -1;
> > +      }
> > +    cache->prev_log_size = cache->log_size;
> > +  }
> > +
> > +  cache->lru_head = DWARF_UNW_CACHE_SIZE(cache->log_size) - 1;
> >    cache->lru_tail = 0;
> >
> > -  for (i = 0; i < DWARF_UNW_CACHE_SIZE; ++i)
> > +  for (i = 0; i < DWARF_UNW_CACHE_SIZE(cache->log_size); ++i)
> >      {
> >        if (i > 0)
> >          cache->buckets[i].lru_chain = (i - 1);


> > +  as->global_cache.log_size = log_size;
> > +
> > +  /* Ensure caches are empty (and initialized).  */
> > +  unw_flush_cache (as, 0, 0);
> > +#ifdef __ia64__
> > +  return 0;
> > +#else
> > +  /* Synchronously purge cache, to ensure memory is allocated */
> > +  dwarf_flush_rs_cache(&as->global_cache);
> 
> missing return. please note, that if you use the return value from
> dwarf_flush_rs_cache, it will for the caller look like he gets
> -UNW_EUNSPEC in case of an error, as -1 will be returned there. Though
> -UNW_ENOMEM would be more appropriate. Thus it may be better to
> convert the result:
> 
> if (0 > dwarf_flush_rs_cache(&as->global_cache))
>     return -UNW_ENOMEM;
> return 0;
> 

Nice catch.  Looks like I should return UNW_ENOMEM from flush
directly.