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

Add minimal_memory_lock package #1

Merged
merged 5 commits into from
May 1, 2022
Merged

Conversation

carlossvg
Copy link
Contributor

No description provided.

@carlossvg carlossvg force-pushed the add-minimal-memory-lock branch from 9a2775c to 26e92b2 Compare April 26, 2022 16:17
@carlossvg
Copy link
Contributor Author

FYI @shuhaowu @razr @christophebedard @LanderU @JanStaschulat I created a first PR with the memory lock example. Could you take a look and provide some feedback? Thanks.

@shuhaowu
Copy link
Member

Just as a note, I have my memory locking example in my demo app: https://github.com/shuhaowu/rt-demo/blob/b33a8be/libs/rt/src/app.cc#L15-L16

Copy link
Member

@shuhaowu shuhaowu left a comment

Choose a reason for hiding this comment

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

Looks mostly good, but two main comment:

  • Probably should use RUSAGE_THREAD instead of RUSAGE_SELF to avoid the need for the --sleep.
  • I would consider just calling mlockall all the time and not be able to turn it off. It seems to me that any RT thread should always mlockall and configure malloc. Having an option to turn it off may make it confusing for people who want to implement this in their program, as they won't know when and why they should turn off configure_memory_behavior (which is never?). This also removes the need to memset during memory reservation.

README.md Show resolved Hide resolved
minimal_memory_lock/README.md Outdated Show resolved Hide resolved
minimal_memory_lock/minimal_memory_lock.cpp Outdated Show resolved Hide resolved
minimal_memory_lock/minimal_memory_lock.cpp Outdated Show resolved Hide resolved

print_process_memory("Process memory before spin: ");
print_page_faults("Total page faults before spin");
get_new_page_faults(); // init page fault count
Copy link
Member

Choose a reason for hiding this comment

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

Why is this function called and the return value not consumed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just to initialize the static members from the functions. Since this is not obvious I will change this. I can create a MemoryPageFaultCount class with reset and get functions to make this cleaner.

Copy link
Member

Choose a reason for hiding this comment

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

Minor nitpick: Is it needed to store the pagefault as a state? I just store the page fault count in local variable (A) at one point in the code, do some stuff, and then get the counts again in another local variable (B). I then subtract B - A. Like https://github.com/shuhaowu/rt-demo/blob/7116d52/docs/prefault-experiments/prefault-test.cc#L80

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm checking the page faults inside the node callback so I need to save them either as part of the Node class or somewhere else. I started using the same approach as the rt wiki, so this is why I was using storing them inside the function itself.

Copy link
Member

Choose a reason for hiding this comment

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

Ah maybe what you have is fine. Maybe add a comment to call out the fact that you're initialization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will create a class so it's easier to understand.

minimal_memory_lock/minimal_memory_lock.cpp Show resolved Hide resolved
minimal_memory_lock/minimal_memory_lock.cpp Outdated Show resolved Hide resolved
@shuhaowu
Copy link
Member

Sorry this PR review dragged out to 30 comments, didn't intend it to be like that 😅 ..

That said, I want to pull this comment out of that thread to keep things a bit more clear (if that's possible):

This is how it was implemented at the begging. Then, I started debugging I wanted to compare with and without memory lock. In fact, I think is very helpful for the users to compare both cases.

I see the following options:

  1. Remove all the options, keep it minimal
  2. Keep all the options. Add comments explaining the memory has to be locked for real-time behavior.
  3. Create a separate executable without locking the memory

In this other comment, you mentioned that if RT is needed for the DDS thread, then you need a sleep (which is kind of a bug from RMW..). There are some commonalities here:

I think we should figure out who the target audience for this code is. Right now it feels like code that proves that memory locking is working (almost like unit-tests). Maybe, we should target RT app developers instead who are probably more interested in a "sample app framework" for ROS? Something with all the scheduling and memory setup. We can still put some logging about page faults, as that could be the "sample application code", if that makes sense. This way you don't need any of the options. We can quickly document all the things in the README. I also have a blogpost coming that explains all these options in hopefully a week.

The "sample app approach" is also what my repo is trying to do for non-ROS apps.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
minimal_memory_lock/README.md Outdated Show resolved Hide resolved
ros2_realtime_examples/package.xml Show resolved Hide resolved
@carlossvg carlossvg force-pushed the add-minimal-memory-lock branch from f9fe960 to 0073bc3 Compare April 29, 2022 11:57
@carlossvg
Copy link
Contributor Author

@shuhaowu @LanderU I updated the example. These are the main changes:

  • Moved locking and rusage functions to header files
  • I extended the README with some instructions (I'm thinking about using asciinema)
  • I added one additional option to simulate allocations on the heap during runtime. This helps to explain why using a preallocated memory pool could be useful.

Signed-off-by: Carlos San Vicente <[email protected]>
@carlossvg carlossvg force-pushed the add-minimal-memory-lock branch from 0073bc3 to d2e080f Compare April 29, 2022 12:03
@carlossvg carlossvg force-pushed the add-minimal-memory-lock branch from d2e080f to d72751f Compare April 29, 2022 12:07
Copy link
Member

@shuhaowu shuhaowu left a comment

Choose a reason for hiding this comment

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

Thanks for working this through!

@github-pages github-pages bot temporarily deployed to github-pages April 30, 2022 08:22 Inactive
@github-pages github-pages bot temporarily deployed to github-pages April 30, 2022 08:30 Inactive
@github-pages github-pages bot temporarily deployed to github-pages April 30, 2022 08:32 Inactive
@carlossvg
Copy link
Contributor Author

@shuhaowu Thanks a lot for the review. I added some asciicenema recordings to show what the output looks like. It provides a nice visualization in github pages. See the result here: https://ros-realtime.github.io/ros2-realtime-examples/minimal_memory_lock/

@carlossvg carlossvg force-pushed the add-minimal-memory-lock branch from f157096 to 3c1851e Compare April 30, 2022 08:36
@github-pages github-pages bot temporarily deployed to github-pages April 30, 2022 08:36 Inactive
@github-pages github-pages bot temporarily deployed to github-pages April 30, 2022 08:37 Inactive
@LanderU LanderU merged commit c4d5190 into rolling May 1, 2022
@carlossvg carlossvg deleted the add-minimal-memory-lock branch May 3, 2022 15:22
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.

3 participants