Re: [PATCH v3 3/4] rust: sync: add `CondVar::wait_timeout`

[Alice Ryhl]

Boqun Feng <boqun.feng@xxxxxxxxx> writes:
> On Thu, Jan 04, 2024 at 02:02:43PM +0000, Alice Ryhl wrote:
>> Sleep on a condition variable with a timeout.
>> 
>> This is used by Rust Binder for process freezing. There, we want to
>> sleep until the freeze operation completes, but we want to be able to
>> abort the process freezing if it doesn't complete within some timeout.
>> 
>> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@xxxxxxxxx>
>> Reviewed-by: Tiago Lam <tiagolam@xxxxxxxxx>
>> Signed-off-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>
> 
> Reviewed-by: Boqun Feng <boqun.feng@xxxxxxxxx>
> 
> That said, I want to hear from Thomas on the usage of jiffies, see
> below:
> 
>> -    fn wait_internal<T: ?Sized, B: Backend>(&self, wait_state: u32, guard: &mut Guard<'_, T, B>) {
>> +    fn wait_internal<T: ?Sized, B: Backend>(
>> +        &self,
>> +        wait_state: u32,
>> +        guard: &mut Guard<'_, T, B>,
>> +        timeout_in_jiffies: c_long,
> 
> This is an internal function, and it makes sense we use the same type
> for durations as the C function we rely on.
>
>> +    /// Releases the lock and waits for a notification in interruptible mode.
>> +    ///
>> +    /// Atomically releases the given lock (whose ownership is proven by the guard) and puts the
>> +    /// thread to sleep. It wakes up when notified by [`CondVar::notify_one`] or
>> +    /// [`CondVar::notify_all`], or when a timeout occurs, or when the thread receives a signal.
>> +    #[must_use = "wait_interruptible_timeout returns if a signal is pending, so the caller must check the return value"]
>> +    pub fn wait_interruptible_timeout<T: ?Sized, B: Backend>(
>> +        &self,
>> +        guard: &mut Guard<'_, T, B>,
>> +        jiffies: Jiffies,
> 
> This is a public interface, so it may make sense use a HZ-independent
> type for durations, e.g. core::time::Duration:
> 
> 	https://doc.rust-lang.org/core/time/struct.Duration.html	
> 
> but we don't have enough users to see whether there would be a need for
> HZ-dependent durations, so I think it's fine that we stick with a simple
> solution in Alice's patchset to get the ball rolling, we can always
> remove a public interface with HZ-dependent durations whenever we want.
> 
> Thoughts?

I tried to justify why I did not do that in the commit message for patch
2. Let me include it here:

	Note that we need to convert to jiffies in Binder. It is not enough to
	introduce a variant of `CondVar::wait_timeout` that takes the timeout in
	msecs because we need to be able to restart the sleep with the remaining
	sleep duration if it is interrupted, and if the API takes msecs rather
	than jiffies, then that would require a conversion roundtrip jiffies->
	msecs->jiffies that is best avoided.

Alice