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

Reapply "sched/spinlock: remove nesting spinlock support" #14203

Merged
merged 1 commit into from
Oct 13, 2024

Conversation

hujun260
Copy link
Contributor

Summary

since #14200 fix the dead lock.
we reapply #14190

Impact

none

Testing

Build Host(s): Linux x86
Target(s): sim/smp

After that, there will be no warnings for compilation
make distclean -j20; ./tools/configure.sh -l sim:smp;make -j20
Copy files
Select CONFIG_HOST_LINUX=y
Refreshing...
CP: arch/dummy/Kconfig to /home/hujun5/downloads1/vela_sim/nuttx/arch/dummy/dummy_kconfig
CP: boards/dummy/Kconfig to /home/hujun5/downloads1/vela_sim/nuttx/boards/dummy/dummy_kconfig
LN: platform/board to /home/hujun5/downloads1/vela_sim/apps/platform/dummy
LN: include/arch to arch/sim/include
LN: include/arch/board to /home/hujun5/downloads1/vela_sim/nuttx/boards/sim/sim/sim/include
LN: drivers/platform to /home/hujun5/downloads1/vela_sim/nuttx/drivers/dummy
LN: include/arch/chip to /home/hujun5/downloads1/vela_sim/nuttx/arch/sim/include/sim
LN: arch/sim/src/chip to /home/hujun5/downloads1/vela_sim/nuttx/arch/sim/src/sim
LN: arch/sim/src/board to /home/hujun5/downloads1/vela_sim/nuttx/boards/sim/sim/sim/src
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/examples/mcuboot
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/examples
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/crypto
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/audioutils
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/logging
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/system
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/fsutils
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/benchmarks
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/database
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/interpreters/luamodules
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/interpreters
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/boot
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/testing
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/mlearning
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/graphics
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/netutils
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/sdr
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/canutils
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/lte
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/inertial
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/math
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/wireless/ieee802154
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/wireless/bluetooth
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/wireless
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/videoutils
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/games
mkkconfig in /home/hujun5/downloads1/vela_sim/apps/industry
mkkconfig in /home/hujun5/downloads1/vela_sim/apps
Loaded configuration '.config'
Configuration saved to '.config'
Create version.h
LN: platform/board to /home/hujun5/downloads1/vela_sim/apps/platform/dummy
Register: hello
Register: nsh
Register: sh
Register: taskset
Register: ostest
Register: getprime
Register: smp
CP: /home/hujun5/downloads1/vela_sim/nuttx/include/nuttx/config.h
CP: /home/hujun5/downloads1/vela_sim/nuttx/include/nuttx/fs/hostfs.h
LD: 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 Oct 13, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 13, 2024

[Experimental Bot, please feedback here]

The PR summary is missing some key details:

  • Why: It mentions re-applying a previous PR due to a deadlock fix, but doesn't clearly explain what the original PR aimed to achieve.
  • What: It lacks specifics about the functional code changes being made.
  • How: No explanation on how the re-applied changes work.

Impact section is too brief. While stating "none" might be accurate, it's better to explicitly confirm no impact on:

  • User experience
  • Build process (despite showing build commands, it's not a confirmation)
  • Hardware compatibility
  • Documentation needs
  • Security implications
  • Backward/forward compatibility

Testing section has issues:

  • Only mentions build hosts and target; needs more details about the testing environment (OS versions, compiler versions).
  • It claims "no warnings for compilation" but doesn't prove it. The provided logs are build logs, not compilation logs.
  • No "before change" logs are present, making it impossible to assess the impact of the changes.

Verdict: This PR does not meet the NuttX requirements. It lacks crucial information in the summary and impact sections, and the testing section doesn't provide sufficient evidence of verification.

@xiaoxiang781216 xiaoxiang781216 merged commit b964eee into apache:master Oct 13, 2024
16 of 39 checks passed
@lupyuen
Copy link
Member

lupyuen commented Oct 13, 2024

Wonder why this telnetd build is failing? It doesn't seem to be caused by the PR.
https://github.com/apache/nuttx/actions/runs/11313625049/job/31462630855

Configuration/Tool: zkit-arm-1769/nxhello,CONFIG_ARM_TOOLCHAIN_GNU_EABI
2024-10-13 11:11:50
telnetd.c: In function 'telnetd_main':
Error: telnetd.c:57:5: error: 'CONFIG_SYSTEM_TELNETD_SESSION_STACKSIZE' undeclared (first use in this function); did you mean 'CONFIG_SYSTEM_TELNETD_SESSION_PRIORITY'?
   57 |     CONFIG_SYSTEM_TELNETD_SESSION_STACKSIZE,
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |     CONFIG_SYSTEM_TELNETD_SESSION_PRIORITY

@xiaoxiang781216
Copy link
Contributor

@lupyuen fix by apache/nuttx-apps#2716

@hujun260 hujun260 deleted the apache_17 branch October 13, 2024 23:31
@masayuki2009
Copy link
Contributor

since #14200 fix the dead lock.

@anchao @hujun260 @xiaoxiang781216

I tried this PR with #14200, the nsh deadlock issue that I reported in #14079 (comment) was resolved.

However, if I run nxplayer repeatedly with spresense:rndis_smp, deadlock happens.
I think we would find other deadlocks if we test more complicated use cases.

As I commented in #14079 (comment), I think it's too early to remove nesting spinlock support. So again, I suggest we revert this PR.

@anchao
Copy link
Contributor

anchao commented Oct 14, 2024

since #14200 fix the dead lock.

@anchao @hujun260 @xiaoxiang781216

I tried this PR with #14200, the nsh deadlock issue that I reported in #14079 (comment) was resolved.

However, if I run nxplayer repeatedly with spresense:rndis_smp, deadlock happens. I think we would find other deadlocks if we test more complicated use cases.

As I commented in #14079 (comment), I think it's too early to remove nesting spinlock support. So again, I suggest we revert this PR.

@masayuki2009
OK, please review this PR #14218 whether works for you, thanks

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