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

mm/iob: add ostest test #2849

Merged
merged 1 commit into from
Nov 17, 2024
Merged

mm/iob: add ostest test #2849

merged 1 commit into from
Nov 17, 2024

Conversation

hujun260
Copy link
Contributor

@hujun260 hujun260 commented Nov 13, 2024

Summary

testing: add iob test

Impact

iob

Testing

ostest

@nuttxpr
Copy link

nuttxpr commented Nov 13, 2024

[Experimental Bot, please feedback here]

No, this PR description does not adequately meet the NuttX requirements. Here's why:

  • Insufficient Summary: While it mentions adding configurations to ostest, it doesn't explain why these changes are necessary. Is it fixing a bug? Adding a new feature? Improving performance? What part of ostest is affected? How do these configuration options interact and change the behavior?
  • Vague Impact: Simply stating "iob" isn't helpful. Be specific about how these IOB configurations impact the user, the build process, hardware, documentation, security, and compatibility. For example, do these changes increase memory usage? Do they change the behavior of any tests? Do they require any new dependencies?
  • Missing Testing Details: "ostest" is not sufficient testing information. Specify the host operating system, compiler, and target architecture and board configuration. Provide actual testing logs (or links to them) demonstrating the behavior before and after the change. Just saying "ostest" doesn't prove anything.

Example of how to improve this PR description:

Summary

This change adds IOB configurations to the ostest example to demonstrate and test the IOB throttling mechanism. This is necessary to verify the correct behavior of IOB throttling under various buffer and chain configurations. Specifically, it enables CONFIG_IOB_THROTTLE and sets related parameters like CONFIG_IOB_NBUFFERS, CONFIG_IOB_NCHAINS, and enables the notifier. This allows testing of the IOB allocation behavior when the number of allocated buffers exceeds the throttle limit.

Impact

  • Is new feature added? Is existing feature changed? No new feature is added, but existing ostest functionality is extended to demonstrate IOB throttling.
  • Impact on user: NO. This change is specific to the ostest example and doesn't impact regular users of NuttX.
  • Impact on build: NO. No changes to the build process are required.
  • Impact on hardware: NO. No specific hardware dependencies are introduced.
  • Impact on documentation: YES. The ostest documentation should be updated to explain the new configurations and their purpose. (A separate documentation PR will be created for this).
  • Impact on security: NO.
  • Impact on compatibility: NO.
  • Anything else to consider? None.

Testing

I confirm that changes are verified on local setup and works as intended:

  • Build Host(s): Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
  • Target(s): qemu-x86_64:nsh

Testing logs before change:

(Previous ostest output showing IOB allocation without throttling - include relevant snippets)

Testing logs after change:

(New ostest output showing IOB allocation with throttling engaged - demonstrating the intended throttling behavior)

Copy link

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Thank you @hujun260 :-)

Could you also please add Kconfig part so it stays in sync with CMake? :-)

Copy link

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Perfect! Thank you @hujun260 :-)

Signed-off-by: hujun5 <hujun5@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit d9d8518 into apache:master Nov 17, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants