From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (gnu.wildebeest.org [45.83.234.184]) by sourceware.org (Postfix) with ESMTPS id CF0843858D20 for ; Sat, 14 Oct 2023 15:39:30 +0000 (GMT) ARC-Filter: OpenARC Filter v1.0.0 sourceware.org CF0843858D20 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=45.83.234.184 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697297974; cv=none; b=SzHOgB5cc2TGL/KNGhto0oApKskyy02g3wEq1a0od8GbPnvXk9E+sDqGknnWA2F3cUY7gEMS+RFkXrF691BkAD/y0+2uAnnBNu7EAibE2QWlFHEB7T9gW2fcuZZQG9gMffP+fxNZhN9Eg5OpFrWGokvhrznPGdmIW6qJW9AY/Tg= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697297974; c=relaxed/simple; bh=7ltdQjNlHEtFegDf3D3JMIDM3siil6+MSkTQw7MJFWI=; h=Message-ID:Subject:From:To:Date:MIME-Version; b=ATSiL+gaZ/7LYVy+Zm/DS1tBsOCB4ADNxdLP3xueFkjXlr83C1GpiyUcgbEATpkHOLD2z3m3d9iAXWzjUYUMxUGm0+seqzKm9+f8xpb5LBkeDw3DckqCIPbk9y2l6B4yjr41yCkwwL6kiZ29M6oDHNshqKWfcFCgN+2slSVbEm4= ARC-Authentication-Results: i=1; server2.sourceware.org DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CF0843858D20 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=klomp.org Received: from r6.localdomain (82-217-174-174.cable.dynamic.v4.ziggo.nl [82.217.174.174]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id 65100300B302; Sat, 14 Oct 2023 17:39:29 +0200 (CEST) Received: by r6.localdomain (Postfix, from userid 1000) id 0EB8834031B; Sat, 14 Oct 2023 17:39:29 +0200 (CEST) Message-ID: <7e00014702b125aa4a9e0cc4534e1bffdbbe3d0a.camel@klomp.org> Subject: Re: [PATCH] Fix thread-safety for elfutils From: Mark Wielaard To: Heather McIntyre , elfutils-devel@sourceware.org Cc: John Mellor-Crummey Date: Sat, 14 Oct 2023 17:39:28 +0200 In-Reply-To: <301fac87e83ebbbd677750579ae9a3429b461bdf.camel@klomp.org> References: <301fac87e83ebbbd677750579ae9a3429b461bdf.camel@klomp.org> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) MIME-Version: 1.0 X-Spam-Status: No, score=-3026.9 required=5.0 tests=BAYES_00,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,RCVD_IN_BARRACUDACENTRAL,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi Heather, On Tue, 2023-10-10 at 15:40 +0200, Mark Wielaard wrote: > Very nice. That is a lot of work. And I must admit that I cannot hold > that much work in my little head at the same time. So I have split up > your commit into (what I hope are) logical independent parts. That will > make it easier to review. I might have split it into too many parts, > but that at least makes it easier to just add those parts that are > trivially correct. >=20 > The only changes I made were: > 1. Move the ChangeLog entries into the commit message > (This is something we do now and makes cherry picking small > changes easier, but I see it isn't actually documented > in CONTRIBUTING, sorry. I'll try to update that.) > 2. Fixed up some Copyright notices as discussed off-list. > 3. Made some whitespace/indentation changes which made the > diffs slightly smaller (in most cases). >=20 > I'll comment/review the individual commits. Which I'll post to the > list. So I commented on each one individually. Below is the summary. I have pushed 4 patches to git trunk already. I am suggesting to drop 2 changes, but please feel free to say we really need them. I have some questions about the rest, but some are minor issues. In general I really like the direction of this. Hope my splitting of your patch isn't too confusing. But it really helps me review separate smaller independent parts. And it makes it so we can push more of your changes early. > Heather McIntyre (16): > lib: Add new once_define and once macros to eu-config.h Looks good. Added to main branch. > libelf: Make elf_version thread-safe Looks good. Added to main branch. Question whether this could also have been done with atomics? > libelf: Fix deadlock in __libelf_readall I think the locking here is subtly wrong in the case of elf->map_address !=3D NULL Suggest to look into rewriting libelf_acquire_all and libelf_release_all to libelf_acquire_all_children and libelf_release_all_children (which would only be called with the elf->lock already acquired). > libelf: Fix deadlock in elf_cntl Looks correct, but can probably be simplified by just relying on if (__libelf_readall (elf) =3D=3D NULL) > libelf: Fix elf_end deadlock Looks good. Added to main branch. > libelf: Make elf32_getchdr and elf64_getchdr thread-safe Looks good. Just needs elf32_getchdr.h added to noinst_HEADERS. Pushed to the main branch with that change. > lib: Add eu_tsearch and eu_tfind I think this would work. But I really like to discuss whether we can have more fine grained locking. The static global lock will likely cause too much contention. I added a similar change to noinst_HEADERS as above for eu-search.h. > libcpu: Change calls for tsearch/tfind to eu_tsearch/eu_tfind. Suggest to drop this for now. It will need a twalk wrapper. And I am not sure it will be easy to make the bison parser concurrent as a whole. This is for libasm, not libdw, so can imho be handled separately if we really want to make libasm also completely thread-safe. > src: Use eu-search in nm and findtextrel. Although this works, I don't think it is needed because eu-nm and eu- findtextrel are single threaded utilities. > libdw: make dwarf_getalt thread-safe I think this should also include dwarf_setalt. Also lets discuss whether the locking is done correctly. > libdw: Add locking around __libdw_dieabbrev for dwarf_hasattr I think this is also incomplete. dwarf_child, dwarf_getattrs, dwarf_haschildren and dwarf_tag also call __libdw_dieabbrev. I am also concerned this locking is very "heavy". Lets discuss whether there is an alternative way to update the die->abbrev in a thead-safe way. > libdw: Make libdw_find_split_unit thread-safe I have some concerns about whether the locking is complete/wide enough. > libdw: Make libdw_findcu thread-safe Looks mostly correct. Question about whether this could be a per struct Dwarf lock and whether the lock should also be added for the dwarf_formref_die call to __libdw_intern_next_unit. > libdw,libdwfl: Use eu-search for thread-safety These all look good. Just have to think about whether the global static lock currently used in eu-search is the right approach. > tests: Add eu-search tests I am enthusiastic about having these new tests. Some suggestions to simplify the mechanics. I also have updated the Copyright on the new files. Could you look into the invocation of eu_search_macros. Maybe I am misinterpreting how this should be used. But it looks like this will always fail. > configure: No longer mark --enable-thread-safety as EXPERIMENTAL Looks good. Will push once at least all libelf patches are in. Note, your NEWS entry seems missing. > Which can also be found here: > https://code.wildebeest.org/git/user/mjw/elfutils/log/?h=3Dthread-safety I have updated that git branch with some of the suggestions above and removed the commits that have already been pushed. Thanks, Mark