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

pthread: remove enter_critical_section in pthread_barrier_wait #14626

Merged
merged 1 commit into from
Nov 15, 2024

Conversation

hujun260
Copy link
Contributor

@hujun260 hujun260 commented Nov 4, 2024

Summary

reason:
We decouple semcount from business logic by using an independent counting variable, which allows us to remove critical sections in many cases.

Impact

pthread_barrier_wait

Testing

Build Host:

OS: Ubuntu 20.04
CPU: x86_64
Compiler: GCC 9.4.0
Configuring NuttX and compile:
$ ./tools/configure.sh -l qemu-armv8a:nsh_smp
$ make
Running with qemu
$ qemu-system-aarch64 -cpu cortex-a53 -smp 4 -nographic
-machine virt,virtualization=on,gic-version=3
-net none -chardev stdio,id=con,mux=on -serial chardev:con
-mon chardev=con,mode=readline -kernel ./nuttx

@github-actions github-actions bot added Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Nov 4, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 4, 2024

[Experimental Bot, please feedback here]

This PR appears to partially meet the NuttX requirements. Here's a breakdown:

Strengths:

  • Provides a reason for the change: Decoupling semcount from business logic to remove critical sections is a valid justification.
  • Specifies the impacted function: pthread_barrier_wait is clearly identified.
  • Lists the build host environment: OS, CPU, and compiler details are provided.
  • Includes build instructions and how to run it: This offers helpful context for reviewers.

Weaknesses (requiring further information in the PR):

  • Missing details in the Summary: While the reason is provided, the "what" and "how" are missing. The summary should explain how the decoupling is achieved and what changes are made to the code. A related NuttX issue number should be included if one exists.
  • Incomplete Impact Assessment: While pthread_barrier_wait is mentioned, the PR needs to explicitly answer all the impact questions (user impact, build impact, hardware impact, documentation impact, security impact, compatibility impact). Just stating the impacted function is insufficient. Even if the answer is "NO," it should be stated explicitly.
  • Missing "before" and "after" testing logs: The provided example logs are placeholders. Actual logs demonstrating the change's effect (e.g., performance improvement, corrected behavior) are crucial. The testing section also lacks information about the target being tested (qemu-armv8a is mentioned in the build instructions but not explicitly in the Testing section). It would also benefit from explaining the test case used to generate these logs.

To fully meet the NuttX requirements, the PR needs to:

  1. Expand the Summary: Explain the technical implementation details and how the change works. Include a related NuttX issue number if applicable.
  2. Complete the Impact Assessment: Explicitly answer all the impact questions (even with "NO" if no impact). Provide details if any impact is "YES."
  3. Provide real testing logs: Include "before" and "after" logs demonstrating the change's effect and explaining the test case used. Clearly state the target architecture and board configuration used for testing.

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

@github-actions github-actions bot added Size: M The size of the change in this PR is medium and removed Size: S The size of the change in this PR is small labels Nov 4, 2024
include/pthread.h Outdated Show resolved Hide resolved
sched/pthread/pthread_barrierwait.c Show resolved Hide resolved
include/pthread.h Outdated Show resolved Hide resolved
include/pthread.h Outdated Show resolved Hide resolved
@github-actions github-actions bot added Size: S The size of the change in this PR is small and removed Size: M The size of the change in this PR is medium labels Nov 15, 2024
sched/pthread/pthread_barrierwait.c Outdated Show resolved Hide resolved
include/pthread.h Outdated Show resolved Hide resolved
reason:
We decouple semcount from business logic by using an independent counting variable,
which allows us to remove critical sections in many cases.

Signed-off-by: hujun5 <hujun5@xiaomi.com>
@@ -82,67 +82,35 @@

int pthread_barrier_wait(FAR pthread_barrier_t *barrier)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's create a new patch moving pthread_barrierwait.c to libc/pthread, since the new implementation doesn't call the kernel internal api.

@xiaoxiang781216 xiaoxiang781216 merged commit 7ba0f11 into apache:master Nov 15, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OS Components OS Components issues Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants