Re: [PATCH v3 1/2] gpu: nova-core: fix barrier usage in CPU->GSP messaging path

["Eliot Courtney" <ecourtney@nvidia.com>]

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.