Re: [PATCH v5 11/14] gpu: nova-core: gsp: cmdq: use consume_element to implement drain

["Alexandre Courbot" <acourbot@nvidia.com>]

On Wed Oct 21, 2026 at 12:23 PM JST, Eliot Courtney wrote:
> On Wed Oct 21, 2026 at 12:12 PM JST, John Hubbard wrote:
>> On 10/20/26 7:57 PM, Eliot Courtney wrote:
>>> On Tue Oct 20, 2026 at 11:27 PM JST, Alexandre Courbot wrote:
>>>> On Tue Oct 20, 2026 at 7:18 PM JST, sashiko-bot wrote:
>>> 
>>> [...]
>>> 
>>>>> In rust/kernel/io/poll.rs:read_poll_timeout(), when the condition is
>>>>> false, the timeout check uses a strict greater-than comparison:
>>>>>
>>>>>         if start.elapsed() > timeout_delta {
>>>>>             // Unlike the C version, we immediately return.
>>>>>             // We have just called `op()` so we don't need to call it again.
>>>>>             return Err(ETIMEDOUT);
>>>>>         }
>>>>>
>>>>>         if !sleep_delta.is_zero() {
>>>>>             fsleep(sleep_delta);
>>>>>         }
>>>>>
>>>>> Because op() only reads two u32 indices from coherent memory in RAM, if
>>>>> the monotonic clock has not advanced by at least 1 ns between
>>>>> Instant::now() and start.elapsed() (for instance on architectures with
>>>>> coarser timer resolution or under virtualization), start.elapsed() is
>>>>> Delta::ZERO. Since Delta::ZERO > Delta::ZERO is false,
>>>>> read_poll_timeout() falls through to fsleep(Delta::from_millis(1)) while
>>>>> holding the Cmdq mutex (in paths like GspInterrupt::handle_threaded() ->
>>>>> Cmdq::drain() and Gpu::new() -> Cmdq::drain()).
>>>>>
>>>>> Would it make sense to keep the while
>>>>> !self.gsp_mem.driver_read_area().0.is_empty() loop condition around
>>>>> self.consume_element(Delta::ZERO, |message| message.log(self.dev))?
>>>>
>>>> Mmm, that's an interesting one, in that I am not sure there are any
>>>> concrete implications but I'd like to fix it anyway.
>>>>
>>>> I was thinking that maybe we should add a special-case to
>>>> `read_poll_timeout` so it returns unconditionally after checking the
>>>> condition once if `timeout_delta == Delta::ZERO`, but doing so would
>>>> make the `sleep_delta` argument moot so I guess that's not adequate (and
>>>> the current behavior is technically correct). So I think I'll
>>>> special-case nova-core's `wait_for_msg` instead.
>>> 
>>> Personally I don't mind the special case in `read_poll_timeout`.
>>> If you say, I want the timeout to be zero and then it sleeps, that
>>> sounds like wonky behaviour to me. I think it feels weird to have
>>
>> Yes, it's odd behavior by the function, but on the other hand,
>> it pairs perfectly with the odd behavior of the caller. :)
>>
>> Let's avoid special casing. Just let the zero flow naturally
>> through the function and leave it alone. Special casing should
>> only be added in special cases!
>>
>>
>>> this special case w.r.t. `sleep_delta` only because we are using
>>> constant values and that's a common use case. TBH I think it would
>>> even be fine to change `start.elapsed() > timeout_delta` to
>>> `start.elapsed() >= timeout_delta`, then it's not even really a
>>> special case. I could try sending a patch for this.
>>> 
>>> I'm also fine with special casing `wait_for_msg`, but personally
>>> (maybe) sleeping when you set timeout to Delta::ZERO sounds busted to
>>> me.
>>
>> It's not necessarily wrong, depending on how you think about it:
>> "sleep, but timeout at zero" is something I've seen used (I forget
>> where) as to mean "context switch away", kind of like a cpu_relax().
>
> Here, it only sometimes sleeps ("(maybe)"), depending on the resolution
> of the timer and how long stuff takes. So it's really pretty
> inconsistent IMO.

The alternative if we special-case `read_poll_timeout` is that the
`sleep_delta` argument becomes meaningless, which from an API
perspective does not sound right to me. With the current behavior, even
if we pass `Delta::ZERO` the semantics of the function (which is "wait
for at least at least `timeout_delta` and sleep for `sleep_delta` if you
need to test a second time"), still parses. Special-casing would
complicate this, which is why I would rather lean towards doing that on
the caller side.