From: Thomas Gleixner <tglx@linutronix.de>
To: Andreas Hindborg <nmi@metaspace.dk>
Cc: "Miguel Ojeda" <ojeda@kernel.org>,
"Alex Gaynor" <alex.gaynor@gmail.com>,
"Wedson Almeida Filho" <wedsonaf@gmail.com>,
"Anna-Maria Behnsen" <anna-maria@linutronix.de>,
"Frederic Weisbecker" <frederic@kernel.org>,
"Andreas Hindborg" <a.hindborg@samsung.com>,
"Boqun Feng" <boqun.feng@gmail.com>,
"Gary Guo" <gary@garyguo.net>,
"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
"Benno Lossin" <benno.lossin@proton.me>,
"Alice Ryhl" <aliceryhl@google.com>,
rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] rust: hrtimer: introduce hrtimer support
Date: Wed, 01 May 2024 00:25:01 +0200 [thread overview]
Message-ID: <87ikzysd36.ffs@tglx> (raw)
In-Reply-To: <87le4uk936.fsf@metaspace.dk>
Andreas!
On Tue, Apr 30 2024 at 20:18, Andreas Hindborg wrote:
> Thomas Gleixner <tglx@linutronix.de> writes:
>> On Thu, Apr 25 2024 at 11:46, Andreas Hindborg wrote:
>>> +// SAFETY: A `Timer` can be moved to other threads and used from there.
>>> +unsafe impl<T> Send for Timer<T> {}
>>> +
>>> +// SAFETY: Timer operations are locked on C side, so it is safe to operate on a
>>> +// timer from multiple threads
>>
>> Kinda. Using an hrtimer from different threads needs some thought in the
>> implementation as obviously ordering matters:
>>
>> T1 T2
>> hrtimer_start() hrtimer_cancel()
>>
>> So depending on whether T1 gets the internal lock first or T2 the
>> outcome is different. If T1 gets it first the timer is canceled by
>> T2. If T2 gets it first the timer ends up armed.
>
> That is all fine. What is meant here is that we will not get UB in the
> `hrtimer` subsystem when racing these operations. As far as I can tell
> from the C source, the operations are atomic, even though their
> interleaving will not be deterministic.
That's correct. All operations happen with the associated base lock held.
>>> +unsafe impl<T> Sync for Timer<T> {}
>>> +
>>> +impl<T: TimerCallback> Timer<T> {
>>> + /// Return an initializer for a new timer instance.
>>> + pub fn new() -> impl PinInit<Self> {
>>> + crate::pin_init!( Self {
>>> + timer <- Opaque::ffi_init(move |place: *mut bindings::hrtimer| {
>>> + // SAFETY: By design of `pin_init!`, `place` is a pointer 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,
>>
>> This is odd. The initializer really should take a clock ID and a mode
>> argument. Otherwise you end up implementing a gazillion of different
>> timers.
>
> I implemented the minimum set of features to satisfy the requirements
> for the Rust null block driver. It is my understanding that most
> maintainers of existing infrastructure prefers to have a user for the
> implemented features, before wanting to merge them.
>
> I can try to extend the abstractions to cover a more complete `hrtimer`
> API. Or we can work on this subset and try to get that ready to merge,
> and then expand scope later.
Wouldn't expanding scope later require to change already existing call sites?
>>> + );
>>> + }
>>> +
>>> + // SAFETY: `place` is pointing to a live allocation, so the deref
>>> + // is safe. The `function` field might not be initialized, but
>>> + // `addr_of_mut` does not create a reference to the field.
>>> + let function: *mut Option<_> = unsafe { core::ptr::addr_of_mut!((*place).function) };
>>> +
>>> + // SAFETY: `function` points to a valid allocation.
>>> + unsafe { core::ptr::write(function, Some(T::Receiver::run)) };
>>
>> We probably should introduce hrtimer_setup(timer, clockid, mode, function)
>> to avoid this construct. That would allow to cleanup existing C code too.
>
> Do you want me to cook up a C patch for that, or would you prefer to do
> that yourself?
Please create that patch yourself and convert at least one C location to
this new interface in a separate patch. THe remaining C cleanup can go
from there and mostly be scripted with coccinelle.
>>> +/// [`Box<T>`]: Box
>>> +/// [`Arc<T>`]: Arc
>>> +/// [`ARef<T>`]: crate::types::ARef
>>> +pub trait RawTimer: Sync {
>>> + /// Schedule the timer after `expires` time units
>>> + fn schedule(self, expires: u64);
>>
>> Don't we have some time related rust types in the kernel by now?
>
> There are patches on the list, but I think they are not applied to any
> tree yet? I did not want to depend on those patches before they are
> staged somewhere. Would you prefer this patch on top of the Rust `ktime`
> patches?
The initial set is queued in
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git timers/core
for 6.10. Boqun has some updates on top IIRC. Your stuff should go
through that branch too.
>>> + // SAFETY: This `Arc` comes from a call to `Arc::into_raw()`
>>> + let receiver = unsafe { Arc::from_raw(data_ptr) };
>>> +
>>> + T::run(receiver);
>>> +
>>> + bindings::hrtimer_restart_HRTIMER_NORESTART
>>
>> One of the common use cases of hrtimers is to create periodic schedules
>> where the timer callback advances the expiry value and returns
>> HRTIMER_RESTART. It might be not required for your initial use case at
>> hand, but you'll need that in the long run IMO.
>
> If you are OK with taking that feature without a user, I will gladly add
> it.
I'm fine with taking a more complete API which does not require to
change usage sites later on.
Thanks,
tglx
next prev parent reply other threads:[~2024-04-30 22:25 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-25 9:46 [PATCH] rust: hrtimer: introduce hrtimer support Andreas Hindborg
2024-04-26 7:52 ` Benno Lossin
2024-04-26 9:27 ` Andreas Hindborg
2024-04-29 17:31 ` Boqun Feng
2024-04-30 12:33 ` Andreas Hindborg
2024-04-30 15:17 ` Boqun Feng
2024-04-30 18:22 ` Andreas Hindborg
2024-04-30 17:05 ` Thomas Gleixner
2024-04-29 12:49 ` Alice Ryhl
2024-04-29 13:47 ` Andreas Hindborg
2024-04-30 17:02 ` Thomas Gleixner
2024-04-30 18:18 ` Andreas Hindborg
2024-04-30 22:25 ` Thomas Gleixner [this message]
2024-05-01 11:37 ` Andreas Hindborg
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87ikzysd36.ffs@tglx \
--to=tglx@linutronix.de \
--cc=a.hindborg@samsung.com \
--cc=alex.gaynor@gmail.com \
--cc=aliceryhl@google.com \
--cc=anna-maria@linutronix.de \
--cc=benno.lossin@proton.me \
--cc=bjorn3_gh@protonmail.com \
--cc=boqun.feng@gmail.com \
--cc=frederic@kernel.org \
--cc=gary@garyguo.net \
--cc=linux-kernel@vger.kernel.org \
--cc=nmi@metaspace.dk \
--cc=ojeda@kernel.org \
--cc=rust-for-linux@vger.kernel.org \
--cc=wedsonaf@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).