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. 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.
