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().
Anyway, I am voting heavily away from special casing here.
thanks,
--
John Hubbard
