From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-0010f301.pphosted.com (mx0a-0010f301.pphosted.com [148.163.149.254]) by sourceware.org (Postfix) with ESMTPS id 8E80A3858D20 for ; Sat, 14 Oct 2023 18:29:48 +0000 (GMT) ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 8E80A3858D20 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=148.163.149.254 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697308192; cv=none; b=D3dwuEuEYo68uj5K+cHIbLcFaiYGk8hNhQ7dJiNSjVUaXxd/zh8xIcodIk3vk1m2UHXe385da0MrVX89ojlYtZ2jXdNGZONQ5wFtQACO+U2O0nO0nHchnkdBzh/D8ClPcyYe8Be8CJODZEOasPA3EUCWXndw2WGE8+2q1QNiaI0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697308192; c=relaxed/simple; bh=Cn+r+oeqU8M2OCWOGFKy/bngAlBY3Fg7j7WJ/caWxU8=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=aUmnailEi5UhS/jqncSyM86EJbvAmsXXBqFYAlybLY1xVVY+ZIjRCTC23ZyomL84BPNm+fwnjo9EYd1NatDe1UxZV3vSirgIaGPbBwAkmEm+pTHD4Xt/AYfq+Yu/YaQH43hFFujSVFBbpxLHhZ6wDkzZdfJA38rJ84uLRXuKDdA= ARC-Authentication-Results: i=1; server2.sourceware.org DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8E80A3858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=rice.edu Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=rice.edu Received: from pps.filterd (m0102855.ppops.net [127.0.0.1]) by mx0b-0010f301.pphosted.com (8.17.1.22/8.17.1.22) with ESMTP id 39EH0205023485 for ; Sat, 14 Oct 2023 13:29:46 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rice.edu; h= mime-version:references:in-reply-to:from:date:message-id:subject :to:cc:content-type; s=ricemail; bh=iGJpOXbdbk1vEMeVEQRfiQ0bl/2Z 2LJgmd8J2ckhTWk=; b=AI5jD5tiAyempjumURPIzMquvryOwbcCNzKNTb8ma2rt jzqVzAkGPcMpUhYPY1hbg0+vnlRREV8ygq1kf7Jchdz9apQLRdtUs1oci1w3jGS2 tJLzwl56Xla8UygvIk4TKxXBJPK+buB8QohG+JzHLgPfyRpFix1zmMC6Mmzzj5As lpmuixFlapJ6q/uDquwgmVokKPrASL4xB4gXpKEuL4CODVD0ZLBAitBd7z/XcX1z dlwYWUfo1nn3DkVYEOczDP3FaS7T230t6QuDvvxULQtktwOP4SBFE6eDcMSAySIc jF8ICtGcJnUCLBWKmdEspKR/F3oexhi8wiWoAxR3bQ== Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by mx0b-0010f301.pphosted.com (PPS) with ESMTPS id 3tqra48ewj-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NOT) for ; Sat, 14 Oct 2023 13:29:46 -0500 (CDT) Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-40540179bcdso24226945e9.2 for ; Sat, 14 Oct 2023 11:29:46 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697308184; x=1697912984; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=iGJpOXbdbk1vEMeVEQRfiQ0bl/2Z2LJgmd8J2ckhTWk=; b=YZAvqDc76V2J5s3l+erJMal/7jkaXMR+6W+u+KvHoaeP0glejVvkcIWQ4Tt4X0G6Vr igCa/ishyF+jHxNpH+hfY7qW91sZUb20AbHYkoJAmU11Xs+hDk1mQApNmYp/HBv79GWO /0rTXTqsg/22s8nMfFDmhn+WlFtz4Be4Fnl+NsS5/b55XEJRFQ8cu5xiWJOCwEoyCJ+o f2dwOweCzLsbZej6RVZC0RW0BBtsBz/XS4TZzTMAinOeYYZ0VFCr/DEjybuDSvIv5y+1 xd4HJQxCelXWvtby33TZ9AejJ4pmYu1MvpkRYzFzg9eggTDPl230AL1I0/PB6n+YPx6P Xh0g== X-Gm-Message-State: AOJu0YyHjTHCqJYBXZGac3yrrb8xHiY8H5gWnJyGS5TKaDLZT/obIwF0 Xdw9gvtzF8VRbRoO2UuOhpGQhcV6WnacRHQGe9KR4bxrF9jV7SWVdX+WVGGilIPqJ6frJW+o3Vz kqiayYaI1QR92SJgCjgjESiTz2Y3ZU5sOST86QnpXE4SKmwe9wrTbggp/kVnnJ8GtS17H X-Received: by 2002:adf:8b95:0:b0:32d:81f7:d6f2 with SMTP id o21-20020adf8b95000000b0032d81f7d6f2mr8373282wra.30.1697308184177; Sat, 14 Oct 2023 11:29:44 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHkOEDzU0/VeLpajdcksQgvG0O5K9dDLYSc0XH6i788IEn5pqUBfsf4lFiVJVTnvVxWNuw4Bz7/qBa2BJCwGok= X-Received: by 2002:adf:8b95:0:b0:32d:81f7:d6f2 with SMTP id o21-20020adf8b95000000b0032d81f7d6f2mr8373267wra.30.1697308183744; Sat, 14 Oct 2023 11:29:43 -0700 (PDT) MIME-Version: 1.0 References: <301fac87e83ebbbd677750579ae9a3429b461bdf.camel@klomp.org> <7e00014702b125aa4a9e0cc4534e1bffdbbe3d0a.camel@klomp.org> In-Reply-To: <7e00014702b125aa4a9e0cc4534e1bffdbbe3d0a.camel@klomp.org> From: Heather McIntyre Date: Sat, 14 Oct 2023 13:29:32 -0500 Message-ID: Subject: Re: [PATCH] Fix thread-safety for elfutils To: Mark Wielaard Cc: elfutils-devel@sourceware.org, John Mellor-Crummey Content-Type: multipart/alternative; boundary="0000000000005ded360607b15bbb" X-Proofpoint-DLP: Gmail-Outbound X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.272,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26 definitions=2023-10-13_12,2023-10-12_01,2023-05-22_02 X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,HTML_MESSAGE,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --0000000000005ded360607b15bbb Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Mark, Thank you for taking the time to review the patch and for your thoughtful feedback. I really appreciate your attention to detail and effort in splitting the commit into smaller, more manageable parts. John and I went over some of your comments and concerns yesterday, and I will be working on addressing them ASAP. I have a quick query regarding how you'd prefer to handle these changes. Would you like me to implement some of the recommended modifications and commit them (if possible), or would you prefer that I simply leave comments so you can make the necessary adjustments? I noticed that the split-up branch resides on what appears to be your personal space at https://code.wildebeest.org/git/user/mjw/elfutils/log/?h=3Dthread-safety. I= 'm uncertain whether I have the necessary access for making commits there. Best regards, Heather McIntyre On Sat, Oct 14, 2023 at 10:40=E2=80=AFAM Mark Wielaard wro= te: > 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. > > > > 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). > > > > 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 > --0000000000005ded360607b15bbb--