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.
