Skip to content

py/malloc,rp2: Add mutex for tracked allocations. #16071

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
Merged
merged 2 commits into from
May 2, 2025
Merged

py/malloc,rp2: Add mutex for tracked allocations. #16071

merged 2 commits into from
May 2, 2025

Conversation

projectgus
Copy link
Contributor

@projectgus projectgus commented Oct 24, 2024 •
edited
Loading

Note: This draft PR is currently one commit stacked on top of #16015, will need rebase after the linked PR is merged. (Done.)

Summary

As discussed in this comment, non-GIL threaded ports (i.e. rp2) need some thread safety protection around tracked allocations. This PR reuses the GC mutex for this purpose.

Testing

Trade-offs and Alternatives

  • This fix is based on an assumption that threaded ports with a GIL will always hold the GIL when calling tracked allocation functions. I believe this is true in all current ports using existing configurations, although will need to take care in the future (for example, the openamp module libmetal integration uses tracked allocations - it's currently not enabled on any threaded ports, but if it was then this might be unsafe.)
  • Initial version of this fix added a new mutex for tracked alloc linked list only, instead of reusing the GC mutex. This has all the downsides of reusing the GC mutex (potential for deadlocks), plus the downside of requiring a m_tracked_malloc_init function to initialise the mutex.

@projectgus projectgus added py-core Relates to py/ directory in source port-rp2 labels Oct 24, 2024
@projectgus projectgus mentioned this pull request Oct 24, 2024
Merged
@projectgus projectgus force-pushed the bugfix/tracked_alloc_mutex branch from 743143c to c30ac5e Compare October 24, 2024 03:15
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 24, 2024 •
edited
Loading

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:   +64 +0.007% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:   +40 +0.004% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

@codecov Codecov
Copy link

codecov bot commented Oct 24, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.54%. Comparing base (bee1fd5) to head (79abdad).
Report is 2 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master   #16071   +/-   ##
=======================================
  Coverage   98.54%   98.54%           
=======================================
  Files         169      169           
  Lines       21890    21898    +8     
=======================================
+ Hits        21572    21580    +8     
  Misses        318      318

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@projectgus projectgus force-pushed the bugfix/tracked_alloc_mutex branch from c30ac5e to c2db4be Compare February 4, 2025 00:27
@projectgus projectgus marked this pull request as ready for review February 4, 2025 00:56
@projectgus
Copy link
Contributor Author

Rebased, re-tested. The bug this fixes is no longer easy to reproduce, but I believe it still exists.

@projectgus projectgus requested a review from dpgeorge February 4, 2025 00:57
@dpgeorge dpgeorge added this to the release-1.26.0 milestone Feb 11, 2025
dpgeorge
dpgeorge reviewed May 1, 2025
View reviewed changes
Copy link
Member

@dpgeorge dpgeorge left a comment

Choose a reason for hiding this comment

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

I tested this on RPI_PICO2_W, running the new test both before and after the patch in this commit. It passes. Also tested the test on ESP32_GENERIC, and it passes there.

I agree that even if the test passes without the patch here, it's still worth having this fix merged. It should be very minimal (unmeasurable) in terms of cost to performance.

@projectgus projectgus force-pushed the bugfix/tracked_alloc_mutex branch from c2db4be to cdd94ba Compare May 1, 2025 06:26
@projectgus
Copy link
Contributor Author

Rebased, addressed comments, re-ran the new unit test on RPI_PICO_W (still passes).

@projectgus projectgus force-pushed the bugfix/tracked_alloc_mutex branch from cdd94ba to 01d6315 Compare May 2, 2025 02:33
projectgus added 2 commits May 2, 2025 17:24
@projectgus @dpgeorge
py/malloc: Add mutex for tracked allocations.
70ed315 
Fixes thread safety issue that could cause memory corruption on ports
with (MICROPY_PY_THREAD && !MICROPY_PY_THREAD_GIL) - currently only rp2 and
unix have this configuration.

Adds unit test for TLS sockets that exercises this code path.  I wasn't
able to make this fail on rp2, the race condition window is pretty narrow
and may not have a direct impact on a quiet system.

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
@projectgus @dpgeorge
tests/extmod: Rename ssl tests that only use the tls module.
79abdad 
Signed-off-by: Angus Gratton <angus@redyak.com.au>
@dpgeorge dpgeorge force-pushed the bugfix/tracked_alloc_mutex branch from 01d6315 to 79abdad Compare May 2, 2025 07:26
@dpgeorge dpgeorge merged commit 79abdad into micropython:master May 2, 2025
59 of 63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpgeorge dpgeorge dpgeorge approved these changes

Assignees
No one assigned
Labels
port-rp2 py-core Relates to py/ directory in source
Projects
None yet
Milestone
release-1.26.0
Development

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