arch/risc-v/mpfs: Remove big kernel lock from several drivers

[pussuw]

Summary

This replaces the big kernel lock (enter/leave_critical_section()) with local spinlocks from all MPFS drivers that still use it. The reason is obviously to alleviate big kernel lock congestion which has a massive performance impact in SMP

Impact

Only the MPFS target is impacted.
App impact: No
Doc impact: No
Build system impact: No

Testing

Tested with private downstream MPFS target that uses all of the changed drivers.

[nuttxpr]

[Experimental Bot, please feedback here]

The PR mostly meets the NuttX requirements but is missing some crucial information. Here's a breakdown:

Strengths:

• Clear Summary of "Why" and "What": The summary clearly explains the purpose of the change (performance improvement by replacing the big kernel lock) and which part of the code is affected (MPFS drivers).
• Testing Confirmation: The author confirms testing, which is essential.
• Concise Impact Assessment: The impact section is concise and addresses several key areas.

Weaknesses and Missing Information:

• Insufficient "How" in Summary: The summary lacks detail on how the change is implemented. It mentions using local spinlocks, but doesn't describe the implementation strategy. Did they create new spinlocks for each driver? How are they integrated? This detail is critical for reviewers.
• Lack of Issue References: There's no mention of related NuttX or NuttX Apps issues. If this addresses a known issue, those links are important.
• Vague Testing Description: "Tested with private downstream MPFS target" is too vague. Reviewers need more specifics:
  • Which MPFS target? Provide the configuration.
  • What tests were run? Just stating "it works as intended" is insufficient. Describe the specific tests and their results. How was performance measured and what were the improvements?
  • "Private downstream" is a red flag. Ideally, tests should be reproducible by others. Consider how to make the test setup more accessible (e.g., providing configuration details or contributing the downstream target back upstream if possible).
• Missing "Before/After" Logs: The testing logs sections are empty. These are crucial for demonstrating the impact of the change and verifying its correctness. Include relevant log snippets or performance measurements.
• Incomplete Impact Assessment:
  • Hardware Impact: While the PR says only the MPFS target is impacted, it should specify which architectures/boards were tested. Changes to locking mechanisms can have subtle architecture-specific effects.
  • Security Impact: Changes to locking could have security implications (e.g., introducing new race conditions). Even if the impact is "NO," it's important to explicitly state that the security implications were considered.
  • Compatibility Impact: Similarly, backward compatibility should be explicitly addressed, even if the answer is "NO."
• Missing Build Host Information: The testing section doesn't describe the build host environment used.

Recommendation:

The PR needs revisions to address the missing information and improve the level of detail. Specifically, the author should:

1. Expand the "How" in the summary: Describe the spinlock implementation strategy.
2. Link related issues: If applicable.
3. Provide detailed testing information: Specify the target, configuration, tests performed, and performance results. Make the test setup reproducible if possible.
4. Include "Before/After" logs: Demonstrate the change's impact with concrete data.
5. Complete the impact assessment: Provide specific information about hardware tested, security considerations, and backward compatibility.
6. Specify the build host environment: OS, CPU, compiler.

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

[acassis]

@pussuw nice work! Is there some way to test and display how much it improved the NuttX performance?

[pussuw]

@pussuw nice work! Is there some way to test and display how much it improved the NuttX performance?

I cannot share logs but in our use case total CPU load drops by ~3-5% in SMP mode. This is of course very situational.

The big kernel lock is very contested because semaphores use it so removing its use from modules that don't need it usually has a noticeable impact.