Re: [PATCH 3/5] rust: sync: add WaitQueue infrastructure

[Alice Ryhl <aliceryhl@google.com>]

On Mon, Jul 27, 2026 at 12:36:09AM +0200, Danilo Krummrich wrote:
> +    /// Returns a raw pointer to the underlying `wait_queue_head`.
> +    #[expect(unused)]
> +    #[inline]
> +    pub(super) fn as_raw(&self) -> *mut bindings::wait_queue_head {

This should 'pub'. Then you can drop #[expect(unused)].

> +    /// Sleeps until the condition returns `true` or a signal is received.
> +    ///
> +    /// Returns `Ok(())` when the condition is met, or `Err(WaitError::Signal)` if interrupted
> +    /// by a signal.
> +    #[inline]
> +    pub fn wait_event_interruptible<F: Fn() -> bool>(&self, condition: F) -> Result<(), WaitError> {
> +        self.wait_event_timeout_internal(TASK_INTERRUPTIBLE, &condition, Jiffies::MAX);
> +        if !condition() && current!().signal_pending() {
> +            Err(WaitError::Signal)
> +        } else {
> +            Ok(())
> +        }

I don't think we should call condition() again here. Instead, I think we
should base it on which 'break' statement was used in wait_event_timeout_internal().

> +    /// Sleeps until the condition returns `true` or the timeout expires.
> +    ///
> +    /// Returns `Ok(())` when the condition is met, or `Err(WaitError::Timeout)` if the timeout
> +    /// elapsed first.
> +    #[inline]
> +    pub fn wait_event_timeout<F: Fn() -> bool>(
> +        &self,
> +        condition: F,
> +        jiffies: Jiffies,
> +    ) -> Result<(), WaitError> {
> +        let remaining = self.wait_event_timeout_internal(TASK_UNINTERRUPTIBLE, &condition, jiffies);
> +        if remaining == 0 && !condition() {
> +            Err(WaitError::Timeout)
> +        } else {
> +            Ok(())
> +        }

Ditto here.

> +    fn wait_event_timeout_internal(
> +        &self,
> +        wait_state: c_int,
> +        condition: &dyn Fn() -> bool,
> +        jiffies: Jiffies,
> +    ) -> Jiffies {

Why are we using dynamic dispatch here?

> +    /// Performs a single exclusive prepare-to-wait / finish-wait cycle, calling `schedule_fn`
> +    /// in between.
> +    #[expect(unused)]
> +    pub(super) fn wait_once_exclusive<F, R>(&self, wait_state: c_int, schedule_fn: F) -> R
> +    where
> +        F: FnOnce() -> R,
> +    {
> +        let wait = Opaque::<bindings::wait_queue_entry>::uninit();
> +
> +        // SAFETY: `wait` points to valid memory.
> +        unsafe { bindings::init_wait(wait.get()) };
> +
> +        // SAFETY: Both `wait` and `wait_queue_head` point to valid memory.
> +        unsafe {
> +            bindings::prepare_to_wait_exclusive(self.wait_queue_head.get(), wait.get(), wait_state)

What about the non-exclusive prepare_to_wait? That's the one Rust Binder
*should* be using here. It's not ideal that it's using the exclusive
wait.

> +/// Error returned by [`WaitQueue`] wait functions.
> +#[derive(Debug, PartialEq)]
> +pub enum WaitError {
> +    /// Interrupted by a signal.
> +    Signal,
> +    /// The timeout elapsed without the condition being met.
> +    Timeout,
> +}
> +
> +impl From<WaitError> for Error {
> +    #[inline]
> +    fn from(e: WaitError) -> Error {
> +        match e {
> +            WaitError::Signal => ERESTARTSYS,
> +            WaitError::Timeout => ETIMEDOUT,
> +        }
> +    }
> +}

I agree with Gary's comment about splitting up this error type.

Alice