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

sched/spinlock: remove nesting spinlock support #14079

Merged
merged 1 commit into from
Oct 12, 2024

Conversation

anchao
Copy link
Contributor

@anchao anchao commented Oct 11, 2024

Summary

sched/spinlock: remove nesting spinlock support

developers must be careful to hold spinlocks and ensure all of protected code is under control, so remove support for nested spinlocks to improve performance.

Signed-off-by: chao an [email protected]

Impact

N/A

Testing

ci-check

developers must be careful to hold spinlocks and ensure all
of protected code is under control, so remove support for nested
spinlocks to improve performance.

Signed-off-by: chao an <[email protected]>
@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 Oct 11, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 11, 2024

[Experimental Bot, please feedback here]

No, this PR does not meet the NuttX requirements.

Summary:

  • The summary is too brief and lacks details.
  • It doesn't explain why removing nested spinlock support is necessary beyond a general statement about developer responsibility and performance.
  • Concrete examples of performance improvements or potential issues caused by nested spinlocks are needed.
  • It's unclear what "functional part of the code" is being changed beyond a vague mention of "sched/spinlock".

Impact:

  • Incorrectly stating "N/A" for impact is a major red flag. Removing a feature like nested spinlocks will have impact:
    • Impact on user: Existing code that relies on nested spinlocks will break. This needs to be clearly stated, and a migration path or alternative solution should be provided.
    • Impact on documentation: Documentation must be updated to reflect the removal of nested spinlock support.
    • Impact on compatibility: This change breaks backward compatibility. This needs to be explicitly mentioned.

Testing:

  • Simply stating "ci-check" is insufficient.
  • The PR needs to provide specific details about the testing environment:
    • Build Host(s): List the operating systems, CPUs, and compiler versions used for testing.
    • Target(s): Specify the architectures, boards, and configurations tested.
  • Testing logs before and after the change are missing. Concrete evidence demonstrating the change's impact is crucial.

In addition to the points above, the PR should also address the following:

  • Alternatives: Are there alternative solutions to improve performance without completely removing nested spinlock support?
  • Risk Assessment: What are the risks associated with this change? Are there any potential negative side effects?
  • Code Examples: Provide code examples demonstrating how the change will affect existing code and how developers should adapt.

Recommendation:

The author should revise the PR to address the missing information and provide a more comprehensive explanation of the change, its impact, and the testing performed.

@xiaoxiang781216 xiaoxiang781216 merged commit 5aa13bc into apache:master Oct 12, 2024
35 of 37 checks passed
@raiden00pl
Copy link
Member

@anchao hi, this PR introduced new warnings in sim/smp. Look at: #14167

@masayuki2009
Copy link
Contributor

@anchao

I noticed that spresense:wifi_smp causes a deadlock with this PR.
I think this PR would affect some SMP configurations that use real hardware.
So I suggest we revert this PR until all the issues have been resolved.

nsh> uname -a
NuttX  12.0.0 5aa13bc490 Oct 13 2024 07:07:59 arm spresense
nsh> ps
  PID GROUP CPU PRI POLICY   TYPE    NPX STATE    EVENT     SIGMASK           STACK   USED  FILLED COMMAND
    0     0   0   0 FIFO     Kthread   - Assigned           0000000000000000 0001008 0000444  44.0%  CPU0 IDLE
    1     0   1   0 FIFO     Kthread   - Running            0000000000000000 0001008 0000272  26.9%  CPU1 IDLE
    2     0 --- 224 RR       Kthread   - Waiting  Semaphore 0000000000000000 0001984 0000304  15.3%  hpwork 0x2d061464 0x2d061488
    3     0 ---  60 RR       Kthread   - Waiting  Semaphore 0000000000000000 0001984 0000304  15.3%  lpwork 0x2d06142c 0x2d061450
    5     5 --- 200 RR       Task      - Waiting  MQ empty  0000000000000000 0000984 0000464  47.1%  cxd56_pm_task
    6     6   0 100 RR       Task      - Running            0000000000000000 0003032 0001304  43.0%  spresense_main
nsh> free
      total       used       free    maxused    maxfree  nused  nfree name
    1136344      34360    1101984      35424    1101280     88      2 Umem
nsh> ifconfig
nsh> mount
  /mnt/sd0 type vfat
  /mnt/spif type smartfs
  /proc type procfs
nsh> gs2200m aterm-ac16fc-g wifi-test-24g & 
gs2200m [7:50]
nsh> renew wlan0
nsh> ntpcstart
Started the NTP daemon as PID=10
nsh> ifconfig
wlan0	Link encap:Ethernet HWaddr 3c:95:09:00:82:d3 at UP mtu 1500
	inet addr:192.168.10.20 DRaddr:192.168.10.1 Mask:255.255.255.0

@anchao
Copy link
Contributor Author

anchao commented Oct 13, 2024

@anchao

I noticed that spresense:wifi_smp causes a deadlock with this PR. I think this PR would affect some SMP configurations that use real hardware. So I suggest we revert this PR until all the issues have been resolved.

Ok, I don't have a spresense board, let's revert this commit first

@anchao
Copy link
Contributor Author

anchao commented Oct 13, 2024

@masayuki2009 could you please help to try PR #14200 is works for you? if not, I will solve this issue until I get the spresense board

hujun260 added a commit to hujun260/nuttx that referenced this pull request Nov 25, 2024
…qsave

reason:
1 There is a similar PR, apache#14079,
2 Currently, no one is using recursive locks with write_lock_irqsave/read_lock_irqsave.
3 Nested spinlock is harmful, prone to abuse and leading to a decline in code quality and performance
4 Nested spinlock is also not available in Linux.
5 In our future plans, nested usage of enter_critical_section and spin_lock_irqsave will also be removed.

Signed-off-by: hujun5 <[email protected]>
xiaoxiang781216 pushed a commit that referenced this pull request Nov 26, 2024
…qsave

reason:
1 There is a similar PR, #14079,
2 Currently, no one is using recursive locks with write_lock_irqsave/read_lock_irqsave.
3 Nested spinlock is harmful, prone to abuse and leading to a decline in code quality and performance
4 Nested spinlock is also not available in Linux.
5 In our future plans, nested usage of enter_critical_section and spin_lock_irqsave will also be removed.

Signed-off-by: hujun5 <[email protected]>
JaeheeKwon pushed a commit to JaeheeKwon/nuttx that referenced this pull request Nov 28, 2024
…qsave

reason:
1 There is a similar PR, apache#14079,
2 Currently, no one is using recursive locks with write_lock_irqsave/read_lock_irqsave.
3 Nested spinlock is harmful, prone to abuse and leading to a decline in code quality and performance
4 Nested spinlock is also not available in Linux.
5 In our future plans, nested usage of enter_critical_section and spin_lock_irqsave will also be removed.

Signed-off-by: hujun5 <[email protected]>
linguini1 pushed a commit to CarletonURocketry/nuttx that referenced this pull request Jan 15, 2025
…qsave

reason:
1 There is a similar PR, apache#14079,
2 Currently, no one is using recursive locks with write_lock_irqsave/read_lock_irqsave.
3 Nested spinlock is harmful, prone to abuse and leading to a decline in code quality and performance
4 Nested spinlock is also not available in Linux.
5 In our future plans, nested usage of enter_critical_section and spin_lock_irqsave will also be removed.

Signed-off-by: hujun5 <[email protected]>
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.

6 participants