10000 gh-121368: Fix seq lock memory ordering in _PyType_Lookup · python/cpython@9a68c4c · GitHub
[go: up one dir, main page]
Skip to content

Commit 9a68c4c

Browse files
colesburycolesbury
committed
gh-121368: Fix seq lock memory ordering in _PyType_Lookup
1 parent 5f660e8 commit 9a68c4c

File tree

9 files changed

+47
-16
lines changed

9 files changed

+47
-16
lines changed

‎Include/cpython/pyatomic.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -510,6 +510,9 @@ _Py_atomic_load_ssize_acquire(const Py_ssize_t *obj);
510510
// See https://en.cppreference.com/w/cpp/atomic/atomic_thread_fence
511511
static inline void _Py_atomic_fence_seq_cst(void);
512512

513+
// Acquire fence
514+
static inline void _Py_atomic_fence_acquire(void);
515+
513516
// Release fence
514517
static inline void _Py_atomic_fence_release(void);
515518

‎Include/cpython/pyatomic_gcc.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -542,6 +542,10 @@ static inline void
542542
_Py_atomic_fence_seq_cst(void)
543543
{ __atomic_thread_fence(__ATOMIC_SEQ_CST); }
544544

545+
static inline void
546+
_Py_atomic_fence_acquire(void)
547+
{ __atomic_thread_fence(__ATOMIC_ACQUIRE); }
548+
545549
static inline void
546550
_Py_atomic_fence_release(void)
547551
{ __atomic_thread_fence(__ATOMIC_RELEASE); }

‎Include/cpython/pyatomic_msc.h

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1066,6 +1066,18 @@ _Py_atomic_fence_seq_cst(void)
10661066
#else
10671067
# error "no implementation of _Py_atomic_fence_seq_cst"
10681068
#endif
1069+
}
1070+
1071+
static inline void
1072+
_Py_atomic_fence_acquire(void)
1073+
{
1074+
#if defined(_M_ARM64)
1075+
__dmb(_ARM64_BARRIER_ISHLD);
1076+
#elif defined(_M_X64) || defined(_M_IX86)
1077+
_ReadBarrier();
1078+
#else
1079+
# error "no implementation of _Py_atomic_fence_acquire"
1080+
#endif
10691081
}
10701082

10711083
static inline void

‎Include/cpython/pyatomic_std.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -961,6 +961,13 @@ _Py_atomic_fence_seq_cst(void)
961961
atomic_thread_fence(memory_order_seq_cst);
962962
}
963963

964+
static inline void
965+
_Py_atomic_fence_acquire(void)
966+
{
967+
_Py_USING_STD;
968+
atomic_thread_fence(memory_order_acquire);
969+
}
970+
964971
static inline void
965972
_Py_atomic_fence_release(void)
966973
{

‎Include/internal/pycore_lock.h

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -228,12 +228,12 @@ PyAPI_FUNC(void) _PySeqLock_AbandonWrite(_PySeqLock *seqlock);
228228
PyAPI_FUNC(uint32_t) _PySeqLock_BeginRead(_PySeqLock *seqlock);
229229

230230
// End the read operation and confirm that the sequence number has not changed.
231-
// Returns 1 if the read was successful or 0 if the read should be re-tried.
232-
PyAPI_FUNC(uint32_t) _PySeqLock_EndRead(_PySeqLock *seqlock, uint32_t previous);
231+
// Returns 1 if the read was successful or 0 if the read should be retried.
232+
PyAPI_FUNC(int) _PySeqLock_EndRead(_PySeqLock *seqlock, uint32_t previous);
233233

234234
// Check if the lock was held during a fork and clear the lock. Returns 1
235-
// if the lock was held and any associated datat should be cleared.
236-
PyAPI_FUNC(uint32_t) _PySeqLock_AfterFork(_PySeqLock *seqlock);
235+
// if the lock was held and any associated data should be cleared.
236+
PyAPI_FUNC(int) _PySeqLock_AfterFork(_PySeqLock *seqlock);
237237

238238
#ifdef __cplusplus
239239
}

‎Misc/NEWS.d/next/Core and Builtins/2024-07-04-23-38-30.gh-issue-121368.m3EF9E.rst

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
TODO

‎Modules/_testcapi/pyatomic.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ test_atomic_fences(PyObject *self, PyObject *obj) {
125125
// Just make sure that the fences compile. We are not
126126
// testing any synchronizing ordering.
127127
_Py_atomic_fence_seq_cst();
128+
_Py_atomic_fence_acquire();
128129
_Py_atomic_fence_release();
129130
Py_RETURN_NONE;
130131
}

‎Objects/typeobject.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5387,7 +5387,7 @@ _PyType_LookupRef(PyTypeObject *type, PyObject *name)
53875387
#ifdef Py_GIL_DISABLED
53885388
// synchronize-with other writing threads by doing an acquire load on the sequence
53895389
while (1) {
5390-
int sequence = _PySeqLock_BeginRead(&entry->sequence);
5390+
uint32_t sequence = _PySeqLock_BeginRead(&entry->sequence);
53915391
uint32_t entry_version = _Py_atomic_load_uint32_relaxed(&entry->version);
53925392
uint32_t type_version = _Py_atomic_load_uint32_acquire(&type->tp_version_tag);
53935393
if (entry_version == type_version &&

‎Python/lock.c

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -547,28 +547,31 @@ uint32_t _PySeqLock_BeginRead(_PySeqLock *seqlock)
547547
return sequence;
548548
}
549549

550-
uint32_t _PySeqLock_EndRead(_PySeqLock *seqlock, uint32_t previous)
550+
int _PySeqLock_EndRead(_PySeqLock *seqlock, uint32_t previous)
551551
{
552-
// Synchronize again and validate that the entry hasn't been updated
553-
// while we were readying the values.
554-
if (_Py_atomic_load_uint32_acquire(&seqlock->sequence) == previous) {
552+
// gh-121368: We need an explicit acquire fence here to ensure that
553+
// this load of the sequence number is not reordered before any loads
554+
// withing the read lock.
< 102AC div aria-hidden="true" style="left:-2px" class="position-absolute top-0 d-flex user-select-none DiffLineTableCellParts-module__in-progress-comment-indicator--hx3m3">
555+
_Py_atomic_fence_acquire();
556+
557+
if (_Py_atomic_load_uint32_relaxed(&seqlock->sequence) == previous) {
555558
return 1;
556-
}
559+
}
557560

558-
_Py_yield();
559-
return 0;
561+
_Py_yield();
562+
return 0;
560563
}
561564

562-
uint32_t _PySeqLock_AfterFork(_PySeqLock *seqlock)
565+
int _PySeqLock_AfterFork(_PySeqLock *seqlock)
563566
{
564567
// Synchronize again and validate that the entry hasn't been updated
565568
// while we were readying the values.
566-
if (SEQLOCK_IS_UPDATING(seqlock->sequence)) {
569+
if (SEQLOCK_IS_UPDATING(seqlock->sequence)) {
567570
seqlock->sequence = 0;
568571
return 1;
569-
}
572+
}
570573

571-
return 0;
574+
return 0;
572575
}
573576

574577
#undef PyMutex_Lock

0 commit comments

Comments
 (0)
0