From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31608 invoked by alias); 21 Aug 2019 11:16:08 -0000 Mailing-List: contact elfutils-devel-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Post: List-Help: List-Subscribe: Sender: elfutils-devel-owner@sourceware.org Received: (qmail 31598 invoked by uid 89); 21 Aug 2019 11:16:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Checked: by ClamAV 0.100.3 on sourceware.org X-Virus-Found: No X-Spam-SWARE-Status: No, score=-6.6 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.1 spammy=HX-Languages-Length:2276, atomics, our X-Spam-Status: No, score=-6.6 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on sourceware.org X-Spam-Level: X-HELO: gnu.wildebeest.org Received: from wildebeest.demon.nl (HELO gnu.wildebeest.org) (212.238.236.112) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 21 Aug 2019 11:16:07 +0000 Received: from tarox.wildebeest.org (tarox.wildebeest.org [172.31.17.39]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id 07102300073F; Wed, 21 Aug 2019 13:16:04 +0200 (CEST) Received: by tarox.wildebeest.org (Postfix, from userid 1000) id B9B7740006CA; Wed, 21 Aug 2019 13:16:04 +0200 (CEST) Message-ID: <410becd068420cbd3b93c161be2084c6a5d2f362.camel@klomp.org> Subject: Re: [PATCH] libdw: add thread-safety to dwarf_getabbrev() From: Mark Wielaard To: Jonathon Anderson , elfutils-devel@sourceware.org Cc: Srdan Milakovic Date: Wed, 21 Aug 2019 11:16:00 -0000 In-Reply-To: <1565983469.1826.0@smtp.mail.rice.edu> References: <1565983469.1826.0@smtp.mail.rice.edu> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Evolution 3.28.5 (3.28.5-5.el7) Mime-Version: 1.0 X-Spam-Flag: NO X-IsSubscribed: yes X-SW-Source: 2019-q3/txt/msg00118.txt.bz2 Hi Jonathon and Sr=C4=91an, On Fri, 2019-08-16 at 14:24 -0500, Jonathon Anderson wrote: > For parallel applications that need the information in the DIEs, the > Dwarf_Abbrev hash table et al. become a massive data race. This fixes=20 > that by: >=20 > 1. Adding atomics & locks to the hash table to manage concurrency > (lib/dynamicsizehash_concurrent.{c,h}) > 2. Adding a lock & array structure to the memory manager (pseudo-TLS) > (libdwP.h, libdw_alloc.c) > 3. Adding extra configure options for Helgrind/DRD annotations > (configure.ac) > 4. Including "stdatomic.h" from FreeBSD, to support C11-style atomics. > (lib/stdatomic.h) This looks like really nice work. Thanks! I am splitting review in some smaller parts if you don't mind. Simply because it is large and I cannot keep everything in my head at once :) But here some initial overall comments. > Notes: > - GCC >=3D 4.9 provides natively; for those versions > lib/stdatomic.h could be removed or disabled. We can also rewrite the > file if the copyright becomes an issue. If the compiler provides stdatomic.h then I think it would be good to use that instead of our own implementation. The copyright isn't a problem. But do you have a reference/URL to the upstream version? I like to add that somewhere, so we can sync with it in the future. I see various commented out parts. Was that already upstream? Should we just remove those parts? > - Currently the concurrent hash table is always enabled,=20 > performance-wise there is no known difference between it=20=20 > and the non-concurrent version. > This can be changed to toggle with --enable-thread-safety > if preferred. I would prefer it always enabled, unless there is a massive slowdown of the single-threaded case. The problem with --enable-thread-safety is that it is a) known broken (sigh) and b) it basically introduces two separate libraries that behave subtly differently. I would very much like to get rid of --enable-thread-safety by fixing the broken locking and simply making it the default. > - Another implementation of #2 above might use dynamic TLS=20 > (pthread_key_*), > we chose this implementation to reduce the overall complexity. Are there any other trade-offs? Thanks, Mark