Hi @picnixz,

Sorry to bother you.
Could you help take a review on this PR?

Wish you a good day!

I think this fix can be correct but this doesn't really fix the underlying problem: we are reading unsynced values so this change doesn't give us any guarantees that the values are consistent with a concurrent call to set them. Indeed we could read half of them from one write and another half from another.

I think this is fine but perhaps we need exclusive access.

@nascheme what do you think?

I think so as well. It will only mitigate some cases. AFAIU, the problem is that we have 3 values to read or write but these values can be changed by another thread at any moment right? the correct fix should be to lock the entire GC state when writing or reading.

Note that get_gc_state() already requires the caller to hold the GIL, so we could just hold it for the entirety of each function?

The approach of "sprinkling in" relaxed atomics to silence TSAN warnings is not correct, IMO. They don't ensure ordering. I'd suggest we keep using relaxed atomics for enabled. gc_should_collect() should acquire a mutex, snapshot the fields to locals, release, then evaluate. The mutex is only needed on the slow path of record_allocation. Writers take the mutex: gc.set_threshold() in Modules/gcmodule.c, interp->gc.long_lived_total = state->long_lived_total. The mutex is only held across the snapshot/write, avoiding deadlocks.

The approach of "sprinkling in" relaxed atomics to silence TSAN warnings is not correct, IMO. They don't ensure ordering. I'd suggest we keep using relaxed atomics for enabled. gc_should_collect() should acquire a mutex, snapshot the fields to locals, release, then evaluate. The mutex is only needed on the slow path of record_allocation. Writers take the mutex: gc.set_threshold() in Modules/gcmodule.c, interp->gc.long_lived_total = state->long_lived_total. The mutex is only held across the snapshot/write, avoiding deadlocks.

Hi @nascheme , @picnixz and @pablogsal ,

Thanks very much for your review and suggestions!
I will follow the mutex way to ensure the threshold's consistency.

Hi @nascheme , @picnixz , @pablogsal ,

Thanks very much for your review suggestions!

I have updated with an exclusive mutex and take a snapshot for the threshold reading.

I'm not sure the gc_generation.count especially young.count should be inside the mutex.
It should be a frequently visited counter and other places use atomic load and increment.

So I only update threshold's case here and plan to update the count related issue in another pr.

Please correct me if there is any problem and let me know if we should handle count along with threshold in this patch.

Wish you a good day!

The thresholds are rarely ever changed so I don't think it needs its own lock, how about doing a stop-the-world pause when changing the threshold instead? If we do that then the code which reads the threshold doesn't need to be synchronized explicitly and probably be a simpler change.

The thresholds are rarely ever changed so I don't think it needs its own lock, how about doing a stop-the-world pause when changing the threshold instead? If we do that then the code which reads the threshold doesn't need to be synchronized explicitly and probably be a simpler change.

Hi @kumaraditya303 ,

Thanks very much for your time and review advice! ❤

Stop-the-world will simplify our code and the threshold should be a rarely updated one.
I will update the patch with the STW path.

Hi @kumaraditya303 ,

Thanks very much for your review and help! ❤️
Sorry to bother you again.

The CI tests have been passed.
Please help take a look if it is ready to be merged.

Wish you a good day!

Thanks @LindaSummer for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14, 3.15.
🐍🍒⛏🤖

GH-150841 is a backport of this pull request to the 3.15 branch.

GH-150842 is a backport of this pull request to the 3.14 branch.

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Debian root 3.15 (tier-1) has failed when building commit d750529.

What do you need to do:

1. Don't panic.
2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/2019/builds/136) and take a look at the build logs.
4. Check if the failure is related to this commit (d750529) or if it is a false positive.
5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/2019/builds/136

Failed tests:

• test_embed

Failed subtests:

• test_thread_state_ensure - test.test_embed.AuditingTests.test_thread_state_ensure

Summary of the results of the build (if available):

==

Traceback (most recent call last):
  File "/root/buildarea/3.15.angelico-debian-amd64/build/Lib/test/test_embed.py", line 2000, in test_thread_state_ensure
    self.run_embedded_interpreter("test_thread_state_ensure")
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/root/buildarea/3.15.angelico-debian-amd64/build/Lib/test/test_embed.py", line 138, in run_embedded_interpreter
    self.assertEqual(p.returncode, returncode,
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
                     "bad returncode %d, stderr is %r" %
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                     (p.returncode, err))
                     ^^^^^^^^^^^^^^^^^^^^
AssertionError: -6 != 0 : bad returncode -6, stderr is "_testembed: ./Programs/_testembed.c:2736: test_thread_state_ensure: Assertion `_Py_atomic_load_int(&data.done) == 1' failed.\n"


Traceback (most recent call last):
  File "/root/buildarea/3.15.angelico-debian-amd64/build/Lib/test/support/__init__.py", line 948, in gc_collect
    gc.collect()
    ~~~~~~~~~~^^
ResourceWarning: unclosed file <_io.FileIO name=11 mode='wb' closefd=True>