Atomic operations in PSRAM (esp. with 3rd party libraries)

[ProfFan]

AFAIK the current atomic operations on the S3 uses native atomics. However this creates a problem: a 3rd party library could be using atomics on their data structure placed on the SPI PSRAM.

In the IDF, they handled it by first checking for PSRAM address, then try acquire the external RAM lock, then do CAS: https://github.com/espressif/esp-idf/blob/c9df77efbf871d4c3ae9fb828778ff8c4ab36804/components/esp_hw_support/cpu.c#L199

I wonder if a similar thing should be done for the S3 (for example, a PR to portable-atomic that adds a feature for a similar implementation?

Based on discussions with @ivmarkov

[bugadani]

I don't see a simple way out of this without introducing our own, "PSRAM-safe" atomics. Which will be a usability and interoperability nightmare, because library developers will not likely use it, or even know about it.

Turning portable-atomic into a shim of sorts for us is at least technically possible, but still doesn't solve every issue - for MCUs that can use the core atomic types, those ones can't really be shimmed for us to provide our own implementation.

I don't think we can extend portable-atomic to know about our particular memory mappings.

[MabezDev]

Do we know if an exception is raised whilst accessing atomics in psram, or does it just silently fail? If we can detect it, that at the very least opens up some options for us.

[ivmarkov]

Do we know if an exception is raised whilst accessing atomics in psram, or does it just silently fail? If we can detect it, that at the very least opens up some options for us.

I doubt any exception is raised. From the APP/PRO CPU POV, accessing PSRAM is like accessing regular internal RAM. And it ends up accessing regular internal RAM after all - thanks to the caching MMU.

With that said, I'm not in the position to discuss a possible fix, as I do not understand the operation of the MMUs to a deeper-enough detail. All I know is that ESP-IDF provides a special xchg atomic implemented in software, which is to be used when PSRAM is enabled, as pointed to by @ProfFan .

With that said, whoever wrote and maintains this is probably in that position. (@bjoernQ - is that you?).

I would be happy to know though why hardware atomics cannot work with PSRAM:

• Is it because the two CPUs might have their own separate dram caches for the same psram; why would we even want such a setup?
• Or is it because an atomic might span across two cache-lines (the end of one, and the beginning of the other) and as such the writing might actually not be atomic at all?

Anyway, I guess me and @ProfFan are just raising the flag that there is an issue with atomics and PSRAM, and somebody needs to look into it. For both ESP-IDF and baremetal by the way, as the ESP-IDF multicore targets are also happily using only hardware atomics ATM; and once we know of a fix, we should probably change the definition of the upstream multicore STD targets from just using hardware atomics to emitting calls to the atomic intrinsic functions, so that the user code can use whatever ESP-IDF had put in the definition of these intrinsic functions, depending on user settings).

Ah, one more question, couldn't resist:

• Would hardware atomics work correctly, if the programmer is ensuring that the relevant PSRAM memory region is accessed only by one of the two CPUs? As in, do we even have a workaround for the current situation...

[bjoernQ]

With that said, whoever wrote and maintains this is probably in that position. (@bjoernQ - is that you?).

Yes, I ported the code and no I don't know. I also doubt an exception is raised there but we can just give it a try

I'm also curious about the "why doesn't it just work"

[MabezDev]

I'll try and find those answers and get back to you.

In the meantime, the only truly bullet-proof way to solve this right now is to have a target variant where we disable hardware atomics, falling back to portable-atomic or something else to ensure atomicity (note: this is exactly what esp-idf does, when the work around is enabled). I'd really like to avoid doing this though if we can help it.

[ivmarkov]

I'll try and find those answers and get back to you.

In the meantime, the only truly bullet-proof way to solve this right now is to have a target variant where we disable hardware atomics, falling back to portable-atomic or something else to ensure atomicity (note: this is exactly what esp-idf does, when the work around is enabled). I'd really like to avoid doing this though if we can help it.

The question is, is portable-atomic even having a feature that would enable atomics (at least on bare-metal, for ESP-IDF we have other options) to operate correctly.

The critical-section one is tempting, but do you remember why our support for u64 on multicore riscv got kicked out from upstream STD? Because it essentially used disable-all-interrupts-critical-sections-plus-spinlocks and they said using spinloocks for atomic operations defeats their very purpose in the first place.

Also if you notice, the software PSRAM-safe xchg impl in the ESP-IDF does something different... yet similar. They seem to implement the "fallible" xchg variant only, which can sporadically fail. But then the question is, what would the user do when presented with such an xchg impl, other than re-trying in a loop, which suspiciously looks like a cousin to spinlocks anyway...

[ProfFan]

• Would hardware atomics work correctly, if the programmer is ensuring that the relevant PSRAM memory region is accessed only by one of the two CPUs?

I think the answer currently for the S3 is most likely true but I am not sure. It's probably good to open an issue in esp-idf and tag @igrr for help 🍭

for MCUs that can use the core atomic types, those ones can't really be shimmed for us to provide our own implementation.

If we have a custom target (for example xtensa-none-elf-esp32s3) this can be put as a part in core right? As we build core with the application anyway.

The critical-section one is tempting, but do you remember why our support for u64 on multicore riscv got kicked out from upstream STD? Because it essentially used disable-all-interrupts-critical-sections-plus-spinlocks and they said using spinloocks for atomic operations defeats their very purpose in the first place.

I wonder what they did for Cortex-M0/M0+ and such? See https://doc.rust-lang.org/std/sync/atomic/index.html#portability
thumbv6m does not support CAS at all.

[ivmarkov]

I wonder what they did for Cortex-M0/M0+ and such? See https://doc.rust-lang.org/std/sync/atomic/index.html#portability thumbv6m does not support CAS at all.

If Cortex-M0/M0+ is always single-core (is it?) then disable-all-interrupts-one-way-or-another as a strategy does work fine and since it is a single core, you don't have the "is a spinlock a lock or just waiting" issue.

By the way... by reading the opening paragraph in the "Portability section"... namely:
"
All atomic types in this module are guaranteed to be lock-free if they’re available. This means they don’t internally acquire a global mutex. Atomic types and operations are not guaranteed to be wait-free. This means that operations like fetch_or may be implemented with a compare-and-swap loop.
"

I'm now even more puzzled as to why they kicked out our u64 multicore support...
I mean, if we talk about "spinlocks" (that we used in that implementation...)

• Are these "locks" in the terminology of that section? <-- I DOUBT this now
• Or are these "waits" in the terminology of that section <-- I now believe this is the case, and if so, our spinlocking impl should've not been kicked out in the first place...

[bugadani]

If Cortex-M0/M0+ is always single-core (is it?)

No, see RP2040

[ivmarkov]

If Cortex-M0/M0+ is always single-core (is it?)

No, see RP2040

Ah so you mean for rp2040 they use a spinlock+disable-all-interrupts software impl? If yes, then this only reinforces my suspicion, that our u64 atomic impl Shall Be Re-Instated...

I guess at the time (and now still) I was not understanding the subject matter well enough to object their decision on technical ground...

[ivmarkov]

...and if spinlocking+disable-all-interrupts is fine as a strategy, then the critical-section impl of portable-atomic is good enough of a workaround for multicore.

[ProfFan]

No, see RP2040

Beaten me in 5 secs lol

Ah so you mean for rp2040 they use a spinlock+disable-all-interrupts software impl? If yes, then this only reinforces my suspicion, that our u64 atomic impl Shall Be Re-Instated...

Probably no, see rust-lang/rust#99668

...and if spinlocking+disable-all-interrupts is fine as a strategy, then the critical-section impl of portable-atomic is good enough of a workaround for multicore.

For real-time applications like what we use ESPs for I think this is worse than the IDF way of "best effort" weak CAS, but can live without if it's too complicated to make something similar in Rust.

[ivmarkov]

No, see RP2040

Beaten me in 5 secs lol

Ah so you mean for rp2040 they use a spinlock+disable-all-interrupts software impl? If yes, then this only reinforces my suspicion, that our u64 atomic impl Shall Be Re-Instated...

Probably no, see rust-lang/rust#99668

TL;DR :(
If you can summarize for me, what does anybody use for CAS in multicore M0?
Because I don't think - whatever is used - can avoid using a loop and a loop is as close to a spinlock as you can get? As both are essentially just wait-and-check OR wait-and-retry which is essentially the same?

...and if spinlocking+disable-all-interrupts is fine as a strategy, then the critical-section impl of portable-atomic is good enough of a workaround for multicore.

For real-time applications like what we use ESPs for I think this is worse than the IDF way of "best effort" weak CAS, but can live without if it's too complicated to make something similar in Rust.

Because the weak cas esp idf impl does not disable interrupts (which is... slow?) or if not why then?

Because a strong cas impl on top of a weak cas would also require a loop anyway...