From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23847 invoked by alias); 21 Oct 2019 16:28:19 -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 23836 invoked by uid 89); 21 Oct 2019 16:28:19 -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=-22.4 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,HTML_MESSAGE,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.1 spammy=HTo:U*mark, H*m:edu, idiot, H*c:alternative X-Spam-Status: No, score=-22.4 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,HTML_MESSAGE,RCVD_IN_DNSWL_LOW,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: mx0b-0010f301.pphosted.com Received: from mx0b-0010f301.pphosted.com (HELO mx0b-0010f301.pphosted.com) (148.163.153.244) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 21 Oct 2019 16:28:17 +0000 Received: from pps.filterd (m0102859.ppops.net [127.0.0.1]) by mx0b-0010f301.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id x9LGQTrh023713; Mon, 21 Oct 2019 11:28:13 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rice.edu; h=date : from : subject : to : cc : message-id : in-reply-to : references : mime-version : content-type; s=ricemail; bh=ueSgK/uuEDYXXR7Af5d/Z/erLo0jeN9DjKphWUWEP2Y=; b=jBR3OImduw6+JXC0u6ITarsJL5MHJ/AUVO8Q89aDUUt+4ELI1y8VOeUZwAerNuXouDIt NHANeQz6Y8cY3PtuNIhuVWvn/8ecaQlUP04yt7tZGO8AXyuRsVglfqtH4y27TH9NBBBu KAoYOYmMDkgYZHTmtI27YqwxEB+5ohLmw00gMzxBebi3YBf5y5F34pH4lWPV4dNQXbJ1 1rCCpM8zaGm9W0pPu+5Cq5No9MS2cyK2Cktt+K60Kaz3y+ly3bdDQl7eoi2d5ka2TK1y +Iq9iRRpre8NmFAUJ16qlDum9ZasgJMpJWQv9iwvFXtiJi4MGAiOSPHDQ1gmyz/rf6zg +A== Received: from mh2.mail.rice.edu (mh2.mail.rice.edu [128.42.201.21]) by mx0b-0010f301.pphosted.com with ESMTP id 2vqwuva0va-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 21 Oct 2019 11:28:13 -0500 Received-X: from mh2.mail.rice.edu (localhost.localdomain [127.0.0.1]) by mh2.mail.rice.edu (Postfix) with ESMTP id B9E3D500213; Mon, 21 Oct 2019 11:28:12 -0500 (CDT) Received-X: from mh2.mail.rice.edu (localhost.localdomain [127.0.0.1]) by mh2.mail.rice.edu (Postfix) with ESMTP id B8AB95001F0; Mon, 21 Oct 2019 11:28:12 -0500 (CDT) X-Virus-Scanned: by amavis-2.7.0 at mh2.mail.rice.edu, auth channel Received-X: from mh2.mail.rice.edu ([127.0.0.1]) by mh2.mail.rice.edu (mh2.mail.rice.edu [127.0.0.1]) (amavis, port 10026) with ESMTP id Z_Zo-aAm30zb; Mon, 21 Oct 2019 11:28:12 -0500 (CDT) Received: from cslinux29.cs.rice.edu (cslinux29.cs.rice.edu [168.7.116.104]) (using TLSv1.2 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) (Authenticated sender: jma14) by mh2.mail.rice.edu (Postfix) with ESMTPSA id 891B85001C8; Mon, 21 Oct 2019 11:28:12 -0500 (CDT) Date: Mon, 21 Oct 2019 16:28:00 -0000 From: Jonathon Anderson Subject: Re: [PATCH 2/3] libdw: Rewrite the memory handler to be thread-safe. To: Mark Wielaard Cc: elfutils-devel@sourceware.org, Srdan Milakovic Message-Id: <1571675292.2151.2@rice.edu> In-Reply-To: <0b22cc7764f6486e7e423c0aa8dc8571dddb9fd5.camel@klomp.org> References: <1566877968.10901.0@smtp.mail.rice.edu> <20190829131614.18190-1-mark@klomp.org> <20190829131614.18190-3-mark@klomp.org> <0b22cc7764f6486e7e423c0aa8dc8571dddb9fd5.camel@klomp.org> X-Mailer: geary/3.34.1 MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.95,1.0.8 definitions=2019-10-21_04:2019-10-21,2019-10-21 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 suspectscore=2 malwarescore=0 impostorscore=0 phishscore=0 spamscore=0 mlxscore=0 clxscore=1011 priorityscore=1501 bulkscore=0 mlxlogscore=256 adultscore=0 lowpriorityscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-1908290000 definitions=main-1910210156 Content-Type: text/plain; charset=windows-1251; format=flowed Content-Transfer-Encoding: quoted-printable X-SW-Source: 2019-q4/txt/msg00026.txt.bz2 On Mon, Oct 21, 2019 at 18:13, Mark Wielaard wrote: > Hi, >=20 > Finally back to this patch series. >=20 > On Thu, 2019-08-29 at 15:16 +0200, Mark Wielaard wrote: >> diff --git a/libdw/dwarf_end.c b/libdw/dwarf_end.c >> index 29795c10..fc573cb3 100644 >> --- a/libdw/dwarf_end.c >> +++ b/libdw/dwarf_end.c >> @@ -94,14 +94,15 @@ dwarf_end (Dwarf *dwarf) >> /* And the split Dwarf. */ >> tdestroy (dwarf->split_tree, noop_free); >>=20 >> - struct libdw_memblock *memp =3D dwarf->mem_tail; >> - /* The first block is allocated together with the Dwarf=20 >> object. */ >> - while (memp->prev !=3D NULL) >> + /* Free the internally allocated memory. */ >> + struct libdw_memblock *memp =3D (struct libdw_memblock=20 >> *)dwarf->mem_tail; >> + while (memp !=3D NULL) >> { >> struct libdw_memblock *prevp =3D memp->prev; >> free (memp); >> memp =3D prevp; >> } >> + pthread_key_delete (dwarf->mem_key); >>=20 >> /* Free the pubnames helper structure. */ >> free (dwarf->pubnames_sets); >=20 > This doesn't build on older GCCs (I am using 4.8.5) with this compile=20 > error: >=20 >=20 > libdw/dwarf_end.c: In function =91dwarf_end=92: > libdw/dwarf_end.c:98:45: error: cannot convert to a pointer type > struct libdw_memblock *memp =3D (struct libdw_memblock=20 > *)dwarf->mem_tail; > ^ Ah, whoops. Thanks for catching that one. >=20 > This is because mem_tail is defined as: >=20 >> diff --git a/libdw/libdwP.h b/libdw/libdwP.h >> index eebb7d12..ad2599eb 100644 >> --- a/libdw/libdwP.h >> +++ b/libdw/libdwP.h >> @@ -218,16 +231,11 @@ struct Dwarf >> /* Similar for addrx/constx, which will come from .debug_addr=20 >> section. */ >> struct Dwarf_CU *fake_addr_cu; >>=20 >> - /* Internal memory handling. This is basically a simplified >> - reimplementation of obstacks. Unfortunately the standard=20 >> obstack >> - implementation is not usable in libraries. */ >> - struct libdw_memblock >> - { >> - size_t size; >> - size_t remaining; >> - struct libdw_memblock *prev; >> - char mem[0]; >> - } *mem_tail; >> + /* Internal memory handling. Each thread allocates separately=20 >> and only >> + allocates from its own blocks, while all the blocks are=20 >> pushed atomically >> + onto a unified stack for easy deallocation. */ >> + pthread_key_t mem_key; >> + atomic_uintptr_t mem_tail; >>=20 >> /* Default size of allocated memory blocks. */ >> size_t mem_default_size; >=20 > And for older compilers, without stdatomic.h, this means=20 > atomic_uintptr_t is really: >=20 > typedef _Atomic(uintptr_t) atomic_uintptr_t; > #define _Atomic(T) struct { volatile=20 > __typeof__(T) __val; } >=20 > And you cannot cast a struct to a pointer type directly. >=20 > To make this work on both older and newer gcc versions I changed this=20 > to: >=20 > diff --git a/libdw/dwarf_end.c b/libdw/dwarf_end.c > index fc573cb3..76ab9954 100644 > --- a/libdw/dwarf_end.c > +++ b/libdw/dwarf_end.c > @@ -95,7 +95,8 @@ dwarf_end (Dwarf *dwarf) > tdestroy (dwarf->split_tree, noop_free); >=20 > /* Free the internally allocated memory. */ > - struct libdw_memblock *memp =3D (struct libdw_memblock=20 > *)dwarf->mem_tail; > + struct libdw_memblock *memp; > + memp =3D (struct libdw_memblock *) atomic_load=20 > (&dwarf->mem_tail); > while (memp !=3D NULL) > { > struct libdw_memblock *prevp =3D memp->prev; >=20 > Does that look reasonable? It does, although I would prefer: diff --git a/libdw/dwarf_end.c b/libdw/dwarf_end.c index 9ca17212..6da9e0cd 100644 --- a/libdw/dwarf_end.c +++ b/libdw/dwarf_end.c @@ -95,7 +95,9 @@ dwarf_end (Dwarf *dwarf) tdestroy (dwarf->split_tree, noop_free); /* Free the internally allocated memory. */ - struct libdw_memblock *memp =3D (struct libdw_memblock=20 *)dwarf->mem_tail; + struct libdw_memblock *memp; + memp =3D (struct libdw_memblock *)atomic_load(&dwarf->mem_tail, +=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20= =20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20= =20=20 memory_order_relaxed); while (memp !=3D NULL) { struct libdw_memblock *prevp =3D memp->prev; Because some idiot thought making seq_cst the default was a good idea.=20 And this way it notes in the code that this load is non-synchronizing. -Jonathon >=20 > Thanks, >=20 > Mark