nuttxpr commented on PR #15820: URL: https://github.com/apache/nuttx/pull/15820#issuecomment-2653009509
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR summary does not fully meet the NuttX requirements. Here's why and what's missing: * **Summary (Insufficient Detail):** While it states *what* is changing, it lacks sufficient detail on *why*. Why is balancing the interrupt load important? What problem does this solve? What was the behavior before, and what is the behavior after? Be more specific about the functional code being changed (e.g., names of functions or files modified). * **Impact (Incomplete):** While mentioning the MPFS platform, it's not specific enough. Which MPFS platform? "Impact is MPFS platform, SMP mode only" implies other areas are *not* impacted, which needs explicit confirmation (NO for all others). Even for the MPFS platform, more detail is required: * **Is new feature added? Is existing feature changed?** (This is completely missing). * While user impact might be none, you should explicitly state "NO". Same for build process, documentation, security, and compatibility. If it's "YES" for any of these, describe the impact. * **Testing (Severely Lacking):** "Tested with downstream MPFS target" isn't helpful. What's the downstream target? What are the specific configurations? The "Testing logs before change" and "Testing logs after change" sections are empty. You *must* provide concrete evidence demonstrating the change works as intended. Include relevant log snippets demonstrating the interrupt behavior before and after the change, ideally showing a more balanced distribution. Also, specify the build host details. **Example of how to improve the PR Summary:** "Currently, enabling an external interrupt source only enables the interrupt on the calling hart (CPU core). In multi-core systems (SMP), this can lead to an unbalanced interrupt load, particularly if the interrupt source is enabled only once, which typically occurs on CPU 0. This change modifies the `irq_attach` function (and potentially related functions like `enable_irq`) in the `arch/xxx/src/xxx/irq.c` file to enable the interrupt on all harts when an external interrupt source is attached. This aims to distribute interrupt handling more evenly across available cores, improving responsiveness and potentially performance in multi-core scenarios, especially on the MPFS platform." **Example of how to improve the Impact Section:** * **Is new feature added?** NO * **Is existing feature changed?** YES (Interrupt enabling behavior modified to distribute across all cores for external interrupt sources) * **Impact on user:** NO * **Impact on build:** NO * **Impact on hardware:** YES (Affects interrupt handling on MPFS platform, specifically the XYZ board, in SMP mode. No impact on single-core operation.) * **Impact on documentation:** YES (Documentation for `irq_attach` and related functions should be updated to reflect the change in behavior). Provide a link to the documentation update or indicate where the updates will be made. * **Impact on security:** NO * **Impact on compatibility:** NO * **Anything else to consider:** Potentially a slight increase in interrupt latency immediately after enabling an interrupt, as all cores need to be updated. This should be negligible in most cases. **Example of how to improve the Testing Section:** * **Build Host(s):** Linux (Ubuntu 20.04), x86_64, GCC 9.4.0 * **Target(s):** MPFS-based XYZ board, sim/qemu-rv64 (RISC-V), Configuration: `configs/xyz_board/smp` **Testing logs before change:** (Show logs demonstrating interrupts handled primarily on CPU 0) ``` ... interrupt handler on CPU0 ... ... interrupt handler on CPU0 ... ... interrupt handler on CPU0 ... (rarely) ... interrupt handler on CPU1 ... ``` **Testing logs after change:** (Show logs demonstrating a more balanced distribution of interrupts across cores) ``` ... interrupt handler on CPU0 ... ... interrupt handler on CPU1 ... ... interrupt handler on CPU0 ... ... interrupt handler on CPU1 ... ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
