04:07 <cr1901> Can someone explain to me why the Acquire fence isn't sufficient here (and this line is required)? https://github.com/stm32-rs/stm32f1xx-hal/blob/59777ecad18ff7ed3fca82ed5ee4860c94a0031b/src/dma.rs#L390 I don't think I understand the comment

05:34 <re_irc> <@burrbull:matrix.org> I can't answer the question. I only know that this was insperred by https://docs.rust-embedded.org/embedonomicon/dma.html

12:29 <re_irc> <@mciantyre:matrix.org> cr1901: Just guessing: "compiler_fence(Acquire)" prevents instruction movement up to the preceding read. If a read isn't happening within (or at the end of) "stop()", then the compiler could move the subsequent buffer and payload reads (https://github.com/stm32-rs/stm32f1xx-hal/blob/59777ecad18ff7ed3fca82ed5ee4860c94a0031b/src/dma.rs#L403-L404) before / interleaved with the "stop()" instructions.

15:19 <cr1901> AIUI, fence(Acquire) should prevent insns after the fence from being place before the fence. Is that incorrect?

15:20 <cr1901> So the ptr::read_volatile(&0) seems redundant

15:33 <re_irc> <@jamesmunns:beeper.com> the volatile read of a temporary definitely seems super out of place to me, and would be very sad if that was necessary for things to work correctly.

15:33 <re_irc> <@ryan-summers:matrix.org> If that were the case, it would indicate to me that the fence is incorrectly selected

15:34 <re_irc> <@jamesmunns:beeper.com> Yeah, the one after might want to be "Release"? Like you typically acquire before to make sure you "see" all changes, and you release after to make sure others "see" all changes.

15:34 <re_irc> <@jamesmunns:beeper.com> I still haven't finished reading Mara's book tho 😅

15:35 <cr1901> Well I feel a lot better just that other ppl think it's odd

15:35 <cr1901> (for want of a better term)

15:35 <re_irc> <@ryan-summers:matrix.org> To be frank, most code feels odd to me nowadays, and I immediately become suspicious when someone points me at any of it

15:38 <re_irc> <@dirbaio:matrix.org> yeah that code is not right

15:38 <cr1901> most code feels odd to me nowadays <-- most code was a mistake :'D

15:38 <re_irc> <@dirbaio:matrix.org> you need an AcqRel fence between writing the buffer and starting DMA

15:38 <re_irc> and another AcqRel fence between stopping the DMA and reading the buffer

15:39 <cr1901> Idk enough about DMA to fix it

15:39 <cr1901> About safe DMA in Rust*

15:39 <re_irc> <@jamesmunns:beeper.com> : this matches my understanding as well

15:39 <re_irc> <@dirbaio:matrix.org> that code is missing the 1st

15:40 <re_irc> <@dirbaio:matrix.org> and the 2nd does an Acquire + dummy read instead... not sure what the point is because other memory accesses can be reordered with the dummy read

15:41 <cr1901> the 2nd should be a Release fence?

15:41 <re_irc> <@jamesmunns:beeper.com> honestly, I'd say here: https://github.com/stm32-rs/stm32f1xx-hal/blob/59777ecad18ff7ed3fca82ed5ee4860c94a0031b/src/dma.rs#L164-L173

15:41 <re_irc> <@jamesmunns:beeper.com> put an acqrel fence before the pac interaction in start, and put an acqrel fence after the interaction in stop

15:42 <re_irc> <@dirbaio:matrix.org> also when you stop DMA you have to spin waiting for EN=0, I can't find that there :D

15:42 <re_irc> <@jamesmunns:beeper.com> Yeah, I wonder if their volatile and fence (which issues a dmb!) is just long enough for en=0 to take

15:42 <re_irc> <@jamesmunns:beeper.com> oh nvm

15:43 <re_irc> <@jamesmunns:beeper.com> it's compiler fence not fence, no instruction emitted.

15:43 <re_irc> <@dirbaio:matrix.org> dunno, maybe not because it's only if you forcefully stop it, not needed if it stops naturally

15:43 <cr1901> >not sure what the point is because other memory accesses can be reordered with the dummy read <-- yea, but the Acquire fence is right after the dummy read. So only memory access before the Acquire can be reordered with--

15:44 <cr1901> yea the dummy read stands out as not supposed to be there, but Idk what the fix should look like

15:44 <re_irc> <@jamesmunns:beeper.com> > put an acqrel fence before the pac interaction in start, and put an acqrel fence after the interaction in stop

15:44 <re_irc> IMO.

15:44 <re_irc> <@dirbaio:matrix.org> idk

15:44 <re_irc> <@dirbaio:matrix.org> just use embassy-stm23, it has no DMA bugs :)

15:44 <re_irc> <@dirbaio:matrix.org> * embassy-stm32,

15:45 <re_irc> <@dirbaio:matrix.org> 🙈

15:46 <re_irc> <@mciantyre:matrix.org> cr1901: I'm going off the docs here (https://doc.rust-lang.org/core/sync/atomic/fn.compiler_fence.html).

15:46 <re_irc> > with Acquire, subsequent reads and writes cannot be moved ahead of preceding reads.

15:46 <re_irc> The key is "preceding reads" at the end of that statement. The fence is based on a read instruction, not just any instruction.

15:46 <re_irc> If there's no preceding read, the fence doesn't have a post. That's why I'm guessing there's a dummy "ptr::read_volatile".

15:47 <re_irc> <@jamesmunns:beeper.com> Honestly in my understanding the only thing you need to ensure is not reordered is the read on https://github.com/stm32-rs/stm32f1xx-hal/blob/59777ecad18ff7ed3fca82ed5ee4860c94a0031b/src/dma.rs#L364-L369 relative to "stop" on line 348.

15:49 <cr1901> mciantyre: Yea, it makes sense under that definition

15:49 <re_irc> <@jamesmunns:beeper.com> I don't think the fence on 346 actually _does_ anything. volatile ops are not reordered relative to each other, and 344 and 348 are basically both volatile ops.

15:50 <cr1901> I thought the definition of Acquire was " subsequent reads and writes cannot be moved ahead preceding memory accessed period"

15:51 <re_irc> <@jamesmunns:beeper.com> https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html#variant.Acquire

15:52 <re_irc> <@jamesmunns:beeper.com> acquire is only "writes with Release or stronger ordering cannot be reordered across an acquire fence"

15:52 <re_irc> <@jamesmunns:beeper.com> and volatile ops have no synchronization interaction with atomic operations, afaik.

15:54 <re_irc> <@jamesmunns:beeper.com> the embedonomicon example shows this: https://docs.rust-embedded.org/embedonomicon/dma.html#compiler-misoptimizations

15:54 <re_irc> <@jamesmunns:beeper.com> "release before start, acquire after stop"

15:57 <re_irc> <@jamesmunns:beeper.com> https://doc.rust-lang.org/std/sync/atomic/fn.compiler_fence.html has some good docs too

15:57 <cr1901> https://doc.rust-lang.org/nomicon/atomics.html#acquire-release

15:57 <cr1901> >Intuitively, an acquire access ensures that every access after it stays after it.

15:58 <re_irc> <@jamesmunns:beeper.com> Yeah, we want the reads of buffer and payload to stay AFTER stop.

15:58 <cr1901> Sure, but my interpretation is that subsequent reads/writes won't be moved ahead of preceding reads AND writes

15:59 <cr1901> and docs say that "subsequent reads/writes won't be moved ahead of preceding reads"

15:59 <re_irc> <@jamesmunns:beeper.com> compiler fence docs at least say "with Acquire, subsequent reads and writes cannot be moved ahead of preceding reads."

15:59 <cr1901> But it says nothing about writes

16:00 <cr1901> >and volatile ops have no synchronization interaction with atomic operations, afaik. <-- I would think the fence would prevent things from being reordered before the volatile op

16:02 <cr1901> The volatile in the code I linked appears to be there as a "do not optimize away" measure

16:02 <re_irc> <@jamesmunns:beeper.com> Gotcha, I understand the point you are making better now

16:03 <re_irc> <@jamesmunns:beeper.com> honestly it feels like just using seqcst would be better here: "with SeqCst, no re-ordering of reads and writes across this point is allowed."

16:03 <re_irc> <@jamesmunns:beeper.com> (which is different than AcqRel)

16:04 <re_irc> <@jamesmunns:beeper.com> but I dunno. I should finish reading Mara's book before pretending to know what I am talking about, despite trying to understand this all for 5 years :p

16:13 <cr1901> Okay I think I get it: "However operations that occur before an acquire are free to be reordered to occur after it." <-- Actually, rereading this, this doesn't really contradict the fence docs.

16:14 <cr1901> This would be the same thing as saying "preceding writes can be moved after the fence" (as long as reads before the fence don't depend on these writes)

16:14 <cr1901> Anyway, atomics hard, brain bandwidth used for the day

16:29 <re_irc> <@dngrs:matrix.org> I think it would be good to have concrete examples for each scenario

16:29 <re_irc> <@dngrs:matrix.org> but I guess Mara's book covers that

16:30 <re_irc> <@jamesmunns:beeper.com> Yeah, her book is also available online, so they could be referenced directly in the embedded book

16:30 <re_irc> <@jamesmunns:beeper.com> https://marabos.nl/atomics/

16:30 <re_irc> <@jamesmunns:beeper.com> https://marabos.nl/atomics/memory-ordering.html is probably the one :D

18:31 <re_irc> <@peter9477:matrix.org> : If that's correct, it's just about the first and only description of any of this stuff that I can comprehend and make use of.

18:31 <re_irc> <@jamesmunns:beeper.com> Read Mara's book, it's 100% guaranteed to be more accurate than anything I said.

18:38 <re_irc> <@peter9477:matrix.org> Yep, nice looking resource. Hadn't seen a reference to it before.

18:39 <re_irc> <@jamesmunns:beeper.com> (sorry, that came off grumpy, atomics are one of those "every time I think I have it I am proven wrong", and I very much trust Mara's knowledge over mine :D)

18:39 <re_irc> <@peter9477:matrix.org> Very timely too, as I just threw a random mixed of Orderings into my Pdm changes as placeholders, knowing half of them are probably very wrong...