Re: [HACKERS] Obsolete use of volatile in walsender.c, walreceiver.c, walreceiverfuncs.c?
On Sat, Oct 17, 2015 at 3:21 AM, Robert Haas wrote: > OK, committed his, and yours. > > I back-patched his spin.h comment fix to 9.5 since that's a factual > error, but the rest of this seems like optimization so I committed it > only to master. That sounds right. Thanks! -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Obsolete use of volatile in walsender.c, walreceiver.c, walreceiverfuncs.c?
On Thu, Oct 15, 2015 at 10:36 PM, Michael Paquier wrote: > On Fri, Oct 16, 2015 at 9:07 AM, Thomas Munro > wrote: >> On Wed, Oct 7, 2015 at 8:52 AM, Robert Haas wrote: >>> On Thu, Oct 1, 2015 at 11:01 PM, Michael Paquier >>> wrote: > Right, see attached. It seems to me that we could as well simplify checkpoint.c and logical.c. In those files volatile casts are used as well to protect from reordering for spinlock operations. See for example 0002 on top of 0001 that is Thomas' patch. >>> >>> These patches look good to me, so I have committed them. >> >> Thanks. Also, spin.h's comment contains an out of date warning about >> this. Here's a suggested fix for that, and a couple more volatrivia >> patches. > > I have looked at the rest of the code, and it seems that we can get > rid of volatile in a couple of extra places like in the attached as > those are used with spin locks. This applies on top of Thomas' set. OK, committed his, and yours. I back-patched his spin.h comment fix to 9.5 since that's a factual error, but the rest of this seems like optimization so I committed it only to master. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Obsolete use of volatile in walsender.c, walreceiver.c, walreceiverfuncs.c?
On Fri, Oct 16, 2015 at 9:07 AM, Thomas Munro
wrote:
> On Wed, Oct 7, 2015 at 8:52 AM, Robert Haas wrote:
>> On Thu, Oct 1, 2015 at 11:01 PM, Michael Paquier
>> wrote:
Right, see attached.
>>>
>>> It seems to me that we could as well simplify checkpoint.c and
>>> logical.c. In those files volatile casts are used as well to protect
>>> from reordering for spinlock operations. See for example 0002 on top
>>> of 0001 that is Thomas' patch.
>>
>> These patches look good to me, so I have committed them.
>
> Thanks. Also, spin.h's comment contains an out of date warning about
> this. Here's a suggested fix for that, and a couple more volatrivia
> patches.
I have looked at the rest of the code, and it seems that we can get
rid of volatile in a couple of extra places like in the attached as
those are used with spin locks. This applies on top of Thomas' set.
--
Michael
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index b13fe03..0dc4117 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -3718,8 +3718,6 @@ static int
KnownAssignedXidsGetAndSetXmin(TransactionId *xarray, TransactionId *xmin,
TransactionId xmax)
{
- /* use volatile pointer to prevent code rearrangement */
- volatile ProcArrayStruct *pArray = procArray;
int count = 0;
int head,
tail;
@@ -3734,10 +3732,10 @@ KnownAssignedXidsGetAndSetXmin(TransactionId *xarray, TransactionId *xmin,
*
* Must take spinlock to ensure we see up-to-date array contents.
*/
- SpinLockAcquire(&pArray->known_assigned_xids_lck);
- tail = pArray->tailKnownAssignedXids;
- head = pArray->headKnownAssignedXids;
- SpinLockRelease(&pArray->known_assigned_xids_lck);
+ SpinLockAcquire(&procArray->known_assigned_xids_lck);
+ tail = procArray->tailKnownAssignedXids;
+ head = procArray->headKnownAssignedXids;
+ SpinLockRelease(&procArray->known_assigned_xids_lck);
for (i = tail; i < head; i++)
{
@@ -3777,8 +3775,6 @@ KnownAssignedXidsGetAndSetXmin(TransactionId *xarray, TransactionId *xmin,
static TransactionId
KnownAssignedXidsGetOldestXmin(void)
{
- /* use volatile pointer to prevent code rearrangement */
- volatile ProcArrayStruct *pArray = procArray;
int head,
tail;
int i;
@@ -3786,10 +3782,10 @@ KnownAssignedXidsGetOldestXmin(void)
/*
* Fetch head just once, since it may change while we loop.
*/
- SpinLockAcquire(&pArray->known_assigned_xids_lck);
- tail = pArray->tailKnownAssignedXids;
- head = pArray->headKnownAssignedXids;
- SpinLockRelease(&pArray->known_assigned_xids_lck);
+ SpinLockAcquire(&procArray->known_assigned_xids_lck);
+ tail = procArray->tailKnownAssignedXids;
+ head = procArray->headKnownAssignedXids;
+ SpinLockRelease(&procArray->known_assigned_xids_lck);
for (i = tail; i < head; i++)
{
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 2c2535b..bb10c1b 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -283,15 +283,13 @@ InitProcGlobal(void)
void
InitProcess(void)
{
- /* use volatile pointer to prevent code rearrangement */
- volatile PROC_HDR *procglobal = ProcGlobal;
PGPROC * volatile * procgloballist;
/*
* ProcGlobal should be set up already (if we are a backend, we inherit
* this by fork() or EXEC_BACKEND mechanism from the postmaster).
*/
- if (procglobal == NULL)
+ if (ProcGlobal == NULL)
elog(PANIC, "proc header uninitialized");
if (MyProc != NULL)
@@ -299,11 +297,11 @@ InitProcess(void)
/* Decide which list should supply our PGPROC. */
if (IsAnyAutoVacuumProcess())
- procgloballist = &procglobal->autovacFreeProcs;
+ procgloballist = &ProcGlobal->autovacFreeProcs;
else if (IsBackgroundWorker)
- procgloballist = &procglobal->bgworkerFreeProcs;
+ procgloballist = &ProcGlobal->bgworkerFreeProcs;
else
- procgloballist = &procglobal->freeProcs;
+ procgloballist = &ProcGlobal->freeProcs;
/*
* Try to get a proc struct from the appropriate free list. If this
@@ -314,7 +312,7 @@ InitProcess(void)
*/
SpinLockAcquire(ProcStructLock);
- set_spins_per_delay(procglobal->spins_per_delay);
+ set_spins_per_delay(ProcGlobal->spins_per_delay);
MyProc = *procgloballist;
@@ -578,13 +576,10 @@ InitAuxiliaryProcess(void)
void
PublishStartupProcessInformation(void)
{
- /* use volatile pointer to prevent code rearrangement */
- volatile PROC_HDR *procglobal = ProcGlobal;
-
SpinLockAcquire(ProcStructLock);
- procglobal->startupProc = MyProc;
- procglobal->startupProcPid = MyProcPid;
+ ProcGlobal->startupProc = MyProc;
+ ProcGlobal->startupProcPid = MyProcPid;
SpinLockRelease(ProcStructLock);
}
@@ -627,12 +622,9 @@ HaveNFreeProcs(int n)
{
PGPROC *proc;
- /* use volatile pointer to prevent code rearrangement */
- volatile PROC_HDR *procglobal = ProcGlobal;
-
SpinLockAcquire(ProcStructLock);
- proc = procglobal->freeProcs;
+ proc = ProcGlobal->freeProcs;
while (n > 0 &&
Re: [HACKERS] Obsolete use of volatile in walsender.c, walreceiver.c, walreceiverfuncs.c?
On Wed, Oct 7, 2015 at 8:52 AM, Robert Haas wrote: > On Thu, Oct 1, 2015 at 11:01 PM, Michael Paquier > wrote: >>> Right, see attached. >> >> It seems to me that we could as well simplify checkpoint.c and >> logical.c. In those files volatile casts are used as well to protect >> from reordering for spinlock operations. See for example 0002 on top >> of 0001 that is Thomas' patch. > > These patches look good to me, so I have committed them. Thanks. Also, spin.h's comment contains an out of date warning about this. Here's a suggested fix for that, and a couple more volatrivia patches. -- Thomas Munro http://www.enterprisedb.com spin-header-comment.patch Description: Binary data strip-volatile-hash.patch Description: Binary data strip-volatile-ipc.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Obsolete use of volatile in walsender.c, walreceiver.c, walreceiverfuncs.c?
On Thu, Oct 1, 2015 at 11:01 PM, Michael Paquier wrote: >> Right, see attached. > > It seems to me that we could as well simplify checkpoint.c and > logical.c. In those files volatile casts are used as well to protect > from reordering for spinlock operations. See for example 0002 on top > of 0001 that is Thomas' patch. These patches look good to me, so I have committed them. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Obsolete use of volatile in walsender.c, walreceiver.c, walreceiverfuncs.c?
On Tue, Sep 22, 2015 at 7:25 AM, Thomas Munro
wrote:
> On Tue, Sep 22, 2015 at 8:19 AM, Alvaro Herrera
> wrote:
>> Thomas Munro wrote:
>>
>>> In walsender.c, walreceiver.c, walreceiverfuncs.c there are several
>>> places where volatile qualifiers are used apparently only to prevent
>>> reordering around spinlock operations.
>>
>> In replication/slot.c there are a number of places (12, I think) that
>> introduce a block specifically to contain a volatile cast on a variable
>> for spinlock-protected access. We could remove the whole thing and save
>> at least 3 lines and one indentation level for each of them.
>
> Right, see attached.
It seems to me that we could as well simplify checkpoint.c and
logical.c. In those files volatile casts are used as well to protect
from reordering for spinlock operations. See for example 0002 on top
of 0001 that is Thomas' patch.
--
Michael
From 929db46e249ac3f6e587b38aff8ba4468e7d776d Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Fri, 2 Oct 2015 10:57:15 +0900
Subject: [PATCH 1/2] Remove obsolete use of volatile in WAL-related files
Patch by Thomas Munro.
---
src/backend/replication/slot.c | 100 ++---
src/backend/replication/walreceiver.c | 16 ++---
src/backend/replication/walreceiverfuncs.c | 22 ++-
src/backend/replication/walsender.c| 39 ---
4 files changed, 59 insertions(+), 118 deletions(-)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index c66619c..5b18cb6 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -288,15 +288,11 @@ ReplicationSlotCreate(const char *name, bool db_specific,
slot->in_use = true;
/* We can now mark the slot active, and that makes it our slot. */
- {
- volatile ReplicationSlot *vslot = slot;
-
- SpinLockAcquire(&slot->mutex);
- Assert(vslot->active_pid == 0);
- vslot->active_pid = MyProcPid;
- SpinLockRelease(&slot->mutex);
- MyReplicationSlot = slot;
- }
+ SpinLockAcquire(&slot->mutex);
+ Assert(slot->active_pid == 0);
+ slot->active_pid = MyProcPid;
+ SpinLockRelease(&slot->mutex);
+ MyReplicationSlot = slot;
LWLockRelease(ReplicationSlotControlLock);
@@ -329,12 +325,10 @@ ReplicationSlotAcquire(const char *name)
if (s->in_use && strcmp(name, NameStr(s->data.name)) == 0)
{
- volatile ReplicationSlot *vslot = s;
-
SpinLockAcquire(&s->mutex);
- active_pid = vslot->active_pid;
+ active_pid = s->active_pid;
if (active_pid == 0)
-vslot->active_pid = MyProcPid;
+s->active_pid = MyProcPid;
SpinLockRelease(&s->mutex);
slot = s;
break;
@@ -380,10 +374,8 @@ ReplicationSlotRelease(void)
else
{
/* Mark slot inactive. We're not freeing it, just disconnecting. */
- volatile ReplicationSlot *vslot = slot;
-
SpinLockAcquire(&slot->mutex);
- vslot->active_pid = 0;
+ slot->active_pid = 0;
SpinLockRelease(&slot->mutex);
}
@@ -459,11 +451,10 @@ ReplicationSlotDropAcquired(void)
}
else
{
- volatile ReplicationSlot *vslot = slot;
bool fail_softly = slot->data.persistency == RS_EPHEMERAL;
SpinLockAcquire(&slot->mutex);
- vslot->active_pid = 0;
+ slot->active_pid = 0;
SpinLockRelease(&slot->mutex);
ereport(fail_softly ? WARNING : ERROR,
@@ -533,16 +524,13 @@ ReplicationSlotSave(void)
void
ReplicationSlotMarkDirty(void)
{
+ ReplicationSlot *slot = MyReplicationSlot;
Assert(MyReplicationSlot != NULL);
- {
- volatile ReplicationSlot *vslot = MyReplicationSlot;
-
- SpinLockAcquire(&vslot->mutex);
- MyReplicationSlot->just_dirtied = true;
- MyReplicationSlot->dirty = true;
- SpinLockRelease(&vslot->mutex);
- }
+ SpinLockAcquire(&slot->mutex);
+ MyReplicationSlot->just_dirtied = true;
+ MyReplicationSlot->dirty = true;
+ SpinLockRelease(&slot->mutex);
}
/*
@@ -557,13 +545,9 @@ ReplicationSlotPersist(void)
Assert(slot != NULL);
Assert(slot->data.persistency != RS_PERSISTENT);
- {
- volatile ReplicationSlot *vslot = slot;
-
- SpinLockAcquire(&slot->mutex);
- vslot->data.persistency = RS_PERSISTENT;
- SpinLockRelease(&slot->mutex);
- }
+ SpinLockAcquire(&slot->mutex);
+ slot->data.persistency = RS_PERSISTENT;
+ SpinLockRelease(&slot->mutex);
ReplicationSlotMarkDirty();
ReplicationSlotSave();
@@ -593,14 +577,10 @@ ReplicationSlotsComputeRequiredXmin(bool already_locked)
if (!s->in_use)
continue;
- {
- volatile ReplicationSlot *vslot = s;
-
- SpinLockAcquire(&s->mutex);
- effective_xmin = vslot->effective_xmin;
- effective_catalog_xmin = vslot->effective_catalog_xmin;
- SpinLockRelease(&s->mutex);
- }
+ SpinLockAcquire(&s->mutex);
+ effective_xmin = s->effective_xmin;
+ effective_catalog_xmin = s->effective_catalog_xmin;
+ SpinLockRelease(&s->mutex);
/* check the data xmin */
if (TransactionIdIsValid(effective_xmin) &&
@@ -641,13 +621,9 @@ ReplicationSlotsComputeRequiredLSN(void)
if (!s->in_use)
continue;
- {
- volatile ReplicationSlot *vslot = s;
-
-
Re: [HACKERS] Obsolete use of volatile in walsender.c, walreceiver.c, walreceiverfuncs.c?
Hi, On 2015-09-17 16:32:17 +1200, Thomas Munro wrote: > In walsender.c, walreceiver.c, walreceiverfuncs.c there are several > places where volatile qualifiers are used apparently only to prevent > reordering around spinlock operations. My understanding is that if > potential load/store reordering around spinlock operations is the only > reason for using volatile, 0709b7ee72e4bc71ad07b7120acd117265ab51d0 > made it unnecessary. For example see > 6ba4ecbf477e0b25dd7bde1b0c4e07fc2da19348 which stripped some volatile > qualifiers out of xlog.c. Same in bufmgr.c et al. There it's actually rather annoying for new code because volatile needs to be casted away in a bunch of places... Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Obsolete use of volatile in walsender.c, walreceiver.c, walreceiverfuncs.c?
On Tue, Sep 22, 2015 at 8:19 AM, Alvaro Herrera wrote: > Thomas Munro wrote: > >> In walsender.c, walreceiver.c, walreceiverfuncs.c there are several >> places where volatile qualifiers are used apparently only to prevent >> reordering around spinlock operations. > > In replication/slot.c there are a number of places (12, I think) that > introduce a block specifically to contain a volatile cast on a variable > for spinlock-protected access. We could remove the whole thing and save > at least 3 lines and one indentation level for each of them. Right, see attached. -- Thomas Munro http://www.enterprisedb.com replication-strip-volatile-v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Obsolete use of volatile in walsender.c, walreceiver.c, walreceiverfuncs.c?
Thomas Munro wrote: > In walsender.c, walreceiver.c, walreceiverfuncs.c there are several > places where volatile qualifiers are used apparently only to prevent > reordering around spinlock operations. In replication/slot.c there are a number of places (12, I think) that introduce a block specifically to contain a volatile cast on a variable for spinlock-protected access. We could remove the whole thing and save at least 3 lines and one indentation level for each of them. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Obsolete use of volatile in walsender.c, walreceiver.c, walreceiverfuncs.c?
Hi In walsender.c, walreceiver.c, walreceiverfuncs.c there are several places where volatile qualifiers are used apparently only to prevent reordering around spinlock operations. My understanding is that if potential load/store reordering around spinlock operations is the only reason for using volatile, 0709b7ee72e4bc71ad07b7120acd117265ab51d0 made it unnecessary. For example see 6ba4ecbf477e0b25dd7bde1b0c4e07fc2da19348 which stripped some volatile qualifiers out of xlog.c. I did notice that sometimes walsnd->pid is read without acquiring the spinlock. Is that actually OK anyway (taking a stale and inconsistent view of the contents of walsnd->pid WRT to the other members that are later accessed while holding the spinlock)? Would it be safe to remove all those volatile qualifiers, something like in the attached, or am I missing something? (There is also code in syncrep.c that is reading shmem without acquiring spinlocks using volatile qualifiers, that is a different situation, though I don't yet see how it is ordering sensitive or reading the same object repeatedly, but I'm not talking about that here). -- Thomas Munro http://www.enterprisedb.com replication-strip-volatile.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
