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

Revert "libc/atomic: decoupling atomic and spinlock to avoid recursion" #14218

Merged
merged 3 commits into from
Oct 14, 2024

Conversation

anchao
Copy link
Contributor

@anchao anchao commented Oct 14, 2024

Summary

Revert "pthread: We should not directly include arch/spinlock.h as it may cause compilation errors in C++."
Revert "sched/spinlock: remove nesting spinlock support"
Revert "libc/atomic: decoupling atomic and spinlock to avoid recursion"

Further work(without this feature could be closed):
#14200

Impact

N/A

Testing

ci-check

@nuttxpr
Copy link

nuttxpr commented Oct 14, 2024

[Experimental Bot, please feedback here]

This PR does not meet the NuttX requirements, despite claiming "N/A" for impact.

Here's why:

  • Insufficient Summary: Simply stating "Revert" without explaining the original problem these commits aimed to solve, why they are problematic, and the implications of reverting is insufficient.
  • Missing Impact Details: Reverting changes, especially those dealing with core components like pthread, scheduling, and atomic operations, will undoubtedly have an impact. Claiming "N/A" is misleading and unacceptable. The PR needs to clearly outline potential negative impacts like re-introducing bugs, breaking compatibility, or affecting performance.
  • Vague Testing: "ci-check" isn't descriptive enough. The PR needs to specify the target architectures, boards, and configurations tested, along with relevant logs demonstrating the reverts don't negatively impact functionality.

In short, this PR needs significant work to meet NuttX standards. The author needs to:

  1. Expand the Summary: Provide context for the reverts, explaining the original issue and the rationale for reverting.
  2. Thoroughly Detail Impact: Analyze and document all potential impacts of these reverts, especially negative ones.
  3. Improve Testing Description: Provide specific details about the testing environment and include comprehensive logs showcasing the before/after behavior.

Without these improvements, the PR is incomplete and risks introducing regressions into the NuttX codebase.

@GUIDINGLI
Copy link
Contributor

@lupyuen
Could you help to see what happens to CI in this PR, haven't see any errors.

@GUIDINGLI
Copy link
Contributor

I know what happens, will fix it in a quick time.

@lupyuen
Copy link
Member

lupyuen commented Oct 14, 2024

@GUIDINGLI Yep it's the same problem as apache/nuttx-apps#2716 (comment)

Configuration/Tool: qemu-armv8a/netnsh_smp_hv
  Normalize qemu-armv8a/netnsh_smp_hv
103d102
< CONFIG_SYSTEM_TELNETD_SESSION_STACKSIZE=8192
Saving the new configuration file
HEAD detached at pull/14218/merge

https://github.com/apache/nuttx/actions/runs/11320111668/job/31477184924?pr=14218

@GUIDINGLI
Copy link
Contributor

Here is the fix:
#14229

@anchao
Copy link
Contributor Author

anchao commented Oct 14, 2024

@lupyuen @GUIDINGLI Please merge this PR if CI errors is not related to these commits

@lupyuen
Copy link
Member

lupyuen commented Oct 14, 2024

@GUIDINGLI @xiaoxiang781216 I'll let you decide when to merge this, since we are busy fixing the CI right now thanks :-)

@GUIDINGLI
Copy link
Contributor

@GUIDINGLI @xiaoxiang781216 I'll let you decide when to merge this, since we are busy fixing the CI right now thanks :-)

OK, I am waiting for the CI result.

@GUIDINGLI
Copy link
Contributor

@anchao
Please help to rebase this PR, thanks

@lupyuen
Copy link
Member

lupyuen commented Oct 14, 2024

@GUIDINGLI Yep I forked the NuttX Repo 30 mins ago, the CI looks OK so far :-)
https://github.com/lupyuen5/label-nuttx/actions/runs/11321795057

@GUIDINGLI
Copy link
Contributor

Still CI error:
#14252

@GUIDINGLI
Copy link
Contributor

@lupyuen
One suggestion, can we move the defconfig check to coding style check stage ?
Just configure & savedefconfig compare.
Then we can catch these questions in time before the compiling all the chips.

@lupyuen
Copy link
Member

lupyuen commented Oct 14, 2024

One suggestion, can we move the defconfig check to coding style check stage ?
Just configure & savedefconfig compare.

@GUIDINGLI Lemme think about this, the CI Build Flow is a little complicated, I'm still learning :-)

Also I wonder: Do we really need to build so many Arm Targets every time? If we build only the latest targets (e.g. SAMV7, Goldfish), then the problems will show up earlier. (And reduce our cost of GitHub Actions)

Or we should move SAMV7, Goldfish etc to the first job arm-01, keep the job small, so that it will fail earlier.

@GUIDINGLI
Copy link
Contributor

One suggestion, can we move the defconfig check to coding style check stage ?
Just configure & savedefconfig compare.

@GUIDINGLI Lemme think about this, the CI Build Flow is a little complicated, I'm still learning :-)

Also I wonder: Do we really need to build so many Arm Targets every time? If we build only the latest targets (e.g. SAMV7, Goldfish), then the problems will show up earlier. (And reduce our cost of GitHub Actions)

Or we should move SAMV7, Goldfish etc to the first job arm-01, keep the job small, so that it will fail earlier.

Agreed with you.

I think we only need keep:
armv8-m armv8-a armv8-r
armv7-m armv7-a armv7-r
armv6-m
Each of them keep two/three boards is enough,
Each of them:
Flat mode, Kernel mode(if support), Qemu (if support)

How about others think about it ?

@lupyuen
Copy link
Member

lupyuen commented Oct 14, 2024

Let's discuss the Arm32 Targets here thanks!

@xiaoxiang781216 xiaoxiang781216 merged commit 2a65a34 into apache:master Oct 14, 2024
36 of 39 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: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants