This is the mail archive of the elfutils-devel@sourceware.org mailing list for the elfutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] lib + libdw: Add and use a concurrent version of the dynamic-size hash table.


Hi,

On Thu, 2019-11-07 at 09:24 -0600, Jonathon Anderson wrote:
> On Thu, Nov 7, 2019 at 12:07, Mark Wielaard <mark@klomp.org> wrote:
> > Looking at the difference between the previous version and this one, 
> > it
> > incorporates my simplification of FIND and lookup functions. And fixes
> > it by making insert_helper consistently return -1 when the value was
> > already inserted (hash == hval). And it fixes an issue where we were
> > using the the table entry val_ptr instead of the hashval as hash (was
> > that the typo? it didn't look harmless).
> 
> Yep, those are the changes (plus the Sig8 patch). That typo was 
> harmless because hash would be overwritten before its next use (or just 
> unused), now with the (hash == hval) clause its always read so the typo 
> is fixed.

Thanks for explaining. I have now finally pushed this to master.

> Regarding the Sig8 table: I took a close look, and at the moment its 
> use is in an area that isn't thread-safe anyway (in particular, 
> __libdw_intern_next_unit). Depending on how that is parallelized there 
> might be an issue (if its just wrapped with a separate mutex a thread 
> might "miss" a CU if its not already in the table), but since that 
> region would need inspection at that time anyway I'm fine with either 
> option.

I still kept the code to handle the Sig8 table with the new concurrent-
safe code, since I think it is better if we use the new code always
(even in the single threaded case).

So to fix this we do need some mutex to protect the binary search tree
when calling tsearch/tfind? Or do you see other issues too?

> This isn't an issue for Dwarf_Abbrev, the worst that can happen there 
> is a duplicate alloc and parsing (as long as the DWARF doesn't have 
> erroneous abbrev entries, if it does we would need to thread-safe the 
> error handling too).

Unfortunately we don't always control the data, so bad abbrev entries
could happen. The extra alloc wouldn't really "leak" because it would
be freed with the Dwarf. So I am not too concerned about that. Is that
the worse that can happen in __libdw_getabbrev? When we goto invalid
the Dwarf_Abbrev would indeed "leak", but it isn't really lost, it will
get cleaned up when the Dwarf is destroyed.

Thanks,

Makr


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]