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

Multiple threads memorystress #2912

Merged
merged 2 commits into from
Dec 23, 2024

Conversation

crafcat7
Copy link
Contributor

Summary

In the original memstress multithreading, multiple threads shared a context. This is not enough for the stress testing of the use case, so we modified it so that each thread has an independent context to increase the overall memory pressure.

  1. In a multi-threaded scenario, each thread has its own context
  2. Adjust code structure

Impact

This slightly changes the usage, some parameters now have initial values.

Testing

Linux
sim:nsh

nsh> memstress
MemoryStress:
 max_allocsize: 8192
 nodelen: 1024
 sleep_us: 100
 nthreads: 1
 debug: false
MemoryStress:testing...

Summary:
  1. In a multi-threaded scenario, each thread has its own context
  2. Adjust code structure

Signed-off-by: chenrun1 <[email protected]>
Summary:
CC:  memorystress_main.c memorystress_main.c: In function ‘global_init’:
memorystress_main.c:39:26: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 5 has type ‘uint32_t’ {aka ‘unsigned int’} [-Wformat=]
   39 | #define MEMSTRESS_PREFIX "MemoryStress:"
      |                          ^~~~~~~~~~~~~~~
memorystress_main.c:442:22: note: in expansion of macro ‘MEMSTRESS_PREFIX’
  442 |     syslog(LOG_INFO, MEMSTRESS_PREFIX "\n max_allocsize: %zu\n"
      |                      ^~~~~~~~~~~~~~~~
memorystress_main.c:443:41: note: format string is defined here
  443 |            " nodelen: %zu\n sleep_us: %lu\n nthreads: %zu\n debug: %s\n",
      |                                       ~~^
      |                                         |
      |                                         long unsigned int
      |                                       %u

Signed-off-by: chenrun1 <[email protected]>
@nuttxpr
Copy link

nuttxpr commented Dec 23, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to mostly meet the NuttX requirements, but it's missing some crucial information.

Here's what's good:

  • Clear Summary: Explains the "why" and "how" of the change. The mention of increasing memory pressure is helpful context.
  • Impact Acknowledged: The impact on the user (parameter changes) is mentioned.
  • Testing Provided (Partially): Testing logs are included, although minimal.

Here's what needs improvement:

  • Missing Issue References: If this relates to a specific NuttX issue, include the link. Even if it doesn't directly address an existing issue, consider creating one to track the improvement.
  • Incomplete Impact Assessment: The impact section needs significant expansion. Specifically address all the points:
    • Is new feature added? Is existing feature changed? (This is answered implicitly but should be stated explicitly).
    • Impact on build? Probably NO, but state it explicitly.
    • Impact on hardware? Likely NO, but state it.
    • Impact on documentation? If the usage changed, the documentation needs updating. State whether this update is included in the PR.
    • Impact on security? Unlikely, but state it.
    • Impact on compatibility? Important! Does this break any existing usage?
  • Insufficient Testing Details: The testing information is too vague.
    • Build Host: Provide more detail about your Linux build host (distribution, kernel version, GCC version).
    • Target: sim:nsh isn't enough detail. Which simulator? qemu? What configuration?
    • Before/After Logs: The logs are almost identical and don't show the actual effect of the change. Show logs demonstrating the multi-threaded behavior before and after the change, highlighting the increased memory pressure. Include metrics like memory usage, allocation failures (if any), and execution time.

In short: The core information is there, but the PR needs more detail and more thorough testing information to be properly reviewed and accepted.

@xiaoxiang781216 xiaoxiang781216 merged commit 42d6ae6 into apache:master Dec 23, 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.

3 participants