-
Notifications
You must be signed in to change notification settings - Fork 11
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
Current librtpi API breaks down for process shared condvars #22
Comments
gratian
added a commit
to gratian/librtpi
that referenced
this issue
Jan 22, 2020
The current librtpi conditional variable API has the user pass a pointer to the associated mutex at pi_cond_init() time and it is stored in the condvar private data for later use when waiting or signaling. As described in issue dvhart#22[1] this breaks down for process shared use cases. In those cases the associated mutex is mapped at different addresses in different processes so the pointer passed by one process to pi_cond_init() will not be valid in the context of another process doing operations on the shared condvar. Change the API to pass in the associated mutex where it is needed instead of at init time: * pi_cond_wait(), pi_cond_timedwait() - mutex used in the FUTEX_WAIT_REQUEUE_PI futex call. Passing in a pointer to the associated mutex as a parameter matches the equivalent libpthread/POSIX APIs. * pi_cond_signal(), pi_cond_broadcast() - mutex used in the FUTEX_CMP_REQUEUE_PI futex call as the requeue target in order to avoid "thundering herd" wake-ups. [1] dvhart#22 Signed-off-by: Gratian Crisan <gratian.crisan@ni.com>
gratian
added a commit
to gratian/librtpi
that referenced
this issue
Jan 22, 2020
Commit 6814992 ("pi_cond: Update API to support process shared condvar use cases") introduced an API change in order to address issue "dvhart#22 Current librtpi API breaks down for process shared condvars"[1]. Make the necessary changes to port all tests to the new API: - remove the mutex parameter when initializing the condvar: DEFINE_PI_COND(), pi_cond_init(). - pass a pointer to the associated mutex for pi_cond_wait(), pi_cond_timedwait(), pi_cond_signal(), and pi_cond_broadcast() calls. [1] dvhart#22 Signed-off-by: Gratian Crisan <gratian.crisan@ni.com>
dvhart
pushed a commit
that referenced
this issue
Jan 30, 2020
The current librtpi conditional variable API has the user pass a pointer to the associated mutex at pi_cond_init() time and it is stored in the condvar private data for later use when waiting or signaling. As described in issue #22[1] this breaks down for process shared use cases. In those cases the associated mutex is mapped at different addresses in different processes so the pointer passed by one process to pi_cond_init() will not be valid in the context of another process doing operations on the shared condvar. Change the API to pass in the associated mutex where it is needed instead of at init time: * pi_cond_wait(), pi_cond_timedwait() - mutex used in the FUTEX_WAIT_REQUEUE_PI futex call. Passing in a pointer to the associated mutex as a parameter matches the equivalent libpthread/POSIX APIs. * pi_cond_signal(), pi_cond_broadcast() - mutex used in the FUTEX_CMP_REQUEUE_PI futex call as the requeue target in order to avoid "thundering herd" wake-ups. [1] #22 Signed-off-by: Gratian Crisan <gratian.crisan@ni.com>
dvhart
pushed a commit
that referenced
this issue
Jan 30, 2020
Commit 6814992 ("pi_cond: Update API to support process shared condvar use cases") introduced an API change in order to address issue "#22 Current librtpi API breaks down for process shared condvars"[1]. Make the necessary changes to port all tests to the new API: - remove the mutex parameter when initializing the condvar: DEFINE_PI_COND(), pi_cond_init(). - pass a pointer to the associated mutex for pi_cond_wait(), pi_cond_timedwait(), pi_cond_signal(), and pi_cond_broadcast() calls. [1] #22 Signed-off-by: Gratian Crisan <gratian.crisan@ni.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The current librtpi API creates a strong association between a user mutex and a conditional variable by passing a pointer to the mutex to pi_cond_init() along with the pointer to the conditional variable to be initialized. The pointer to the associated mutex is stored in the private data for the conditional variable.
This is done because in order to guarantee proper RT behaviour we have to use the FUTEX_WAIT_REQUEUE_PI and FUTEX_CMP_REQUEUE_PI futex ops for waiting and signalling respectively and the user mutex is a parameter for both calls. Storing a pointer to the user mutex at init time prevents the practice of signalling the condition prior to the association of the mutex.
The problem is the current librtpi API breaks down for process shared conditional variables as illustrated by glibc tests: tst-cond4.c, tst-cond6.c, tst-cond12.c, and tst-cond13.c.
For process-shared use cases the user mutex and the condvar are allocated in shared memory. This shared memory is (usually) mapped at different addresses in the different processes that interact with the condvar. As a result the pointer to the mutex stored by the pi_cond_init() call points to a valid address only for the process that calls init. All other processes that interact with the process-shared condvar will cause an invalid memory access when accessing the stored mutex pointer.
A good example of this problem is glibc test tst-cond12.c. In this test a parent process creates a memory mapping for a process shared conditional variable and mutex that is later re-mapped by the child process at a different address and the old mapping is unmapped via munmap(). Because the conditional variable was initialized before the fork() the stored mutex pointer now points to an unmapped address in the child process and it results in a SIGSEGV crash when the mutex is
accessed inside the pi_cond.c code.
A proposed solution would be to change the librtpi API to pass the user mutex for the condvar operations that need it and do away with storing a pointer to the user mutex inside the condvar at init.
The new API would look like this:
int pi_cond_init(pi_cond_t *cond, uint32_t flags)
int pi_cond_destroy(pi_cond_t *cond)
int pi_cond_wait(pi_cond_t *cond, pi_mutex_t *mutex)
int pi_cond_timedwait(pi_cond_t *cond, pi_mutex_t *mutex, const struct timespec *restrict abstime)
int pi_cond_signal(pi_cond_t *cond, pi_mutex_t *mutex)
int pi_cond_broadcast(pi_cond_t *cond, pi_mutex_t *mutex)
I can follow up with patches if we're in agreement this is the correct path forward.
The text was updated successfully, but these errors were encountered: