-
Notifications
You must be signed in to change notification settings - Fork 55
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
Avoid priority inversions when using FIFO scheduling #174
base: rolling
Are you sure you want to change the base?
Conversation
Signed-off-by: Martin Mayer <[email protected]>
Signed-off-by: Martin Mayer <[email protected]>
Signed-off-by: Martin Mayer <[email protected]>
Signed-off-by: Martin Mayer <[email protected]>
Signed-off-by: Martin Mayer <[email protected]>
Signed-off-by: Martin Mayer <[email protected]>
Signed-off-by: Martin Mayer <[email protected]>
Signed-off-by: Martin Mayer <[email protected]>
Signed-off-by: Martin Mayer <[email protected]>
Signed-off-by: Martin Mayer <[email protected]>
Signed-off-by: Martin Mayer <[email protected]>
Signed-off-by: Martin Mayer <[email protected]>
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/avoiding-priority-inversions-includes-draft-pull-requests/29219/1 |
A ROS Discourse article has been created for this pull request: |
Looks good to me, but I am not a reviewer. I would really like to have the commits squashed / rebased (i.e. stuff like "code style fix" can easily be fixuped). |
That's definitely nicer. |
Signed-off-by: Martin Mayer <[email protected]>
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/avoiding-priority-inversions-includes-draft-pull-requests/29219/7 |
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/avoiding-priority-inversions-includes-draft-pull-requests/29219/8 |
Signed-off-by: Martin Mayer <[email protected]>
I added a CMake option in the CMakeLists.txt which allows to turn the priority inheritance feature on or off without changing a single line of code. The changes also improved readability. |
CMakeLists.txt
Outdated
|
||
option(USE_PI_MUTEX "Enables priority inheritance mutexes." ON) | ||
if(USE_PI_MUTEX) | ||
if(WIN32 OR APPLE) |
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 would do it the other way around. I would list systems with PI_MUTEX support and fail if the system is not in the list. With this approach you will get an error for unknown systems and for new systems you explicitly add them when you know it is supported.
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.
Makes sense. Better now?
Unfortunately I could not find much info on the CMAKE_SYSTEM_NAME variable for VxWorks and QNX systems. I hope I got the right strings.
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.
@razr FYI
Signed-off-by: Martin Mayer <[email protected]>
Signed-off-by: Martin Mayer <[email protected]>
Signed-off-by: Martin Mayer <[email protected]>
The changes in this pull request add two new mutex classes to rclcpp:
These mutex implementations are compatible with the std lib ones, since they derive from them, but they additionally possess the priority inheritance trait. Therefore threads which are using these mutexes cannot suffer from priority inversion when using realtime scheduling, i.e. SCHED_FIFO as defined by the POSIX standard. In general, using mutexes with priority inheritance is a necessary step for making ROS2 more suitable for hard realtime scenarios.
The use of these new mutex implementations is demonstrated in the following pull request for rclcpp: ros2/rclcpp#2078
It can be seen that the necessary code changes are minimal. Usually it is enough to replace std::mutex with rclcpp::PIMutex and std::recursive_mutex with rclcpp::RecursivePIMutex. Only in some cases when lock guards are used it's necessary to explicitly state the desired mutex type, i.e. std::mutex or std::recursive_mutex, to avoid compiler errors.
In general it would be best to avoid the use of mutexes by design through lock free data structures and clever use of atomics, if possible. But refactoring large parts of the ROS2 source code is not really a viable option to make ROS2 more realtime friendly in the short term. The approach with the best cost-benefit ratio presumably is to simply replace all mutex classes as described beforehand.
This pull request contains new units tests. One of the new test cases ensures that priority inversion is avoided when using the new mutex classes. To compile this particular testcase the following pull request for rcutils is needed: ros2/rcutils#406