Thread: typo in fallback implementation for pg_atomic_test_set_flag()
I noticed that the fallback pg_atomic_test_set_flag_impl() implementation that uses atomic-exchange is giving pg_atomic_exchange_u32_impl() an extra argument. This appears to be copy/pasted from the atomic-compare-exchange version a few lines down. It looks like it's been this way since this code was introduced in commit b64d92f (2014). Patch attached. I'd ordinarily suggest removing this section of code since it doesn't seem to have gotten much coverage, but I'm actually looking into adding some faster atomic-exchange implementations that may activate this code for certain compiler/architecture combinations. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
Hi, On 2023-11-13 21:54:39 -0600, Nathan Bossart wrote: > I noticed that the fallback pg_atomic_test_set_flag_impl() implementation > that uses atomic-exchange is giving pg_atomic_exchange_u32_impl() an extra > argument. This appears to be copy/pasted from the atomic-compare-exchange > version a few lines down. It looks like it's been this way since this code > was introduced in commit b64d92f (2014). Patch attached. Oops. I guess it's not too surprising this wasn't required - if the compiler has any atomic intrinsics it's going to have support for the flag stuff. And there's practically no compiler that Are you planning to apply the fix? > I'd ordinarily suggest removing this section of code since it doesn't seem > to have gotten much coverage Which section precisely? > but I'm actually looking into adding some faster atomic-exchange > implementations that may activate this code for certain > compiler/architecture combinations. Hm. I don't really see how adding a faster atomic-exchange implementation could trigger this implementation being used? Greetings, Andres Freund
On Tue, Nov 14, 2023 at 07:17:32PM -0800, Andres Freund wrote:
> Are you planning to apply the fix?
Yes, I'll take care of it.
>> I'd ordinarily suggest removing this section of code since it doesn't seem
>> to have gotten much coverage
>
> Which section precisely?
The lines below this:
/*
* provide fallback for test_and_set using atomic_exchange if available
*/
#if !defined(PG_HAVE_ATOMIC_TEST_SET_FLAG) && defined(PG_HAVE_ATOMIC_EXCHANGE_U32)
but above this:
/*
* provide fallback for test_and_set using atomic_compare_exchange if
* available.
*/
#elif !defined(PG_HAVE_ATOMIC_TEST_SET_FLAG) && defined(PG_HAVE_ATOMIC_COMPARE_EXCHANGE_U32)
>> but I'm actually looking into adding some faster atomic-exchange
>> implementations that may activate this code for certain
>> compiler/architecture combinations.
>
> Hm. I don't really see how adding a faster atomic-exchange implementation
> could trigger this implementation being used?
That'd define PG_HAVE_ATOMIC_EXCHANGE_U32, so this fallback might be used
if PG_HAVE_ATOMIC_TEST_SET_FLAG is not defined. I haven't traced through
all the #ifdefs that lead to this point exhaustively, though, so perhaps
this is still unlikely.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Wed, Nov 15, 2023 at 09:52:34AM -0600, Nathan Bossart wrote: > On Tue, Nov 14, 2023 at 07:17:32PM -0800, Andres Freund wrote: >> Are you planning to apply the fix? > > Yes, I'll take care of it. Committed and back-patched. I probably could've skipped back-patching this one since it doesn't seem to be causing any problems yet, but I didn't see any reason not to back-patch, either. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com