linux-kernel - Re: [PATCH v2 02/14] rust: hrtimer: introduce hrtimer support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <33c65715-3af5-44f9-93c5-149d07a4c503@proton.me>
Date: Thu, 19 Sep 2024 14:09:58 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Andreas Hindborg <a.hindborg@...nel.org>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Anna-Maria Behnsen <anna-maria@...utronix.de>, Frederic Weisbecker <frederic@...nel.org>, Thomas Gleixner <tglx@...utronix.de>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Alice Ryhl <aliceryhl@...gle.com>, rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 02/14] rust: hrtimer: introduce hrtimer support

On 19.09.24 07:43, Andreas Hindborg wrote:
> Hi Benno,
> 
> Thanks for the feedback. I will incorporate all the whitespace
> suggestions you have =F0=9F=91=8D

There seems to be something wrong with the encoding in this email, is
that an issue on my side or yours?

> "Benno Lossin" <benno.lossin@...ton.me> writes:
> 
>> On 18.09.24 00:27, Andreas Hindborg wrote:
> [...]
>>> +
>>> +impl<T> Timer<T> {
>>> +    /// Return an initializer for a new timer instance.
>>> +    pub fn new() -> impl PinInit<Self>
>>> +    where
>>> +        T: TimerCallback,
>>> +    {
>>> +        pin_init!( Self {
>>
>> I would remove the space after the `(`.
>> Would be great if we had rustfmt support for custom macros.
> 
> Yes, that would be great!
> 
>>
>>> +            // INVARIANTS: We initialize `timer` with `hrtimer_init` be=
> low.
>>> +            timer <- Opaque::ffi_init(move |place: *mut bindings::hrtim=
> er| {
>>> +                // SAFETY: By design of `pin_init!`, `place` is a point=
> er live
>>> +                // allocation. hrtimer_init will initialize `place` and=
>  does not
>>> +                // require `place` to be initialized prior to the call.
>>> +                unsafe {
>>> +                    bindings::hrtimer_init(
>>> +                        place,
>>> +                        bindings::CLOCK_MONOTONIC as i32,
>>> +                        bindings::hrtimer_mode_HRTIMER_MODE_REL,
>>> +                    );
>>> +                }
>>> +
>>> +                // SAFETY: `place` is pointing to a live allocation, so=
>  the deref
>>> +                // is safe.
>>> +                let function: *mut Option<_> =3D
>>
>> Do you really need this type hint?
> 
> Apparently not!
> 
> [...]
>>> +pub trait TimerPointer: Sync + Sized {
>>> +    /// A handle representing a scheduled timer.
>>> +    ///
>>> +    /// If the timer is armed or if the timer callback is running when =
> the
>>> +    /// handle is dropped, the drop method of `TimerHandle` should not =
> return
>>> +    /// until the timer is unarmed and the callback has completed.
>>> +    ///
>>> +    /// Note: It must be safe to leak the handle.
>>> +    type TimerHandle: TimerHandle;
>>
>> Why does this need to be an associated type? Couldn't we have a
>> `TimerHandle<T>` struct? The schedule function below could then return
>> `TimerHandle<Self>`.
> 
> At one point, I had some cycles in trait resolution. Moving generics to
> associated types solved that issue. Maybe this can be changed to a
> generic. But are generics preferred over associated types for some
> reason?

The associated type is more complicated IMO, because then every
implementer of the trait needs to create one. If we can avoid that, I
would prefer a generic type.

---
Cheers,
Benno