Feedback-ID: ia2694551:Fastmail
Date: Mon, 1 Jul 2024 21:12:25 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Andres Freund <andres@anarazel.de>
Cc: Alexander Lakhin <exclusion@gmail.com>, 
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, Jeff Davis <pgsql@j-davis.com>, 
	pgsql-hackers@lists.postgresql.org, Jaime Casanova <jcasanov@systemguards.com.ec>
Subject: Re: LogwrtResult contended spinlock
Message-ID: <202407011912.diw4rf4rgd76@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240701171703.in2bvoc4f5abyjdd@alap3.anarazel.de>
Archived-At: <https://www.postgresql.org/message-id/202407011912.diw4rf4rgd76%40alvherre.pgsql>
Precedence: bulk

Hello,

Thanks for your attention here.

On 2024-Jul-01, Andres Freund wrote:

> On 2024-07-01 11:10:24 +0200, Alvaro Herrera wrote:
> > 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.
> 
> If a platform wants to support 8 byte atomics, it better provides a way to
> make variables 8 bytes aligned. We already rely on that, actually. See use of
> pg_attribute_aligned in e.g. src/include/port/atomics/generic-msvc.h

Well, pg_atomic_uint64 is a struct. Passing pointers to it is fine,
which is what non-platform-specific code does; but because the
declaration of the type is in each platform-specific file, it might not
work to use it directly in generic code.  I didn't actually try, but it
seems a bit of a layering violation.  (I didn't find any place where
the struct is used that way.)

If that works, then I think we could simply declare currval as a
pg_atomic_uint64 and it'd be prettier.

> > +	/*
> > +	 * 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;
> 
> I wonder if we should just relax the alignment requirement for currval. It's
> crucial that the pointer is atomically aligned (atomic ops across pages are
> either forbidden or extremely slow), but it's far from obvious that it's
> crucial for comparator value to be aligned.

I'm pretty sure the Microsoft docs I linked to are saying it must be
aligned.

> >  #ifndef PG_HAVE_ATOMIC_U64_SIMULATION
> >  	AssertPointerAlignment(ptr, 8);
> >  #endif
> 
> What's the point of this assert, btw? This stuff is already asserted in lower
> level routines, so it just seems redundant to have it here?

There are in some of them, but not in pg_atomic_compare_exchange_u64_impl.


> > -	return Max(target_, currval);
> > +	return Max(target_, currval.u64);
> 
> What does the Max() actually achieve here? Shouldn't it be impossible to reach
> this with  currval < target_?

When two processes are hitting the cmpxchg concurrently, we need to
return the highest value that was written, even if it was the other
process that did it.  The assertions in the calling site are quickly
broken if we don't do this.  I admit this aspect took me by surprise.

> And why does target_ end in an underscore?

Heh, you tell me -- I just copied the style of the functions just above.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"No nos atrevemos a muchas cosas porque son difíciles,
pero son difíciles porque no nos atrevemos a hacerlas" (Séneca)