Skip to content

rp2: Ensure core1 doesn't hold gc lock in deinit. #8835

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

rp2: Ensure core1 doesn't hold gc lock in deinit. #8835

wants to merge 1 commit into from

Conversation

jimmo
Copy link
Member

@jimmo jimmo commented Jun 29, 2022

This addresses the second comment on #8494 (comment)

It's easy to reproduce:

import gc, _thread

def collect_thread():
    while True:
        gc.collect()

_thread.start_new_thread(collect_thread, [])

after running that, hitting Ctrl-D at the repl will make the device stop responding. With this commit, the issue goes away.

@jimmo
rp2: Ensure core1 doesn't hold gc lock in deinit.
8ead634 
Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
@dpgeorge
Copy link
Member

Looks good, thanks!

(I guess this wouldn't have been an issue if gc_init() was inside the soft reset loop.)

@jimmo
Copy link
Member Author

jimmo commented Jun 29, 2022

(I guess this wouldn't have been an issue if gc_init() was inside the soft reset loop.)

Yes, that would work too... I don't have strong feelings either way, although we might want to be able to make the heap persist across soft reset (e.g. previously discussed "permanent allocs" etc).

@jimmo jimmo mentioned this pull request Jun 29, 2022
Closed
@jimmo
Copy link
Member Author

jimmo commented Jun 29, 2022

See #8836 for a related fix, and in particular the very last note about a potentially different way to solve this.

@jimmo jimmo mentioned this pull request Jun 29, 2022
Closed
@dpgeorge
Copy link
Member

although we might want to be able to make the heap persist across soft reset

Yes. I would like to eventually move gc_init() outside the soft reset loop on all ports, so having it on rp2 is a good starting point.

@dpgeorge
Copy link
Member

Merged in 797a83a

This may need a follow up for doing the same thing with qstr_mutex. Not sure, would need to create a test to see if it is a problem.

@dpgeorge dpgeorge closed this Jun 30, 2022
tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Jan 25, 2024
@dhalbert
Merge pull request micropython#8835 from tannewt/allocate_spiram_first
b18665a 
Allocate to SPIRAM first on ESP
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
port-rp2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jimmo @dpgeorge