[PATCH RFC] rust: HrTimerMode replacement with bitfield_options macro

[PATCH RFC] rust: HrTimerMode replacement with bitfield_options macro

[Timothy Garwood via B4 Relay]

From: Timothy Garwood <gtimothy-dev@protonmail.com>

replace the HrTimerMode enum with a bitfield wrapper built using the
bitfield_options macro

---
This patch provides an example of usage of the bitfield_options macro
proposed in [1] as a replacement for the HrTimerMode enum introduced in
[2].
[1] is a RFC.
This patch depends on both [1] and [2]

Link:
https://lore.kernel.org/rust-for-linux/20250128-add-bitfield-options-macro-v2-1-d65349b389fc@protonmail.com
Link:
https://lore.kernel.org/rust-for-linux/20250110-hrtimer-v3-v6-12-rc2-v6-0-f71d50f16482@kernel.org
[2]

Signed-off-by: Timothy Garwood <gtimothy-dev@protonmail.com>
---
 rust/kernel/time/hrtimer.rs | 82 ++++++++++-----------------------------------
 1 file changed, 17 insertions(+), 65 deletions(-)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index f759e4b1aa9f9fa6d55d5c144deedccc5d0590ae..ecbd7ebd7bf319291fec5bc5de31fe4a36d03c24 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -364,75 +364,27 @@ fn from(value: HrTimerRestart) -> Self {
     }
 }
 
-/// 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,
+kernel::bitfield_options! {
+    name: HrTimerMode,
+    type: bindings::hrtimer_mode,
+    options: [
+        {name: relative,
+            true: bindings::hrtimer_mode_HRTIMER_MODE_REL,
+            false: bindings::hrtimer_mode_HRTIMER_MODE_ABS
+        },
+        {name: pinned,
+            true: bindings::hrtimer_mode_HRTIMER_MODE_PINNED,
+            false: 0},
+        {name: hard,
+            true: bindings::hrtimer_mode_HRTIMER_MODE_HARD,
+            false: bindings::hrtimer_mode_HRTIMER_MODE_SOFT}
+    ]
 }
 
 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,
-        }
+        use kernel::bitfield::BitField;
+        value.bits()
     }
 }
 

---
base-commit: b9a6cbc3bad5eaf0e413c9487d8a0f2d3309bf0c
change-id: 20250127-bitfield-option-hrtimer-7ece9f179b1d
prerequisite-change-id: 20241017-hrtimer-v3-v6-12-rc2-215dc6b169bf:v6
prerequisite-patch-id: 23d01479b2ab6b48af65eb681817c753f2b36874
prerequisite-patch-id: 407fc78a43b558f7c6d9cde9a7bf211a06185afb
prerequisite-patch-id: d8f3138d942b3ebfa3549cf54f6bb1c522401ba9
prerequisite-patch-id: 11beab205013c97d7e9a686352b1359910b5938b
prerequisite-patch-id: 84bfb19dfd05dba3aac82b4f2b0031cf6aee8566
prerequisite-patch-id: fada7846a89465ac08b4ab049f9bc626abc27589
prerequisite-patch-id: 526e62d0d80535f4c1b2edb62ee8b7a0f6d86f78
prerequisite-patch-id: 201342b1c7635b147218cefdeded3bc326f59503
prerequisite-patch-id: 271bb7aa324adcf71bdc3c8fcfaefa269ed8ab74
prerequisite-patch-id: 75c30cfa3e81a8d4895567c16aa0b279616d0bdd
prerequisite-patch-id: 68c2afbd51b25083614b2d963b6d32cbb3c3bb9a
prerequisite-patch-id: 37d31228bd445445dbb1baa23ea83ab1e5c7a47b
prerequisite-patch-id: fa49bf80231519c5397367f2d9467f3c28868620
prerequisite-patch-id: d1c5ffd588712325a3372dd4f582ace4debd594d
prerequisite-change-id: 20241108-add-bitfield-options-macro-a847b39910aa:v2
prerequisite-patch-id: 7db40a5cde7387419cd346305da28e2c4932eca1

Best regards,
-- 
Timothy Garwood <gtimothy-dev@protonmail.com>

Re: [PATCH RFC] rust: HrTimerMode replacement with bitfield_options macro

[Miguel Ojeda]

On Tue, Jan 28, 2025 at 6:13 PM Timothy Garwood via B4 Relay
<devnull+gtimothy-dev.protonmail.com@kernel.org> wrote:
>
> This patch provides an example of usage of the bitfield_options macro
> proposed in [1] as a replacement for the HrTimerMode enum introduced in
> [2].
> [1] is a RFC.
> This patch depends on both [1] and [2]

This is great, that is exactly what I was thinking about, and
showcases the advantage indeed :)

One quick question: what happens with the generated docs of the enum
and each variant etc.? (since that is part of the savings in lines
here)

Nits/tips: when you send the next version, consider sending both as a
series, i.e. this patch being #2 in the series, and #1 the one the
"real patch" that introduces the macro. That way they stay linked and
people browsing their emails and/or Lore will see it together :)

Also, the notes on the commit that are not part of the commit should
go after the SoB and the `---` line, before the diffstat.

There is also some strange text wrapping going on in the message, e.g.
on the `Link:` lines.

Cheers,
Miguel

Re: [PATCH RFC] rust: HrTimerMode replacement with bitfield_options macro

[Timothy Garwood]

> One quick question: what happens with the generated docs of the enum
> and each variant etc.? (since that is part of the savings in lines
> here)

Here it is all lost.
This is something that could be much improved: for now, the macro just 
generates a default documentation that equates to this:

+ // A wrapper around a bindings::hrtimer_mode bitfield that sets bitflags using its relative, pinned, and hard methods.
+ pub struct HrTimerMode(/* private fields */);
+
+ // if true, unsets the bindings::hrtimer_mode_HRTIMER_MODE_ABS bit(s) and sets the bindings::hrtimer_mode_HRTIMER_MODE_REL bit(s).
+ // Otherwise, does the opposite.
+ pub fn relative(self, relative: bool) -> Self
+
+ // if true, unsets the 0 bit(s) and sets the bindings::hrtimer_mode_HRTIMER_MODE_PINNED bit(s).
+ // Otherwise, does the opposite.
+ pub fn pinned(self, pinned: bool) -> Self
+
+ // if true, unsets the bindings::hrtimer_mode_HRTIMER_MODE_SOFT bit(s) and sets the bindings::hrtimer_mode_HRTIMER_MODE_HARD bit(s).
+ // Otherwise, does the opposite.
+ pub fn hard(self, hard: bool) -> Self

I would like to find a way to have better documentation. 
Doing something like the 
+ <#optional_attributes>
+ <visibility> <wrapper_name>(<field_type>);
mentioned in [1] could open the door to passing the documentation for the type.
The method's documentation would need a different solution.

> Nits/tips: when you send the next version, consider sending both as a
> series, i.e. this patch being #2 in the series, and #1 the one the
> "real patch" that introduces the macro. That way they stay linked and
> people browsing their emails and/or Lore will see it together :)

Even if this patch depends on something ([2]) the "real patch" does not depend on?

> Also, the notes on the commit that are not part of the commit should
> go after the SoB and the `---` line, before the diffstat.

I am using b4, I think I just signed the wrong commit? I'll make sure to sign the
code commit instead of the cover-letter commit

> There is also some strange text wrapping going on in the message, e.g.
> on the `Link:` lines.

I forgot one [1] reference for the first link. I'll fix it.

Thank you Miguel,
Timothy

Re: [PATCH RFC] rust: HrTimerMode replacement with bitfield_options macro

[Miguel Ojeda]

On Tue, Jan 28, 2025 at 10:23 PM Timothy Garwood
<gtimothy-dev@protonmail.com> wrote:
>
> Even if this patch depends on something ([2]) the "real patch" does not depend on?

I think it is fine, as long as it is clearly mentioned that it is
meant as expository only.

If dependencies are an issue, another option that may give you more
freedom is simply replying to the original patch with the extra one,
so that they are linked at least in the same thread.

> I forgot one [1] reference for the first link. I'll fix it.

Ah, what I meant is that some of the text is line-wrapped, i.e. the
newline breaks are a bit strangely positioned in the main text and
e.g. after the `Link:` tags there are newlines that should not be
there.

> Thank you Miguel,

You're welcome!

Cheers,
Miguel

Re: [PATCH RFC] rust: HrTimerMode replacement with bitfield_options macro

[Andreas Hindborg]

"Timothy Garwood via B4 Relay" <devnull+gtimothy-dev.protonmail.com@kernel.org> writes:

> From: Timothy Garwood <gtimothy-dev@protonmail.com>
>
> replace the HrTimerMode enum with a bitfield wrapper built using the
> bitfield_options macro
>
> ---
> This patch provides an example of usage of the bitfield_options macro
> proposed in [1] as a replacement for the HrTimerMode enum introduced in
> [2].
> [1] is a RFC.
> This patch depends on both [1] and [2]
>

Very cool!

Since `hrtimer` is not upstream yet, if you get a non RFC version ready
before the next `hrtimer` spin, I can put a dependency on that if you
want?


Best regards,
Andreas Hindborg

Re: [PATCH RFC] rust: HrTimerMode replacement with bitfield_options macro

[Timothy Garwood]

> Very cool!

Thank you :)

> Since `hrtimer` is not upstream yet, if you get a non RFC version ready
> before the next `hrtimer` spin, I can put a dependency on that if you
> want?

Sure, that would be nice.

Thanks Andreas,

Timothy