Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

gh-112535: Update _Py_ThreadId() to support PowerPC #112624

Merged
merged 4 commits into from
Dec 5, 2023
Merged

gh-112535: Update _Py_ThreadId() to support PowerPC #112624

merged 4 commits into from
Dec 5, 2023

Conversation

corona10
Copy link
Member

@corona10 corona10 commented Dec 2, 2023 •
edited
Loading

@bedevere-app bedevere-app bot mentioned this pull request Dec 2, 2023
Closed
@corona10
Copy link
Member Author

corona10 commented Dec 2, 2023

@colesbury cc @vstinner

Not sure that this implementation was what @colesbury intended, but let's take a look.

vstinner
vstinner reviewed Dec 2, 2023
View reviewed changes
Include/object.h Outdated Show resolved Hide resolved
@vstinner
Copy link
Member

vstinner commented Dec 3, 2023

!buildbot s390x

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @vstinner for commit fd4c844 🤖

The command will test the builders whose names match following regular expression: s390x

The builders matched are:

  • s390x SLES PR
  • s390x RHEL7 PR
  • s390x Fedora Clang PR
  • s390x RHEL7 LTO + PGO PR
  • s390x Fedora Rawhide Clang Installed PR
  • s390x RHEL8 Refleaks PR
  • s390x Fedora Refleaks PR
  • s390x Fedora Rawhide LTO + PGO PR
  • s390x Fedora PR
  • s390x RHEL7 Refleaks PR
  • s390x RHEL7 LTO PR
  • s390x Fedora Rawhide PR
  • s390x Debian PR
  • s390x Fedora LTO + PGO PR
  • s390x Fedora Rawhide Refleaks PR
  • s390x Fedora Rawhide LTO PR
  • s390x RHEL8 PR
  • s390x RHEL8 LTO + PGO PR
  • s390x Fedora Clang Installed PR
  • s390x Fedora Rawhide Clang PR
  • s390x Fedora LTO PR
  • s390x RHEL8 LTO PR

@vstinner
Copy link
Member

vstinner commented Dec 3, 2023

Does someone have 32-bit PowerPC to test the change?

I scheduled buildbot jobs on s390x to test the 64-bit code path. But oh wait, this code path requires to build Python without GIL? I can try to test the change manually on s390x. Which kind of test should I run? The Python test suite?

@corona10
Copy link
Member Author

corona10 commented Dec 3, 2023

I scheduled buildbot jobs on s390x to test the 64-bit code path. But oh wait, this code path requires to build Python without GIL? I can try to test the change manually on s390x. Which kind of test should I run? The Python test suite?

Hmm, this PR is not about s390x. Is it meaningful?

Which kind of test should I run? The Python test suite?

Currently, yes, or you can debug the ob_tid from the gdb.

@corona10
Copy link
Member Author

corona10 commented Dec 3, 2023

At the build bot, we have ppc64 but no ppc32

@corona10
Copy link
Member Author

corona10 commented Dec 3, 2023

!buildbot ppc

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @corona10 for commit fd4c844 🤖

The command will test the builders whose names match following regular expression: ppc

The builders matched are:

  • PPC64LE Fedora Rawhide LTO + PGO PR
  • PPC64LE Fedora Stable LTO + PGO PR
  • PPC64LE Fedora Rawhide Refleaks PR
  • PPC64LE RHEL7 LTO PR
  • PPC64LE CentOS9 LTO PR
  • PPC64LE Fedora Stable LTO PR
  • PPC64 Fedora PR
  • PPC64LE Fedora Stable Clang PR
  • PPC64 AIX XLC PR
  • PPC64LE Fedora Rawhide Clang PR
  • PPC64 AIX PR
  • PPC64LE CentOS9 Refleaks PR
  • PPC64LE Fedora Stable Clang Installed PR
  • PPC64LE CentOS9 LTO + PGO PR
  • PPC64LE Fedora Rawhide Clang Installed PR
  • PPC64LE Fedora Rawhide LTO PR
  • PPC64LE RHEL8 Refleaks PR
  • PPC64LE Fedora Rawhide PR
  • PPC64LE RHEL7 LTO + PGO PR
  • PPC64LE Fedora Stable Refleaks PR
  • PPC64LE RHEL8 LTO PR
  • PPC64LE Fedora Stable PR
  • PPC64LE RHEL8 PR
  • PPC64LE RHEL8 LTO + PGO PR
  • PPC64LE RHEL7 PR
  • PPC64LE RHEL7 Refleaks PR
  • PPC64LE CentOS9 PR

@corona10
Copy link
Member Author

corona10 commented Dec 3, 2023

For IBM Z(s390x), I will create a PR separately after verifying the implementation strategy with @colesbury and @vstinner :)

@corona10
Copy link
Member Author

corona10 commented Dec 3, 2023

For ppc, we need to use qemu.

@vstinner
Copy link
Member

vstinner commented Dec 3, 2023

Hmm, this PR is not about s390x. Is it meaningful?

Oops. I confused ppc64 and s390x. The issue says "PowerPC and IBM Z". But ok, this PR is about PowerPC CPU, 32-bit and 64-bit.

About buildbots, it seems like PPC has no NoGIL builder. So it should be tested manually. I have access to ppc64 machines, I can test manually.

you can debug the ob_tid from the gdb.

What do you mean? Python test suite doesn't check it? Maybe we should add dedicated tests in the C API tests?

@corona10
Copy link
Member Author

corona10 commented Dec 3, 2023 •
edited
Loading

What do you mean? Python test suite doesn't check it? Maybe we should add dedicated tests in the C API tests?

IMO, it is an implementation detail about a thread that has ownership with a specific Python object. I am not sure that we need to have a specific test for this API. IMO, if it is not properly implemented, the refcount will not be processed properly so current tests are enough to check it.

FYI, PEP 703 explains why tid should be recorded to each of Python Object and this API is only used for recording tid.

colesbury
colesbury approved these changes Dec 4, 2023
View reviewed changes
Include/object.h Outdated Show resolved Hide resolved
Include/object.h Outdated Show resolved Hide resolved
@vstinner
Copy link
Member

vstinner commented Dec 4, 2023

I confirm that the ppc64 code path works with GCC 8.5 on RHEL 8.9 ppc64le.

I tested manually my PR #112709 with this change on Linux x86-64 on Python built without GIL (./configure --disable-gil CFLAGS="-O0"): my test pass successfully :-) I also ran the Python test suite: 430 tests OK.

@vstinner
Copy link
Member

vstinner commented Dec 5, 2023 •
edited
Loading

I tested the updated PR (now with _Py_ThreadId() unit test) on Linux (RHEL) ppc64le with GCC: I confirm that the test suite pass.

Commands:

git clone https://github.com/python/cpython --depth=1
cd cpython
wget 'https://github.com/python/cpython/pull/112624.patch'
git apply 112624.patch
./configure --disable-gil
make -j3
./python -m test -v test_capi.test_misc -m test_py_thread_id
./python -m test -j3

Tests output:

$ ./python -m test -j3
== CPython 3.13.0a2+ (heads/main-dirty:81ee026091, Dec 5 2023, 03:43:48) [GCC 8.5.0 20210514 (Red Hat 8.5.0-20)]
== Linux-4.18.0-513.9.1.el8_9.ppc64le-ppc64le-with-glibc2.28 little-endian
== Python build: nogil release
== cwd: /root/victor/cpython/build/test_python_worker_3022386æ
== CPU count: 2
== encodings: locale=UTF-8 FS=utf-8
== resources: all test resources are disabled, use -u option to unskip tests
(...)
431 tests OK.

The == Python build: nogil release line shows that Python was built with nogil.

@corona10 corona10 merged commit d824512 into python:main Dec 5, 2023
30 checks passed
@corona10 corona10 deleted the gh-112535-powerpc branch December 5, 2023 09:03
@vstinner
Copy link
Member

vstinner commented Dec 5, 2023 •
edited
Loading

I also tested this change on Fedora 38 ppc64le with clang 16.0.6: tests pass as well.

== CPython 3.13.0a2+ (heads/main-dirty:81ee026, Dec 5 2023, 04:14:22) [Clang 16.0.6 (Fedora 16.0.6-3.fc38)]
== Linux-5.8.17-200.fc32.ppc64le-ppc64le-with-glibc2.37 little-endian
== Python build: nogil release
(...)
447 tests OK.

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@corona10 @aisk
pythongh-112535: Update _Py_ThreadId() to support PowerPC (pythongh-1…
440cad7 
…12624)
U2FsdGVkX1 pushed a commit to fedora-riscv/python3.13 that referenced this pull request Jun 22, 2024
@hroncok
Don't build the freethreding packages on ppc64le/s390x for now
2da1bb9 
See python/cpython#112535
And python/cpython#112624
And python/cpython#112751

Rather than backporting a handful of small commits, let's wait for the next release.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@corona10 @Glyphack
pythongh-112535: Update _Py_ThreadId() to support PowerPC (pythongh-1…
cd20e00 
…12624)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colesbury colesbury colesbury approved these changes

@vstinner vstinner Awaiting requested review from vstinner

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.
4 participants
@corona10 @vstinner @bedevere-bot @colesbury