Feedback-ID: iad51458e:Fastmail
Date:   Tue, 21 Feb 2023 06:06:33 -0800
From:   Boqun Feng <boqun.feng@gmail.com>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     Asahi Lina <lina@asahilina.net>, Miguel Ojeda <ojeda@kernel.org>,
        Alex Gaynor <alex.gaynor@gmail.com>,
        Wedson Almeida Filho <wedsonaf@gmail.com>,
        Gary Guo <gary@garyguo.net>,
        =?iso-8859-1?Q?Bj=F6rn?= Roy Baron <bjorn3_gh@protonmail.com>,
        John Stultz <jstultz@google.com>,
        Stephen Boyd <sboyd@kernel.org>, linux-kernel@vger.kernel.org,
        rust-for-linux@vger.kernel.org, asahi@lists.linux.dev
Subject: Re: [PATCH] rust: time: New module for timekeeping functions
Message-ID: <Y/TP6as7qqwfcI42@boqun-archlinux>
References: <20230221-gpu-up-time-v1-1-bf8fe74b7f55@asahilina.net>
 <87v8jvnqq4.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87v8jvnqq4.ffs@tglx>
Precedence: bulk

On Tue, Feb 21, 2023 at 01:32:51PM +0100, Thomas Gleixner wrote:
> On Tue, Feb 21 2023 at 16:06, Asahi Lina wrote:
> > +
> > +use crate::bindings;
> > +use core::time::Duration;
> > +
> > +/// Returns the kernel time elapsed since boot, excluding time spent sleeping, as a [`Duration`].
> > +pub fn ktime_get() -> Duration {
> > +    // SAFETY: Function has no side effects and no inputs.
> > +    Duration::from_nanos(unsafe { bindings::ktime_get() }.try_into().unwrap())
> 
> Why is this a Duration? From the spec:
> 

I agree that returning a Duration may not be ideal, but..

>     Duration
> 
>         A Duration type to represent a span of time, typically used for
>         system timeouts.
> 
>     Instant
> 
>         A measurement of a monotonically nondecreasing clock. Opaque and
>         useful only with Duration.
> 
> In my understanding 'Duration' is a time span between two points, while
> ktime_get() and ktime_get_boottime() return the current time of
> monotonically nondecreasing clocks, i.e. they fall into the 'Instant'
> category.
> 
> Now the problem is that 'Instant' in it's specification is bound to
> CLOCK_MONOTONIC and there is no way to express CLOCK_BOOTTIME, but
> that's a shortcoming of the spec which ignores CLOCK_BOOTTIME
> completely. IOW, that's also a problem for user space.
> 
> This makes sense vs. the other representation:
> 
>      SystemTime
>      
>         A measurement of the system clock, useful for talking to
>         external entities like the file system or other processes.
> 
> This maps to CLOCK_REALTIME and CLOCK_TAI, i.e. ktime_get_real_ns() and
> ktime_get_clocktai().
> 
> Similar to 'Instant' 'SystemTime' is strictly bound to CLOCK_REALTIME
> by specification and there is no way to read CLOCK_TAI.
> 

..'Instant' and 'SystemTime' are in Rust std, we cannot use them
directly, similar as we cannot use userspace libc. To me, there seems
two options to provide Rust types for kernel time management:

*	Use KTime which maps to ktime_t, then we have the similar
	semantics around it: sometimes it's a duration, sometimes it's
	a point of time.. but I know "this is a safe language, you
	should do more" ;-)

*	Introduce kernel's own types, e.g. BootTime, RawTime, TAI,
	RealTime, and make them play with Duration (actually I'd prefer
	we have own Duration, because Rust core::time::Duration takes
	more than u64), something like below:


	pub struct BootTime {
	    d: Duration
	}

	impl BootTime {
	    fn now() -> Self {
	        unsafe { BootTime { d: ktime_to_duration(ktime_get_boottime())} }
	    }
	    fn add(self, d: Duration) -> Self {
	        <Add a duration, similar to ktime_add>
	    }
	    fn sub(self, other: Self) -> Duration {
	        ...
	    }
	...
	}

Thoughts?

Regards,
Boqun

> Please fix this in the spec and do not try to work around that by
> pretending that a clock read is a 'Duration'.
> 
> > +}
> > +
> > +/// Returns the kernel time elapsed since boot, including time spent sleeping, as a [`Duration`].
> > +pub fn ktime_get_boottime() -> Duration {
> > +    Duration::from_nanos(
> > +        // SAFETY: Function has no side effects and no variable inputs.
> > +        unsafe { bindings::ktime_get_with_offset(bindings::tk_offsets_TK_OFFS_BOOT) }
> 
> No. Please use ktime_get_boottime() and not the timekeeping internal function.
> 
> Thanks,
> 
>         tglx