From nobody Thu Feb 22 04:27:08 2024 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4TgKqT2zxgz5Bnx3; Thu, 22 Feb 2024 04:27:17 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4TgKqS2snLz46pv; Thu, 22 Feb 2024 04:27:16 +0000 (UTC) (envelope-from kostikbel@gmail.com) Authentication-Results: mx1.freebsd.org; none Received: from tom.home (kib@localhost [127.0.0.1] (may be forged)) by kib.kiev.ua (8.18.1/8.18.1) with ESMTP id 41M4R8vF065353; Thu, 22 Feb 2024 06:27:12 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua 41M4R8vF065353 Received: (from kostik@localhost) by tom.home (8.18.1/8.18.1/Submit) id 41M4R83Y065352; Thu, 22 Feb 2024 06:27:08 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Thu, 22 Feb 2024 06:27:08 +0200 From: Konstantin Belousov To: Jessica Clarke Cc: "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" Subject: Re: git: 8271d9b99a3b - main - libsys: remove usage of pthread_once and _once_stub Message-ID: References: <202402210029.41L0TOH5000231@gitrepo.freebsd.org> <964A29A2-4C51-4037-8EBE-320008D48AE0@freebsd.org> <4715B319-B7DE-4D06-9F27-00CFE5AF89A7@freebsd.org> List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Spam-Status: No, score=-1.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FORGED_GMAIL_RCVD,FREEMAIL_FROM, NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=4.0.0 X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-14) on tom.home X-Spamd-Bar: ---- X-Rspamd-Queue-Id: 4TgKqS2snLz46pv X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:6939, ipnet:2001:470::/32, country:US] On Thu, Feb 22, 2024 at 01:23:08AM +0000, Jessica Clarke wrote: > On 22 Feb 2024, at 01:11, Konstantin Belousov wrote: > > > > On Wed, Feb 21, 2024 at 05:23:10PM +0000, Jessica Clarke wrote: > >> On 21 Feb 2024, at 14:17, Konstantin Belousov wrote: > >>> > >>> On Wed, Feb 21, 2024 at 12:51:04AM +0000, Jessica Clarke wrote: > >>>> On 21 Feb 2024, at 00:29, Konstantin Belousov wrote: > >>>>> > >>>>> The branch main has been updated by kib: > >>>>> > >>>>> URL: https://cgit.FreeBSD.org/src/commit/?id=8271d9b99a3b98c662ee9a6257a144284b7e1728 > >>>>> > >>>>> commit 8271d9b99a3b98c662ee9a6257a144284b7e1728 > >>>>> Author: Konstantin Belousov > >>>>> AuthorDate: 2024-02-20 14:45:29 +0000 > >>>>> Commit: Konstantin Belousov > >>>>> CommitDate: 2024-02-21 00:26:11 +0000 > >>>>> > >>>>> libsys: remove usage of pthread_once and _once_stub > >>>>> > >>>>> that existed in auxv.c, use simple bool gate instead. This leaves a > >>>>> small window if two threads try to call _elf_aux_info(3) simultaneously. > >>>>> The situation is safe because auxv parsing is really idempotent. The > >>>>> parsed data is the same, and we store atomic types (int/long/ptr) so > >>>>> double-init does not matter. > >>>> > >>>> You still need to load acquire and store release aux_once though, > >>>> otherwise you can see aux_once as true yet read the pre-initialised > >>>> data. In practice that’s surely very hard to hit, but the code as > >>>> written is now wrong. Also, idempotence should probably be made > >>>> unnecessary by using 0/1/2 state for uninitialised/initialising/ > >>>> initialised, as it’s still technically UB from a C AM perspective due > >>>> to not being data race free if two threads initialise at the same time. > >>>> Better to just do the correct thing rather than risk things going wrong. > >>> > >>> There is too much to handle 'in process' state for loosing thread, I need > >>> the whole libthr machinery. > >> > >> What do you need libthr for? In pseudo-C: > >> > >> x = load_acquire(&aux_once) > >> if (__predict_true(x == 2)) > >> return; > >> if (x == 1 || !compare_exchange_strong_acquire(&aux_once, &x, 1)) { > >> while (x != 2) { > >> yield(); > >> x = load_acquire(&aux_once) > >> } > >> return; > >> } > >> /* initialise as before */ > >> store_release(&aux_once, 2); > >> > >> I believe that’s all you need. Or compare exchange 0 to 1 as the > >> initial operation; makes the source code shorter at the expense of a > >> more expensive fast path: > >> > >> x = 0; > >> if (__predict_true(!compare_exchange_strong_acquire(&aux_once, &x, 1)) { > >> while (__predict_false(x != 2)) { > >> yield(); > >> x = load_acquire(&aux_once) > >> } > >> return; > >> } > >> /* initialise as before */ > >> store_release(&aux_once, 2); > >> > >> I probably have bugs in the above, but you get the gist. > > The bug in the fragment above is with the yield(). If low-priority thread > > enters the '1' (in progress) block, and then is preempted by high-priority > > thread also entering init_auxv(), the process would never make a progress. > > > > This is why I said that we need libthr (or umtx), to use real locking and > > move the waiting thread off cpu. In kernel, yield can be used in similar > > situations because we can bump the priority, although it is tricky. > > Yes, priority inversion is an issue here. But this is (without all the > C++ abstraction) how libcxxrt implements __cxa_guard_acquire, so if > it’s good enough for C++ constructors for static storage duration > objects declared at local scope, surely it’s also good enough for > aux_once? And if it’s not good enough for aux_once then libcxxrt should > be deemed broken... > > One could easily adapt the above to use UMTX_OP_WAIT/WAKE though. As I noted, umtx(2) should be the solution. Might be we should provide some park/unpark interface in libc, to hide the umtx interface. Its use started to proliferate recently. > > Jess > > >>> I added the fences, thanks for noting. > >> > >> Thanks. > >> > >> Jess > >> > >>> WRT being UB from pure C, we already have much more assumptions about > >>> atomicity. > >