-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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.
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
gh-112606: Use sem_clockwait with monotonic time when supported in parking_lot.c #112733
Conversation
Completes one part of moving `parking_lot.c`'s semaphore waiting to prefer functions that support CLOCK_MONOTONIC. Uses sem_clockwait in the semaphore-based implementation.
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.
Thanks, this looks good to me. @corona10 can you review this please?
Also cc @vstinner - |
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.
I wrote similar code in Python/thread_pthread.h
. Here is my review.
This change only fix one code path. The code path if _Py_USE_SEMAPHORES is not defined still uses the system clock. pthread_condattr_setclock(&ca, CLOCK_MONOTONIC)
can be used for that: see Python/thread_pthread.h
.
Unrelated to this PR, I have concerns about _PySemaphore_PlatformWait()
.
(DWORD) (timeout / 1000000)
can overflow in the Windows code path. Using _PyTime_As100Nanoseconds()
may be a good start.
PEP 475 "Retry system calls failing with EINTR" is not supported: the caller has to handle Py_PARK_INTR. The problem is that it's quite hard to recompute properly a timeout to take in account time elapsed by the call which failed with Py_PARK_INTR. It would be better to retry on EINTR and recompute the timeout. Similar to what Python/thread_pthread.h
does in PyThread_acquire_lock_timed()
. For the sem_clockwait()
code path, it's easy, since we only have to compute an absolute timestamp once.
Misc/NEWS.d/next/Core and Builtins/2023-12-04-23-51-40.gh-issue-112606.pHvfIs.rst
Outdated
Show resolved
Hide resolved
@vstinner Thanks for reviewing, I'm finishing up the remaining |
I will take look this PR today |
@vstinner Please see the other PR, which is still a draft, for discussion about the pthreads code path: #112616. I don't think
"Retry system calls failing with EINTR" - Interrupts are properly retried. It happens at a higher level in the abstraction, see |
… in parking_lot.c (pythongh-112733)
… in parking_lot.c (pythongh-112733)
Completes one part of moving
parking_lot.c
's semaphore waiting to prefer functions that supportCLOCK_MONOTONIC
Usessem_clockwait
in the semaphore-based implementation. Split with previous PR to move support forCLOCK_MONOTONIC
inpthread_condvar_t
implementation to a separate PR.