GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-117657: Use an atomic store to set type flags. by nascheme · Pull Request #127588 · python/cpython

Conversation

@nascheme

@nascheme nascheme commented Dec 4, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

The PyType_HasFeature() function reads the flags with a relaxed atomic load and without holding the type lock. To avoid data races, use atomic stores to update the type flags.

This change eliminates some warnings from TSAN. For non-free-threaded builds, there should be no performance impact.

  • Issue: Make TSAN tests pass with the GIL disabled in free-threaded builds #117657

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nascheme
Use an atomic store to set type flags.
bc26f92 
The `PyType_HasFeature()` function reads the flags with a relaxed atomic
load and without holding the type lock.  To avoid data races, use atomic
stores if `PyType_Ready()` has already been called.
@nascheme nascheme added skip news topic-free-threading labels Dec 4, 2024
@nascheme
nascheme requested a review from colesbury December 4, 2024 01:59
@nascheme
nascheme requested a review from markshannon as a code owner December 4, 2024 01:59
@bedevere-app bedevere-app Bot added the awaiting core review label Dec 4, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Dec 4, 2024
Make TSAN tests pass with the GIL disabled in free-threaded builds #117657
Closed
ZeroIntensity
ZeroIntensity reviewed Dec 4, 2024
View reviewed changes
Comment thread Objects/typeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
colesbury
colesbury reviewed Dec 4, 2024
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, looks good to me. I think type_set_flags can be simplified and I have a question below about locking.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/typeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/typeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
nascheme added 2 commits December 4, 2024 11:23
@nascheme
Simplify type_set_flags() function.
3c67c79 
Just use FT_ATOMIC_STORE_ULONG_RELAXED() always, not worth the extra
code complexity.
@nascheme
Add type lock held assert to type_set_flags().
0fe7d78 
This helps ensure that type flag setting is done in a free-threading
safe way. Fix a few cases were flags were set without holding the type
lock. Use `type_modified_unlocked()` in a couple places where the lock
is already head.
@nascheme

nascheme commented Dec 4, 2024

Copy link
Copy Markdown
Member Author

If I add the assert, it fails like this:

_PyCriticalSection_AssertHeld (mutex=<optimized out>) at ./Include/internal/pycore_critical_section.h:222
type_set_flags (tp=tp@entry=0x200006adc10, flags=22016) at Objects/typeobject.c:355
type_clear_flags (tp=tp@entry=0x200006adc10, flag=flag@entry=1048576) at Objects/typeobject.c:379
type_set_abstractmethods (type=0x200006adc10, value=0x20000342010, context=<optimized out>) at Objects/typeobject.c:1570
getset_set (self=0x20000010790, obj=0x200006adc10, value=0x20000342010) at Objects/descrobject.c:249
type_setattro (self=0x200006adc10, name=<optimized out>, value=0x20000342010) at Objects/typeobject.c:5967
PyObject_SetAttr (v=v@entry=0x200006adc10, name=<optimized out>, value=value@entry=0x20000342010) at Objects/object.c:1409

The code in type_set_abstractmethods is:

    PyType_Modified(type);
    if (abstract)
        type_add_flags(type, Py_TPFLAGS_IS_ABSTRACT);
    else
        type_clear_flags(type, Py_TPFLAGS_IS_ABSTRACT);

When this runs, the type is already marked ready. The type lock is held inside PyType_Modified(). To me this looks like a potential data race. Another thread could read the flags after PyType_Modified releases the lock and before the next lines of code are executed.

Looking at PyType_Modified(), I wonder if there is another data race there. The code is:

    // Quick check without the lock held
    if (type->tp_version_tag == 0) {
        return;
    }

    BEGIN_TYPE_LOCK();
    type_modified_unlocked(type);
    END_TYPE_LOCK();

The tp_version_tag is set with an atomic store (by set_version_unlocked()). I would think when checking if it's 0, we need an atomic load for it.

A possible solution could be to change the type_set_abstractmethods code to be:

    BEGIN_TYPE_LOCK();
    type_modified_unlocked(type);
    if (abstract)
        type_add_flags(type, Py_TPFLAGS_IS_ABSTRACT);
    else
        type_clear_flags(type, Py_TPFLAGS_IS_ABSTRACT);
    END_TYPE_LOCK();
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nascheme

nascheme commented Dec 4, 2024

Copy link
Copy Markdown
Member Author

Using type_modified_unlocked() in couple of places and putting the flag updates inside the critical section seems to have worked well. I only had to modify a couple places and the unit tests now all pass without that lock held assertion failing.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nascheme
Merge 'origin/main' into pythongh-117657-tp-flags-set-atomic
4984eb7
@nascheme
nascheme merged commit baae9cb into python:main Feb 26, 2025
@bedevere-app bedevere-app Bot removed the awaiting core review label Feb 26, 2025
@nascheme
nascheme deleted the gh-117657-tp-flags-set-atomic branch February 28, 2025 04:36
@nascheme nascheme mentioned this pull request Mar 5, 2025
gh-129817: Use _PyType_HasFeature() to check tp_flags. #130892
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesbury colesbury colesbury left review comments
@ZeroIntensity ZeroIntensity ZeroIntensity left review comments
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@nascheme @colesbury @ZeroIntensity
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.