Re: [PATCH v2 2/4] time: make getboottime64 aware of time namespace

["J. Bruce Fields" <bfields@fieldses.org>]

On Fri, Oct 09, 2020 at 10:08:15PM +0200, Thomas Gleixner wrote:
> On Fri, Oct 09 2020 at 09:55, J. Bruce Fields wrote:
> > Looking at how it's used in net/sunrpc/cache.c....  All it's doing is
> > comparing times which have all been calculated relative to the time
> > returned by getboottime64().  So it doesn't really matter what
> > getboottime64() is, as long as it's always the same.
> >
> > So, I don't think this should change behavior of the sunrpc code at all.
> 
> You wish. That's clearly wrong because that code is not guaranteed to
> always run in a context which belongs to the root time namespace.

Argh, right, thanks.

> AFAICT, this stuff can run in softirq context which is context stealing
> and the interrupted task can belong to a different time name space.

Some of it runs in the context of a process doing IO to proc, some from
kthreads.  So, anyway, yes, it's not consistent in the way we'd need.

> In fact the whole thing can be simplified. You can just use time in
> nanoseconds retrieved via ktime_get_coarse_boottime() which does not
> read the clocksource and advances once per tick and does not contain a
> divison and is definitely faster than seconds_since_boot()
> 
> The expiry time is initialized via get_expiry() which does:
> 
>    getboottime64(&boot);
>    return rv - boot.tv_sec; 
> 
> The expiry value 'rv' which is read out of the buffer is wall time in
> seconds. So all you need is are function which convert real to boottime
> and vice versa. That's trivial to implement and again faster than
> getboottime64(). Something like this:
> 
> ktime_t ktime_real_to_boot(ktime_t real)
> {
>         struct timekeeper *tk = &tk_core.timekeeper;
>         ktime_t mono = ktime_sub(real, tk->offs_real);
>               
>         return ktime_add(mono, tk->offs_boot);
> }
> 
> so the above becomes:
> 
>    return ktime_real_to_boot(rv * NSEC_PER_SEC);
> 
> which is still faster than a division.
> 
> The nanoseconds value after converting back to wall clock will need a
> division to get seconds since the epoch, but that's not an issue because
> that backward conversion already has one today.
> 
> You'd obviously need to fixup CACHE_NEW_EXPIRY and the other place which
> add's '1' to the expiry value and some janitoring of function names and
> variable types, but no real big surgery AFAICT.

I'll give it a shot.

Thanks so much for taking a careful look at this.

--b.