Feedback-ID: ia2694551:Fastmail
Date: Mon, 1 Jul 2024 11:10:24 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Alexander Lakhin <exclusion@gmail.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, 
	Tom Lane <tgl@sss.pgh.pa.us>, Andres Freund <andres@anarazel.de>, 
	Jeff Davis <pgsql@j-davis.com>, pgsql-hackers@lists.postgresql.org, 
	Jaime Casanova <jcasanov@systemguards.com.ec>
Subject: Re: LogwrtResult contended spinlock
Message-ID: <202407010910.ktykqkc52ffb@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="fe7nlxqhseziub3f"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <4244f22a-a208-321f-ec32-150ffde35c8e@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/202407010910.ktykqkc52ffb%40alvherre.pgsql>
Precedence: bulk


--fe7nlxqhseziub3f
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Hello,

On 2024-Jun-29, Alexander Lakhin wrote:

> It doesn't, but the following works for me:
>  static inline uint64
>  pg_atomic_monotonic_advance_u64(volatile pg_atomic_uint64 *ptr, uint64 target_)
>  {
> -       uint64          currval;
> +       pg_attribute_aligned(8) uint64          currval;
> 
> because the failed assertion is:
> #ifndef PG_HAVE_ATOMIC_U64_SIMULATION
>     AssertPointerAlignment(&currval, 8);
> #endif

Hah, thank you.

In the meantime I noticed that pg_attribute_aligned() is not supported
in every platform/compiler, so for safety sake I think it's better to go
with what we do for PGAlignedBlock: use a union with a double member.
That should be 8-byte aligned on x86 as well, unless I misunderstand.

BTW if things works after this fix, I suggest you get a buildfarm member
running with this configuration.  Otherwise it's quite likely that we'll
break it again.  Or we could just decide we don't care about this
particular platform ...  but AFAICS the buildfarm does have other 32-bit
animals running.

[ looks at buildfarm ]

Oh!  while looking at Adder's config, I noticed this line:
Checking for alignment of "double" : 4 
So, I do misunderstand doubles.

So my patch is not going to work.  There seems to be nothing that has
alignment 8 there, so I guess we're back to using
pg_attribute_aligned() and abort the compile if that doesn't exist.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"La gente vulgar sólo piensa en pasar el tiempo;
el que tiene talento, en aprovecharlo"

--fe7nlxqhseziub3f
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="v2-0001-Fix-alignment-of-variable-in-pg_atomic_monotonic_.patch"

From 9d240e90f87bf8b53bd5d92b623e33419db64717 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Mon, 1 Jul 2024 10:41:06 +0200
Subject: [PATCH v2] Fix alignment of variable in
 pg_atomic_monotonic_advance_u64

Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/36796438-a718-cf9b-2071-b2c1b947c1b5@gmail.com
---
 src/include/port/atomics.h | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/src/include/port/atomics.h b/src/include/port/atomics.h
index 78987f3154..964732e660 100644
--- a/src/include/port/atomics.h
+++ b/src/include/port/atomics.h
@@ -580,30 +580,38 @@ pg_atomic_sub_fetch_u64(volatile pg_atomic_uint64 *ptr, int64 sub_)
 static inline uint64
 pg_atomic_monotonic_advance_u64(volatile pg_atomic_uint64 *ptr, uint64 target_)
 {
-	uint64		currval;
+	/*
+	 * On 32-bit machines, declaring a bare uint64 variable doesn't promise
+	 * the alignment we need, so coerce the compiler this way.
+	 */
+	union
+	{
+		uint64		u64;
+		double		force_align_d;
+	}			currval;
 
 #ifndef PG_HAVE_ATOMIC_U64_SIMULATION
 	AssertPointerAlignment(ptr, 8);
 #endif
 
-	currval = pg_atomic_read_u64_impl(ptr);
-	if (currval >= target_)
+	currval.u64 = pg_atomic_read_u64_impl(ptr);
+	if (currval.u64 >= target_)
 	{
 		pg_memory_barrier();
-		return currval;
+		return currval.u64;
 	}
 
 #ifndef PG_HAVE_ATOMIC_U64_SIMULATION
-	AssertPointerAlignment(&currval, 8);
+	AssertPointerAlignment(&currval.u64, 8);
 #endif
 
-	while (currval < target_)
+	while (currval.u64 < target_)
 	{
-		if (pg_atomic_compare_exchange_u64_impl(ptr, &currval, target_))
+		if (pg_atomic_compare_exchange_u64_impl(ptr, &currval.u64, target_))
 			break;
 	}
 
-	return Max(target_, currval);
+	return Max(target_, currval.u64);
 }
 
 #undef INSIDE_ATOMICS_H
-- 
2.39.2


--fe7nlxqhseziub3f--