From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.4 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FROM,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 23886 invoked from network); 21 Feb 2022 17:42:39 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 21 Feb 2022 17:42:39 -0000 Received: (qmail 7848 invoked by uid 550); 21 Feb 2022 17:42:36 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 7813 invoked from network); 21 Feb 2022 17:42:36 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net; s=badeba3b8450; t=1645465344; bh=MC1iKvseAkzXZU5Xrpv5F5rW2oKOlR2ozgLFjbB1eC4=; h=X-UI-Sender-Class:Date:From:To:Subject:References:In-Reply-To; b=RyWXp2qXpdi+R898d82e+tQTyiSDpgtrHHuzYPh5WMB0AZT9wQs0+xU/5AMLZG6kC SJuLIBauMsVZwFTmIU2P4Abh3/6QULS8qrK5hy21/rexpmKcX32UWFYGGFYzYCGw1R aJNlGbWHo91bUQmh9JB3JVgqgzO58d0xv0mZxdeo= X-UI-Sender-Class: 01bb95c1-4bf8-414a-932a-4f6e2808ef9c Date: Mon, 21 Feb 2022 18:42:23 +0100 From: Markus Wichmann To: musl@lists.openwall.com Message-ID: <20220221174223.GA2079@voyager> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) X-Provags-ID: V03:K1:aC3F0oz8inhWH7rz9xemxoPjQIfbf3nL3tf7bmveqNjvLZ0mDx0 kjH+IDWetVjLt75T0/eIKEjiFfaXENJcCOdEsHYHA4JxamOg78HK4eFmPjpAhhPOOoE4yic KAzYq0Gqki224V6I5AZd6J06tPJGgoreawg7zk1dBhiiS8hJX2VDY7M9XC+Rh5sl05whfSY mHs89y2pVhztjE2ha1H5A== X-UI-Out-Filterresults: notjunk:1;V03:K0:PX2DyrdT7LM=:XqBJMaBExDE7vTacKVvXQB bXWH+mG7v85RcNGO+WgSFewZ+UBECno6KtKs4zkQWWYfbHOyv/ftcTLeAEWGzHYahL6SYUVKN BR2tG2sLs8C+vVka50T+7FrbE+tI17OD522isrnZcYm+7qUMLXdISAgDvNF+LMFy9eRoOF4Ck ZraXqrvL/wPZPyNlMw9CmOktJTEc7yz0nGhX0lo+zdJPAnYoeVsGHXyJ5tHsCEzj4cA6ghyTi scW51WP2Xu81GrLewtzuVnr7NLSM2wbGUdnBD+mqxtnbBlw7MD+/gxRPsLT6IW0SWxaICGsRl 9EGfifw/8sPeanHY4GHgU6g3nKapynP3jm1LYi1QjJxII3bT9u8+VmvSWm1HFQlNxceF73Wo+ YhonukFjAhhbh3UG+nR29pdldeCQe+8ozvLxIdzxeHdAZytQWOebUj5ibri7Vg6/FKN+tuZRK 1PeBMMczDTlEd/fPhTCv27tR6U+nR+cuKYOa671msMcl9P9EUDD5mgkYLW/9vSN30DbFUcoCC QIZiAQJlNfa9VZqPCJqGRLVpmzP52SYJnh4Wbhjs11GZlZAL0R0xKTK83Gy2QcSfFDezAgrzx q9uAPVmbXBmEveMWE+7w5kcOY/MWVbjPwkHwSR7Hd9xaprk2KSJVxWe4iTR/sZIy9WDVBoiBd 5bJSY9O6lO/lUeWjYCD3Zvf9pDuRQ+RRpvmBudBOdK3c67/ze0m66bzaLMzKX0gDSO5lvVd+o nu/a9xsSuSWPhjdML5RXNOgR4Z2csGK4hZTBXTVWSwzmlnYtbEY8ozWjnk0xjsq2PlFg+QqJ2 ZgN9B4DRVNHxXhHMDD4ZKptkBnUmWHZijsaUQ6GQ25x481xQOcangaMhIkhctn+gvftAVyiHQ GDBaolyZ0X7TaEAM0f8g3nvGubAsGHbXDg14PDq9CH4SfKso6WjfyOuuoP0Pbncr9p/jb4Sww C9rKWxPMlHySQvjRLwvVt24Nq0k3qnpuAuL1GmjXsQGcetE1fbH+3wm81S+Wi5d3L942LrCHX 9SyGtXtiaZNA626peoa5kWTgmPevkZms6USh+xejieS8Zm55b8kHLNr2wXojU5AB2iiYtIX+T O+jTICeRCPG6uI= Content-Transfer-Encoding: quoted-printable Subject: Re: [musl] Suggestion for thread safety On Mon, Feb 21, 2022 at 11:36:06AM +0000, Lee Shallis wrote: > First I'll start with the code snippets I've copied from my own code: > > [...] > > #define LockErrors( ud ) LockStdErr( ud ); LockSysErr( ud ) > #define FreeErrors( ud ) FreeSysErr( ud ); FreeStdErr( ud ) > Style problem: Put a "do {} while (0)" around these, or else you can have unintended effects. The code "if (cond) LockErrors(foo);" will always execute "LockSysErr()". And if you put in complementary code at the bottom of the function, it will always execute "FreeStdErr()", leading to undefined behavior, and good luck finding it. > [...] > dint err; > LOCK_ERRORS > ( > errno =3D 0; > *data =3D *data ? realloc( *data, size ) : malloc( size ); > err =3D *data ? 0 : errno; > ); Why lock anything here? errno is thread local, and the allocation functions must be thread-safe. There is no need to lock anything here. > SHARED_EXP void LockSiData( LOCK **shared, LOCK *ptr ) > { > while ( *shared !=3D ptr ) > { > if ( !(*shared) ) > *shared =3D ptr; > pauseCB( ptr ); > } > } > And now I have to ask what the possible point of this is. Your call to an external function manages to make you avoid the need for a volatile keyword (pauseCB might access global variables, and shared might be pointing at one, so the compiler doesn't know the state of *shared after the return of pauseCB), however, there is probably also some kind of barrier missing. And in practice this is going to burn up a CPU core with completely unnecessary work. After you have established to your satisfaction that the lock is taken, it would be best to suspend the current thread for exactly as long as the lock holder takes to reach the complementary unlock function. You can use futexes for that. Better yet, you can just use a pthread_mutex_t for that, which does all that you need internally. Besides, if "shared" points to a shared variable (as the name suggests), then this constitutes a data race (multiple threads read, some threads write) and is therefore undefined behavior. It also has a race condition beyond that (if one thread is suspended after seeing *shared is NULL, but before setting *shared, another can pass the same check, set *shared, and exit the loop. A function like pthread_yield() doesn't have to do anything useful against that). It looks like you actually want some kind of compare-and-swap function here, but again, you can also just skip it in favor of a pthread_mutex_t. > [...] > > Take what you will of the above, the Allot function was included just > to give you an idea of how the locks are made, as for pauseCB that > should be re-mapped to a variant that calls something like > pthread_yield(), it's what I tested with, anyways the general idea is > that the shared lock defaults to NULL when not locked, when you want > to lock it you 1st wait for it to be NULL then as soon as it's empty > you fill it with your own pointer then wait for other threads who > detected the same situation to fill there pointers into the shared > pointer (assuming any other threads were trying to lock the pointer), > when the wait is over you check again if the pointer is not matching > your own, if not then you failed to lock and should wait longer, using > pointers lends itself well to threading as the pointer being used to > take the shared pointer will normally be unique to the thread (because > rarely will anyone try to use the same pointer in multiple threads for > this process that has a clear example in it's declaration), using > const char * for the LOCK typedef means a thread can give more > information about itself or the pointer it locked with if it so > chooses. > Structure your thoughts into sentences, and your sentences into paragraphs, please. The above is one long run-on sentence, that starts with a point about the Allot function and ends in a justification for the type, and in between has visited many other topics. You are not James Joyce, and you are not writing the Ulysses. Anyway, what I'm still trying to find is the point to all of this. Mutual exclusion is a solved problem, and is in general hard to solve while maintaining some semblance of performance and remaining safe. People with more knowledge than me have poured a lot of thought into libraries that do this, so why not use them? Especially since they are included in the threading implementation already. > [GRIP is a fine-grained lock] Another solved problem. You can use counting semaphores to solve the same problem in a simpler way, at least if I understood correctly. Alternatively, you can use an interesting system of mutexes and conditions, but that typically doesn't end up looking very pretty. Ciao, Markus