Message ID | 20231010134300.53830-2-mark@klomp.org |
---|---|
State | Committed |
Headers |
Return-Path: <elfutils-devel-bounces+patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 9934D3855587 for <patchwork@sourceware.org>; Tue, 10 Oct 2023 13:43:41 +0000 (GMT) X-Original-To: elfutils-devel@sourceware.org Delivered-To: elfutils-devel@sourceware.org Received: from gnu.wildebeest.org (gnu.wildebeest.org [45.83.234.184]) by sourceware.org (Postfix) with ESMTPS id 6F5ED3858C5E for <elfutils-devel@sourceware.org>; Tue, 10 Oct 2023 13:43:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6F5ED3858C5E 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 4FC45301BC17; Tue, 10 Oct 2023 15:43:34 +0200 (CEST) Received: by r6.localdomain (Postfix, from userid 1000) id 137F534013F; Tue, 10 Oct 2023 15:43:34 +0200 (CEST) From: Mark Wielaard <mark@klomp.org> To: elfutils-devel@sourceware.org Cc: hsm2@rice.edu, Mark Wielaard <mark@klomp.org> Subject: [PATCH 02/16] libelf: Make elf_version thread-safe Date: Tue, 10 Oct 2023 15:42:46 +0200 Message-ID: <20231010134300.53830-2-mark@klomp.org> X-Mailer: git-send-email 2.41.0 In-Reply-To: <20231010134300.53830-1-mark@klomp.org> References: <301fac87e83ebbbd677750579ae9a3429b461bdf.camel@klomp.org> <20231010134300.53830-1-mark@klomp.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3033.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, RCVD_IN_BARRACUDACENTRAL, SPF_HELO_NONE, SPF_PASS, 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 X-BeenThere: elfutils-devel@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Elfutils-devel mailing list <elfutils-devel.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/elfutils-devel>, <mailto:elfutils-devel-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/elfutils-devel/> List-Post: <mailto:elfutils-devel@sourceware.org> List-Help: <mailto:elfutils-devel-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/elfutils-devel>, <mailto:elfutils-devel-request@sourceware.org?subject=subscribe> Errors-To: elfutils-devel-bounces+patchwork=sourceware.org@sourceware.org |
Series |
[01/16] lib: Add new once_define and once macros to eu-config.h
|
|
Commit Message
Mark Wielaard
Oct. 10, 2023, 1:42 p.m. UTC
From: Heather McIntyre <hsm2@rice.edu> * elf_version.c (version_once): Define once. (initialize_version): New static function. (elf_version): Use initialize_version version_once. Signed-off-by: Heather S. McIntyre <hsm2@rice.edu> Signed-off-by: Mark Wielaard <mark@klomp.org> --- libelf/elf_version.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
Comments
Hi Heather, On Tue, 2023-10-10 at 15:42 +0200, Mark Wielaard wrote: > From: Heather McIntyre <hsm2@rice.edu> > > * elf_version.c (version_once): Define once. > (initialize_version): New static function. > (elf_version): Use initialize_version version_once. > > Signed-off-by: Heather S. McIntyre <hsm2@rice.edu> > Signed-off-by: Mark Wielaard <mark@klomp.org> > --- > libelf/elf_version.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/libelf/elf_version.c b/libelf/elf_version.c > index 6ec534ab..8296bb65 100644 > --- a/libelf/elf_version.c > +++ b/libelf/elf_version.c > @@ -32,12 +32,21 @@ > #endif > > #include <libelfP.h> > +#include <pthread.h> > > +/* Multiple threads may initialize __libelf_version. > + pthread_once() ensures that __libelf_version is initialized only once. */ > +once_define(static, version_once); > > /* Currently selected version. Should be EV_CURRENT. > Will be EV_NONE if elf_version () has not been called yet. */ > unsigned int __libelf_version = EV_NONE; > > +static void initialize_version(void) > +{ > + __libelf_version = EV_CURRENT; > +} > + > unsigned int > elf_version (unsigned int version) > { > @@ -49,7 +58,7 @@ elf_version (unsigned int version) > /* Phew, we know this version. */ > > /* Signal that the version is now initialized. */ > - __libelf_version = EV_CURRENT; > + once(version_once, initialize_version); > > /* And return the last (or initial) version. */ > return EV_CURRENT; This is an odd function. The intention clearly was to support more "ELF versions" at some point. But (luckily) that never happened and I doubt there will ever be a different (incompatible) ELF version that we'll have to support. So in the end this will always be EV_CURRENT == 1. But the function has to be called to make the rest of the library work. I think this works and is fine. There will most likely never be real contention calling elf_version because normally a program just calls it once at the start. But have you thought about using some atomic operation here instead? Cheers, Mark
John and I discussed that atomic_compare_exchange_strong could have been used here. I see that this has been pushed to the main branch, but I can make the change to the atomic operation if you think that is a better option. Best, Heather On Tue, Oct 10, 2023 at 9:00 AM Mark Wielaard <mark@klomp.org> wrote: > Hi Heather, > > On Tue, 2023-10-10 at 15:42 +0200, Mark Wielaard wrote: > > From: Heather McIntyre <hsm2@rice.edu> > > > > * elf_version.c (version_once): Define once. > > (initialize_version): New static function. > > (elf_version): Use initialize_version version_once. > > > > Signed-off-by: Heather S. McIntyre <hsm2@rice.edu> > > Signed-off-by: Mark Wielaard <mark@klomp.org> > > --- > > libelf/elf_version.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/libelf/elf_version.c b/libelf/elf_version.c > > index 6ec534ab..8296bb65 100644 > > --- a/libelf/elf_version.c > > +++ b/libelf/elf_version.c > > @@ -32,12 +32,21 @@ > > #endif > > > > #include <libelfP.h> > > +#include <pthread.h> > > > > +/* Multiple threads may initialize __libelf_version. > > + pthread_once() ensures that __libelf_version is initialized only > once. */ > > +once_define(static, version_once); > > > > /* Currently selected version. Should be EV_CURRENT. > > Will be EV_NONE if elf_version () has not been called yet. */ > > unsigned int __libelf_version = EV_NONE; > > > > +static void initialize_version(void) > > +{ > > + __libelf_version = EV_CURRENT; > > +} > > + > > unsigned int > > elf_version (unsigned int version) > > { > > @@ -49,7 +58,7 @@ elf_version (unsigned int version) > > /* Phew, we know this version. */ > > > > /* Signal that the version is now initialized. */ > > - __libelf_version = EV_CURRENT; > > + once(version_once, initialize_version); > > > > /* And return the last (or initial) version. */ > > return EV_CURRENT; > > This is an odd function. The intention clearly was to support more "ELF > versions" at some point. But (luckily) that never happened and I doubt > there will ever be a different (incompatible) ELF version that we'll > have to support. So in the end this will always be EV_CURRENT == 1. But > the function has to be called to make the rest of the library work. > > I think this works and is fine. There will most likely never be real > contention calling elf_version because normally a program just calls it > once at the start. > > But have you thought about using some atomic operation here instead? > > Cheers, > > Mark >
Hi Heather, On Tue, Oct 17, 2023 at 02:05:58PM -0500, Heather McIntyre wrote: > John and I discussed that atomic_compare_exchange_strong could have been > used here. I see that this has been pushed to the main branch, but I can > make the change to the atomic operation if you think that is a better > option. It probably doesn't matter for this function since it isn't used often. I was just wondering whether you thought of using atomics here since there are really just two states here (__libelf_version == EV_NONE or __libelf_version == EV_CURRENT). I do have one question though. __libelf_version is checked in elf_begin. Should that check be guarded by a lock too? Thanks, Mark
diff --git a/libelf/elf_version.c b/libelf/elf_version.c index 6ec534ab..8296bb65 100644 --- a/libelf/elf_version.c +++ b/libelf/elf_version.c @@ -32,12 +32,21 @@ #endif #include <libelfP.h> +#include <pthread.h> +/* Multiple threads may initialize __libelf_version. + pthread_once() ensures that __libelf_version is initialized only once. */ +once_define(static, version_once); /* Currently selected version. Should be EV_CURRENT. Will be EV_NONE if elf_version () has not been called yet. */ unsigned int __libelf_version = EV_NONE; +static void initialize_version(void) +{ + __libelf_version = EV_CURRENT; +} + unsigned int elf_version (unsigned int version) { @@ -49,7 +58,7 @@ elf_version (unsigned int version) /* Phew, we know this version. */ /* Signal that the version is now initialized. */ - __libelf_version = EV_CURRENT; + once(version_once, initialize_version); /* And return the last (or initial) version. */ return EV_CURRENT;