On Tue Sep 1, 2026 at 3:47 AM BST, Eliot Courtney wrote: > On Tue Sep 1, 2026 at 2:25 AM JST, Gary Guo wrote: >> On Tue Aug 25, 2026 at 1:38 AM BST, Eliot Courtney wrote: >>> On Mon Aug 24, 2026 at 10:07 PM JST, Gary Guo wrote: >>>> On Mon Aug 24, 2026 at 2:03 PM BST, Eliot Courtney wrote: >>>>> On Mon Aug 24, 2026 at 9:56 PM JST, Gary Guo wrote: >>>>>>>> @@ -683,6 +689,9 @@ fn send_single_command<M>(&mut self, bar: >>>>>>>> Bar0<'_>, command: M) -> Result >>>>>>>> dst.header.length(), >>>>>>>> ); >>>>>>>> >>>>>>>> + // ORDERING: STORE->STORE ordering needed to order >>>>>>>> `cpu_write_ptr` write after data write. >>>>>>>> + dma_mb(Write); >>>>>>>> + >>>>>>> >>>>>>> Is there a reason this can't go into `advance_cpu_write_ptr`? >>>>>> >>>>>> I think it's more clear to consider `advance_cpu_write_ptr` to just be >>>>>> the >>>>>> pointer increment, and the ordering should be visible in code that >>>>>> performs both >>>>>> memory ops. >>>>> >>>>> In the second patch, it looks like you're adding the memory barrier >>>>> directly in `advance_cpu_read_ptr`. So we'd have one barrier directly in >>>>> the code advancing the pointer and one not, which seems asymmetric. I >>>>> think it's less error prone to put the barrier in the function so it >>>>> can't be misused (and we already have evidence the barriers are easy to >>>>> get wrong, since this code was already broken). >>>> >>>> In the second one `message.header.length()` is read, so if I move the >>>> barrier to >>>> before the advance it'll be incorrect. >>>> >>>> Best, >>>> Gary >>> >>> Yerp I mean move the barrier into `advance_cpu_write_ptr` not move the >>> barrier out of `advance_cpu_read_ptr` - I agree that'd be incorrect. On >>> clearness, it feels very odd to me to have these two functions >>> (advance_cpu_read_ptr, advance_cpu_write_ptr) where one controls the >>> memory barrier and one doesn't, purely based off the structure of the >>> callers. And I still think it's less error prone (for future changes) to >>> do it this way too. >> >> Frankly I don't like the asymmetry that the advancing code does the barrier, >> while the pointer reading code doesn't have the barrier. However, if we move >> the >> barrier to the pointer read function, then the `driver_write_area_size` would >> gain a unnecessary barrier. (Actually, `driver_read_area` code have a similar >> issue, a failed pool would execute an unnecessary barrier). >> >> As an alternative to move the barrier into the advancing code, alternatively >> we >> can pull the `message.header.length()` to a separate line instead. >> >> I think we should either always have barrier inside the pointer read/update >> code, or always on the user side. Given the former would mean unnecessary >> barriers, I am erring on the latter. >> >> Best, >> Gary > > I see - so you're saying that one side of the maximally consistent > position is to put the memory barriers in additionally `gsp_read_ptr` > and `gsp_write_ptr`, but those don't always need a barrier e.g. > driver_write_area_size because they are not necessarily followed by an > access that needs ordering, and the alternative is to have callers of > those functions handle that responsibility. > > I think the differrence is that `advance_cpu_read_ptr` and > `advance_cpu_write_ptr` definitely need barriers, so why push up that > one level? Having barriers in `advance_cpu_read_ptr`, > `advance_cpu_write_ptr`, `driver_read_area`, and `driver_write_area` is > sufficiently consistent since it's the deepest set of functions that > can't avoid memory barriers.
To me conceptually it's best to place memory barriers in places in between two operations so it's very clear that it provides ordering between two operations. Hiding memory barrier inside a plain access can be confusing. Alternatively, we can specifiy that the pointer updater have release semantics and the pointer reader have acquire semantics. This way it's also very clear, but does introduce unneeded barrier for the polling case as I mentioned. That said, given that the polling interval is once a millisecond, extra barrier is okay (full barrier is ~100ns). I do wish we have acquire/release barriers for these cases, which would be quite much cheaper! So unless there's an objection, I'll take the second approach by moving barrrier to pointer accessors/updaters and mark these methods to have acq/rel semantics. Best, Gary > > If we want to remove unnecessary memory barriers on polling > `driver_read_area`, we could add an analogous `driver_read_area_size` or > move the memory barrier for driver_read_area up one level. The poll is > only once every millisecond though, so I doubt it makes a difference for > performance. But if we were to change it imo `driver_read_area_size` is > most consistent.
