nuttxpr commented on PR #16134: URL: https://github.com/apache/nuttx/pull/16134#issuecomment-2778325864
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the basic NuttX requirements, but it's missing crucial details. While the summary explains the "why" and the "how," it lacks specifics about *what* code is changed. The impact section also needs more details. The testing section is severely inadequate. Here's a breakdown of what's missing and how to improve it: **Summary - Needs Improvement:** * **Specificity:** "Impact is to gettid() system call..." is too vague. Which files were modified? Were any files added or removed? Mention specific functions or data structures affected. For example: "Removed `gettid()` syscall implementation in `kernel/sched/sched_gettid.c`. Modified `arch/riscv/src/riscv64/up_createstack.c` to initialize TID in TLS during thread creation." * **Clarity:** Rephrase "moves the data into TLS for quick userspace access." Explain *how* the TID is placed in TLS. Is it a dedicated TLS slot? Is it part of an existing structure in TLS? **Impact - Needs Significant Improvement:** * **User Impact:** While you imply there's no user impact *other* than performance, explicitly state it: "NO. Existing user code using `gettid()` will continue to function correctly with improved performance. No adaptation is needed." * **Build Impact:** "NO" (presumably). State it explicitly. * **Hardware Impact:** "NO" (presumably). State it explicitly. If it *does* impact specific architectures (e.g., only affects RISC-V), mention them. * **Documentation Impact:** "YES". Document the change. Specify where the documentation changes are (e.g., "Updated the syscall documentation in `docs/kernel/syscalls.txt`"). * **Security Impact:** Consider potential security implications. Does storing the TID in TLS introduce any new vulnerabilities? Even if the answer is "NO," explicitly state it and briefly justify why. For instance, "NO. The TID is already accessible to user space, so storing it in TLS does not introduce new security risks." * **Compatibility Impact:** "NO" (presumably, as existing code should still work). State it explicitly and briefly explain why. E.g., "NO. This change is backward compatible as existing calls to `gettid()` are still handled. There is no forward compatibility impact anticipated." * **Other Considerations:** Are there any performance benchmarks to show the improvement? Any potential corner cases or edge conditions to consider? **Testing - Needs Significant Improvement:** * **Insufficient Detail:** "rv-virt:knsh64 + ostest" is not enough. What specific tests within `ostest` were run? Which commands in `knsh64` were used? What were the *expected* results? * **Missing Logs:** Provide actual log output *before* and *after* the change to demonstrate the difference in behavior or performance. Even if the output is just the return value of `gettid()`, include it. * **More Test Cases:** Test different scenarios: creating multiple threads, calling `gettid()` from different points in the code, etc. Consider including a simple user-space test program that repeatedly calls `gettid()`. * **Performance Measurements:** If performance is a key benefit, include benchmarks showing the improvement. By addressing these points, the PR will be much more complete and easier for reviewers to evaluate. A well-written PR significantly increases the chances of it being accepted. -- 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
