arch: change nxsched_suspend/resume_scheduler() called position

[GUIDINGLI]

Summary

arch: change nxsched_suspend/resume_scheduler() called position

for the citimon stats:

thread 0:                     thread 1:
enter_critical (t0)
up_switch_context
note suspend thread0 (t1)

                              thread running
                              IRQ happen, in ISR:
                                post thread0
                                up_switch_context
                                note resume thread0 (t2)
                                ISR continue f1
                                ISR continue f2
                                ...
                                ISR continue fn

leave_critical (t3)

You will see, the thread 0, critical_section time is: (t1 - t0) + (t3 - t2)

BUT, this result contains f1 f2 .. fn time spent, it is wrong to tell user thead0 hold the critical lots of time but actually not belong to it.

Resolve:
change the nxsched_suspend/resume_scheduler to real hanppends

Impact

critmon

Testing

qemu

./tools/configure.sh mps2-an500:nsh -j8

[nuttxpr]

[Experimental Bot, please feedback here]

NuttX PR Requirements Review

Meets Requirements? Partially

Summary:

While the PR addresses a specific issue with the citimon statistics, the provided information lacks clarity and completeness.

Strengths:

• Problem Clearly Identified: The issue with inaccurate critical section time measurement in citimon is well-explained.
• Solution Provided: Moving nxsched_suspend/resume_scheduler calls is a reasonable approach.

Weaknesses:

• Impact Underspecified: "critmon" is too vague. Be explicit about:
  • Which critmon values are affected?
  • How will users observe the change? Is it just accuracy, or are new metrics exposed?
• Testing Inadequate:
  • Limited Scope: "qemu" and one configuration is insufficient. Testing should cover:
    • Real hardware, if possible
    • Multiple architectures (if the change is architecture-dependent)
    • Configurations where critical sections are heavily used
  • No Logs: Provide actual log snippets demonstrating the issue before the change and the improvement after. This is crucial for reviewers.
• Missing Details:
  • What is the exact change to nxsched_suspend/resume_scheduler? A code snippet illustrating the modification would be helpful.
  • Are there any potential downsides to this change? E.g., could it impact performance or introduce new race conditions?

Recommendations:

1. Expand Impact: Clearly articulate the specific changes to critmon and how users will perceive them.
2. Thorough Testing: Test on a wider range of platforms/configurations and provide concrete log output showing the before/after difference.
3. Illustrate the Change: Include a code snippet to highlight how nxsched_suspend/resume_scheduler is modified.
4. Consider Downsides: Discuss any potential negative consequences of the change.

By addressing these points, the PR will be much stronger and easier for reviewers to evaluate.

[anchao]

It seems that this PR will cause the task CET(Case Execution Time) to be worst, right?

[xiaoxiang781216]

no, both functions are only used to collect the statistics info, don't influence the behavior. without the fix, critmonitor report the wrong value.

[masayuki2009]

@GUIDINGLI
I noticed that esp32-devkitc:elf (dev board and QEMU) does not work with this PR.

[GUIDINGLI]

Got it, thanks, I will fix it asap.

[GUIDINGLI]

@masayuki2009

Fix & fully test in:
#13662

[GUIDINGLI]

@masayuki2009

By the way, can you share your autotest script on QEMU, (all the qemu)
compile & run cmds

Maybe we can integrate it to CI