linux-kernel - Re: [PATCH v8 12/14] rust: hrtimer: add `HrTimerMode`

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: <87zfifedc4.fsf@kernel.org>
Date: Fri, 21 Feb 2025 12:39:39 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Benno Lossin" <benno.lossin@...ton.me>
Cc: "Miguel Ojeda" <ojeda@...nel.org>,  "Anna-Maria Behnsen"
 <anna-maria@...utronix.de>,  "Frederic Weisbecker" <frederic@...nel.org>,
  "Thomas Gleixner" <tglx@...utronix.de>,  "Danilo Krummrich"
 <dakr@...nel.org>,  "Alex Gaynor" <alex.gaynor@...il.com>,  "Boqun Feng"
 <boqun.feng@...il.com>,  "Gary Guo" <gary@...yguo.net>,  Björn Roy Baron
 <bjorn3_gh@...tonmail.com>,  "Alice Ryhl" <aliceryhl@...gle.com>,  "Trevor
 Gross" <tmgross@...ch.edu>,  "Lyude Paul" <lyude@...hat.com>,  "Guangbo
 Cui" <2407018371@...com>,  "Dirk Behme" <dirk.behme@...il.com>,  "Daniel
 Almeida" <daniel.almeida@...labora.com>,  "Tamir Duberstein"
 <tamird@...il.com>,  <rust-for-linux@...r.kernel.org>,
  <linux-kernel@...r.kernel.org>,  "Timothy G."
 <c6qchwbke@...ay.firefox.com>
Subject: Re: [PATCH v8 12/14] rust: hrtimer: add `HrTimerMode`

"Benno Lossin" <benno.lossin@...ton.me> writes:

> On 21.02.25 10:17, Andreas Hindborg wrote:
>> "Benno Lossin" <benno.lossin@...ton.me> writes:
>>
>>> On 18.02.25 14:27, Andreas Hindborg wrote:
>>>> +/// Operational mode of [`HrTimer`].
>>>> +#[derive(Clone, Copy)]
>>>> +pub enum HrTimerMode {
>>>> +    /// Timer expires at the given expiration time.
>>>> +    Absolute,
>>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>>> +    Relative,
>>>> +    /// Timer does not move between CPU cores.
>>>> +    Pinned,
>>>> +    /// Timer handler is executed in soft irq context.
>>>> +    Soft,
>>>> +    /// Timer handler is executed in hard irq context.
>>>> +    Hard,
>>>> +    /// Timer expires at the given expiration time.
>>>> +    /// Timer does not move between CPU cores.
>>>> +    AbsolutePinned,
>>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>>> +    /// Timer does not move between CPU cores.
>>>> +    RelativePinned,
>>>> +    /// Timer expires at the given expiration time.
>>>> +    /// Timer handler is executed in soft irq context.
>>>> +    AbsoluteSoft,
>>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>>> +    /// Timer handler is executed in soft irq context.
>>>> +    RelativeSoft,
>>>> +    /// Timer expires at the given expiration time.
>>>> +    /// Timer does not move between CPU cores.
>>>> +    /// Timer handler is executed in soft irq context.
>>>> +    AbsolutePinnedSoft,
>>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>>> +    /// Timer does not move between CPU cores.
>>>> +    /// Timer handler is executed in soft irq context.
>>>> +    RelativePinnedSoft,
>>>> +    /// Timer expires at the given expiration time.
>>>> +    /// Timer handler is executed in hard irq context.
>>>> +    AbsoluteHard,
>>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>>> +    /// Timer handler is executed in hard irq context.
>>>> +    RelativeHard,
>>>> +    /// Timer expires at the given expiration time.
>>>> +    /// Timer does not move between CPU cores.
>>>> +    /// Timer handler is executed in hard irq context.
>>>> +    AbsolutePinnedHard,
>>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>>> +    /// Timer does not move between CPU cores.
>>>> +    /// Timer handler is executed in hard irq context.
>>>> +    RelativePinnedHard,
>>>> +}
>>>
>>> At some point we probably want to move this to bitfields, or do you
>>> think it's better to keep it like this?
>>
>> Yes, eventually the would transition. The main difficulty is that not
>> all flag combinations are legal, and the zero value is also a flag.
>> There was some promising work being shared on Zulip for this [1], but I
>> don't think it is completed yet. Added Timothy to CC.
>>
>> [1] https://rust-for-linux.zulipchat.com/#narrow/channel/291565-Help/topic/Best.20way.20to.20handle.20enum.2Fflags.20situation
>
> Ah yeah I remember that. And also the complication about certain
> combinations not being allowed.
>
>>>> +
>>>> +impl From<HrTimerMode> for bindings::hrtimer_mode {
>>>> +    fn from(value: HrTimerMode) -> Self {
>>>> +        use bindings::*;
>>>> +        match value {
>>>> +            HrTimerMode::Absolute => hrtimer_mode_HRTIMER_MODE_ABS,
>>>> +            HrTimerMode::Relative => hrtimer_mode_HRTIMER_MODE_REL,
>>>> +            HrTimerMode::Pinned => hrtimer_mode_HRTIMER_MODE_PINNED,
>>>> +            HrTimerMode::Soft => hrtimer_mode_HRTIMER_MODE_SOFT,
>>>> +            HrTimerMode::Hard => hrtimer_mode_HRTIMER_MODE_HARD,
>>>> +            HrTimerMode::AbsolutePinned => hrtimer_mode_HRTIMER_MODE_ABS_PINNED,
>>>> +            HrTimerMode::RelativePinned => hrtimer_mode_HRTIMER_MODE_REL_PINNED,
>>>> +            HrTimerMode::AbsoluteSoft => hrtimer_mode_HRTIMER_MODE_ABS_SOFT,
>>>> +            HrTimerMode::RelativeSoft => hrtimer_mode_HRTIMER_MODE_REL_SOFT,
>>>> +            HrTimerMode::AbsolutePinnedSoft => hrtimer_mode_HRTIMER_MODE_ABS_PINNED_SOFT,
>>>> +            HrTimerMode::RelativePinnedSoft => hrtimer_mode_HRTIMER_MODE_REL_PINNED_SOFT,
>>>> +            HrTimerMode::AbsoluteHard => hrtimer_mode_HRTIMER_MODE_ABS_HARD,
>>>> +            HrTimerMode::RelativeHard => hrtimer_mode_HRTIMER_MODE_REL_HARD,
>>>> +            HrTimerMode::AbsolutePinnedHard => hrtimer_mode_HRTIMER_MODE_ABS_PINNED_HARD,
>>>> +            HrTimerMode::RelativePinnedHard => hrtimer_mode_HRTIMER_MODE_REL_PINNED_HARD,
>>>> +        }
>>>> +    }
>>>> +}
>>>> +
>>>> +impl From<HrTimerMode> for u64 {
>>>> +    fn from(value: HrTimerMode) -> Self {
>>>> +        Into::<bindings::hrtimer_mode>::into(value) as u64
>>>> +    }
>>>> +}
>>>
>>> Hmm do drivers really need these impls? If not, then you could also just
>>> have a private function that does the conversion and use it only in the
>>> abstraction layer.
>>
>> Similar to the other impls you commented on, I can move them private. I
>> would prefer using `From` rather than some other function.
>
> What's the reason for you preferring `From`? I don't think it's
> important to forbid access from the drivers, but if it's unnecessary,
> why would we give them access in the first place?

TIL trait implementations cannot be hidden. I like `From` because every
single rust developer seeing a `From` implementation will immediately know
what it does. It is more idiomatic in that way than having another
conversion function. I think using existing traits with well defined
semantics is preferable to a local function.

But since driver implementer do not need it, I'm undecided. If you want
them converted to a private function I can do that, for all the ones you
called out. But I am also OK with keeping as is. You decide.


Best regards,
Andreas Hindborg