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

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

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.