Skip to content

Conversation

@adamgreloch
Copy link
Member

@adamgreloch adamgreloch commented Jun 2, 2025

JIRA: RTOS-1057

Description

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (refactoring, style fixes, git/CI config, submodule management, no code logic changes)

How Has This Been Tested?

  • Already covered by automatic testing.
  • New test added: (add PR link here).
  • Tested by hand on: (list targets here).

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing linter checks and tests passed.
  • My changes generate no new compilation warnings for any of the targets.

Special treatment

Copy link

@gemini-code-assist gemini-code-assist bot left a 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! I'm Gemini, your friendly code-assist bot. I've analyzed this pull request to give you a quick summary.

This pull request adapts the perf command-line utility within the psh shell to work with a new kernel syscall interface for performance monitoring. The primary change involves modifying the perf command to accept a 'mode' argument (currently supporting 'threads' and 'trace') as the first parameter, shifting the optional timeout argument to the second position. The underlying syscalls (perf_start, perf_read, perf_finish) are updated to accept this new mode parameter, reflecting changes in how the kernel's performance monitoring is accessed and controlled. The type of the buffer used to read performance data has also been updated, suggesting a change in the data format provided by the new syscall interface.

Highlights

  • Adaptation to New Syscall Interface: The core purpose of this PR is to update the psh perf tool to utilize a new kernel syscall interface for performance monitoring.
  • Introduction of Perf Modes: The perf command now supports different operational modes, specified as the first argument. Currently, 'threads' and 'trace' modes are introduced.
  • Command Line Argument Changes: The command syntax for perf has changed. It now expects the mode as the first argument, followed by the optional timeout as the second argument.
  • Updated Syscall Usage: Calls to perf_start, perf_read, and perf_finish have been modified to include the newly introduced mode parameter.
  • Performance Data Buffer Type Change: The type of the buffer used to read performance data from the kernel has been changed from perf_event_t * to uint32_t *, indicating a potential change in the data structure or format returned by the new syscalls.

Changelog

  • psh/perf/perf.c
    • Updated copyright year to 2025 and added the author to the list.
    • Added necessary includes for <string.h> and <sys/perf.h>.
    • Introduced a perfHelp function to display usage information.
    • Modified the psh_perf function to parse a mode argument ('threads' or 'trace') and validate it.
    • Shifted the timeout argument parsing to the second command-line argument.
    • Changed the type of the buffer used for reading performance data from perf_event_t * to uint32_t *.
    • Updated calls to perf_start, perf_read, and perf_finish to pass the selected mode.
    • Updated informational and error messages to include the active mode.
    • Applied minor formatting fix in the perf_registerapp function.
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.


Syscall changes come,
Perf tool adapts its old ways,
New modes start to trace.

Footnotes

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

Copy link

@gemini-code-assist gemini-code-assist bot left a 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 adapts the perf utility to a new syscall interface, allowing it to operate in different modes (threads or trace) and handle a timeout argument. The changes involve updating includes, adding a help function, modifying argument parsing, and updating calls to the perf_start, perf_read, and perf_finish functions to include the new mode parameter. The error messages have also been improved to include the selected mode.

The overall structure of the changes seems appropriate for integrating the new syscall interface. The addition of the perfHelp function and improved error messages are good improvements for usability.

Summary of Findings

  • Buffer type change: The buffer type for reading performance data was changed from perf_event_t * to uint32_t *. Clarification is needed on the new data format returned by perf_read to ensure correctness and maintainability. (Medium severity - commented)
  • Copyright and Author Update: The copyright year and author list were updated. (Low severity - not commented per settings)
  • Code Formatting: Minor formatting changes were made in the perf_registerapp function initialization. (Low severity - not commented per settings)

Merge Readiness

This pull request introduces necessary changes to adapt the perf utility to the new syscall interface. There is one medium severity issue identified regarding the buffer type change and the expected data format from the new perf_read syscall. This needs clarification to ensure the code is correct and maintainable.

Therefore, I recommend addressing the medium severity issue before merging. I am unable to approve the pull request directly; please have other reviewers review and approve this code before merging.

@adamgreloch adamgreloch force-pushed the adamgreloch/RTOS-1057 branch 2 times, most recently from ec8069c to bcf637a Compare June 2, 2025 09:17
@adamgreloch adamgreloch marked this pull request as ready for review June 2, 2025 09:17
@adamgreloch adamgreloch requested a review from Darchiv June 2, 2025 09:17
@adamgreloch adamgreloch marked this pull request as draft July 21, 2025 14:49
@adamgreloch adamgreloch force-pushed the adamgreloch/RTOS-1057 branch 3 times, most recently from 492befd to e0f8da7 Compare July 23, 2025 07:29
@adamgreloch adamgreloch force-pushed the adamgreloch/RTOS-1057 branch 4 times, most recently from 3b814bb to f49a59a Compare August 14, 2025 12:42
@adamgreloch adamgreloch marked this pull request as ready for review August 14, 2025 13:51
@adamgreloch adamgreloch force-pushed the adamgreloch/RTOS-1057 branch 6 times, most recently from f43a055 to e88ddb8 Compare August 20, 2025 08:33
@adamgreloch adamgreloch force-pushed the adamgreloch/RTOS-1057 branch 2 times, most recently from 9f265cc to 3e722f6 Compare September 1, 2025 08:43
@adamgreloch adamgreloch changed the title perf: adapt to new perf syscall interface perf: adapt to new perf subsystem Sep 1, 2025
@adamgreloch adamgreloch force-pushed the adamgreloch/RTOS-1057 branch 2 times, most recently from 9c24689 to e06d000 Compare September 12, 2025 11:40
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.

2 participants