-
Notifications
You must be signed in to change notification settings - Fork 19
!perf: generalize perf syscalls #415
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
base: master
Are you sure you want to change the base?
Conversation
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.
Hello @adamgreloch, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request. The primary goal of this PR, as indicated by the title and changes, is to generalize the performance monitoring (perf) syscalls within the system. Previously, these functions (perf_start, perf_read, perf_finish) were declared within the sys/threads.h header, implying a thread-specific focus. This change introduces a new header file, sys/perf.h, specifically for performance monitoring functions and modifies the function signatures to include a perf_mode_t mode parameter. This allows the perf syscalls to support different modes of operation beyond just threads, making the performance monitoring framework more flexible. The previous thread-specific behavior is preserved by using the perf_mode_threads mode. This is a breaking change as consumers of the old perf_* functions will need to update their includes and function calls.
Highlights
- Generalization of Perf Syscalls: The performance monitoring syscalls (
perf_start,perf_read,perf_finish) are generalized to support different modes of operation. - New Header File: A dedicated header file,
include/sys/perf.h, is introduced to house the declarations for performance monitoring functions. - Mode Parameter Added: The
perf_start,perf_read, andperf_finishfunctions now accept aperf_mode_t modeparameter to specify the desired monitoring mode. - Removal from sys/threads.h: The old declarations of the perf functions are removed from
include/sys/threads.h. - Breaking Change: This PR is marked as a breaking change, requiring updates for code that previously used the perf functions from
sys/threads.h. - Dependency: This PR requires another PR (phoenix-rtos/phoenix-rtos-kernel/pull/661) to function correctly.
Changelog
- include/sys/perf.h
- Added a new header file for performance monitoring syscall declarations.
- Declared
perf_start,perf_read, andperf_finishfunctions with a newperf_mode_t modeparameter.
- include/sys/threads.h
- Removed the old declarations for
perf_start,perf_read, andperf_finish.
- Removed the old declarations for
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Headers shift and change,
Functions find a new home,
Perf modes now expand.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request successfully generalizes the performance syscall declarations by moving them from include/sys/threads.h to a new dedicated header file include/sys/perf.h and updating their signatures to include a perf_mode_t argument. This is a good step towards better organization and flexibility for performance monitoring features. The removal of the old declarations from sys/threads.h correctly reflects the breaking change mentioned in the description.
It's noted that this PR is dependent on phoenix-rtos/phoenix-rtos-kernel#661 for the corresponding implementation changes. The header changes themselves appear correct and well-structured.
Summary of Findings
- Copyright Year: The copyright year in the new header file
include/sys/perf.his set to 2025. This is likely a typo and should be the current year (2024). This was identified as alowseverity issue and not commented on directly per review settings. - Header Formatting: There are some extra blank lines around function declarations in
include/sys/perf.h. While not functionally incorrect, consistent formatting improves readability. This was identified as alowseverity issue and not commented on directly per review settings.
Merge Readiness
Based on the review of the header file changes presented in this pull request, no critical or high severity issues were found. The changes correctly introduce a new header for performance syscalls and remove the old declarations, aligning with the PR's goal of generalization. The PR is dependent on a linked kernel PR for the implementation, so the full functionality and impact of the breaking change will depend on that. From the perspective of these header changes alone, the pull request appears ready to be merged, but I am unable to approve it directly. Please have other reviewers approve this code before merging.
3a71707 to
70f54db
Compare
fcd38bf to
04b2412
Compare
5210101 to
ef375d9
Compare
ef375d9 to
7c518e2
Compare
7c518e2 to
9a92f27
Compare
9a92f27 to
e836aa6
Compare
Previous perf semantics are preserved by passing `perf_mode_threads` JIRA: RTOS-1057
e836aa6 to
5a816fb
Compare
Previous perf semantics are preserved by passing
perf_mode_threadsJIRA: RTOS-1057
Description
Motivation and Context
Types of changes
How Has This Been Tested?
Checklist:
Special treatment
!perf: introduce performance analysis subsystem phoenix-rtos-kernel#661