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.

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

Open
wants to merge 17 commits into
base: rolling
Choose a base branch
from

Conversation

WideAwakeTN
Copy link

@WideAwakeTN WideAwakeTN commented Jan 6, 2023

The changes in this pull request add two new mutex classes to rclcpp:

  • PIMutex
  • RecursivePIMutex

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

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]>
@ros-discourse
Copy link

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

@WideAwakeTN
Copy link
Author

A ROS Discourse article has been created for this pull request:
https://discourse.ros.org/t/avoiding-priority-inversions-includes-draft-pull-requests/29219

@DasRoteSkelett
Copy link

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).

@WideAwakeTN
Copy link
Author

@DasRoteSkelett

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.

@ros-discourse
Copy link

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

@ros-discourse
Copy link

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

@WideAwakeTN
Copy link
Author

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)
Copy link

@carlossvg carlossvg Feb 17, 2023

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.

Copy link
Author

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.

Choose a reason for hiding this comment

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

@razr FYI

WideAwakeTN and others added 2 commits February 18, 2023 11:02
Signed-off-by: Martin Mayer <[email protected]>
@WideAwakeTN WideAwakeTN marked this pull request as ready for review February 23, 2023 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants