Date: Mon, 14 Aug 2023 08:36:34 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Michail Nikolaev <michail.nikolaev@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
	Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: Replace known_assigned_xids_lck by memory barrier
Message-ID: <20230814153634.GB1395427@nathanxps13>
References: <CANtu0oh0si=jG5z_fLeFtmYcETssQ08kLEa8b6TQqDm_cinroA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CANtu0oh0si=jG5z_fLeFtmYcETssQ08kLEa8b6TQqDm_cinroA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/20230814153634.GB1395427%40nathanxps13>
Precedence: bulk

On Sun, Mar 19, 2023 at 12:43:43PM +0300, Michail Nikolaev wrote:
> In a nutshell: KnownAssignedXids as well as the head/tail pointers are
> modified only by the startup process, so spinlock is used to ensure
> that updates of the array and head/tail pointers are seen in a correct
> order. It is enough to pass the barrier after writing to the array
> (but before updating the pointers) to achieve the same result.

What sort of benefits do you see from this patch?  It might be worthwhile
in itself to remove spinlocks when possible, but IME it's much easier to
justify such changes when there is a tangible benefit we can point to.

 	/*
-	 * Now update the head pointer.  We use a spinlock to protect this
+	 * Now update the head pointer.  We use a memory barrier to protect this
 	 * pointer, not because the update is likely to be non-atomic, but to
 	 * ensure that other processors see the above array updates before they
 	 * see the head pointer change.
 	 *
 	 * If we're holding ProcArrayLock exclusively, there's no need to take the
-	 * spinlock.
+	 * barrier.
 	 */

Are the assignments in question guaranteed to be atomic?  IIUC we assume
that aligned 4-byte loads/stores are atomic, so we should be okay as long
as we aren't handling anything larger.

-		SpinLockAcquire(&pArray->known_assigned_xids_lck);
+		pg_write_barrier();
 		pArray->headKnownAssignedXids = head;
-		SpinLockRelease(&pArray->known_assigned_xids_lck);

This use of pg_write_barrier() looks correct to me, but don't we need
corresponding read barriers wherever we obtain the pointers?  FWIW I tend
to review src/backend/storage/lmgr/README.barrier in its entirety whenever
I deal with this stuff.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com