This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 02/12] Change most internal uses of __gettimeofday to __clock_gettime.
On Tue, Aug 20, 2019 at 4:56 PM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
> On 20/08/2019 10:21, Zack Weinberg wrote:
> > Since gettimeofday will shortly be implemented in terms of
> > clock_gettime on all platforms, internal code should use clock_gettime
> > directly; in addition to removing a layer of indirection, this will
> > allow us to remove the PLT-bypass gunk for gettimeofday. In many
> > cases, the changed code does fewer conversions.
...
> > if (__clock_gettime (CLOCK_MONOTONIC, &result.current) != 0)
> > - {
> > - struct timeval current_tv;
> > - if (__gettimeofday (¤t_tv, NULL) == 0)
> > - __libc_fatal ("Fatal error: gettimeofday system call failed\n");
> > - result.current.tv_sec = current_tv.tv_sec;
> > - result.current.tv_nsec = current_tv.tv_usec * 1000;
> > - }
> > + if (__clock_gettime (CLOCK_REALTIME, &result.current) != 0)
> > + __libc_fatal ("Fatal error: clock_gettime failed\n");
> > assert (result.current.tv_sec >= 0);
> > return result;
> > }
>
> I think it is a fair assumption that CLOCK_REALTIME is always supported, as it
> is also specific by POSIX.1-2017. It allows simplify it further and remove the
> libc_fatal fallback code.
OK, I will make that change (here and elsewhere).
...
> > - __gettimeofday (&tv, NULL);
> > + __clock_gettime (CLOCK_REALTIME, &rt);
> >
> > /* Compute relative timeout. */
> > - rt.tv_sec = abstime->tv_sec - tv.tv_sec;
> > - rt.tv_nsec = abstime->tv_nsec - tv.tv_usec * 1000;
> > + rt.tv_sec = abstime->tv_sec - rt.tv_sec;
> > + rt.tv_nsec = abstime->tv_nsec - rt.tv_nsec;
> > if (rt.tv_nsec < 0)
> > {
> > rt.tv_nsec += 1000000000;
>
> I think we can simplify it even more by adding a cancellable
> lll_futex_clock_wait_bitset variant
I would rather not do that in this patch set.
This patch in particular should be as mechanical a replacement of
(__)gettimeofday with __clock_gettime as possible, but also, this
patch *set* is supposed to be about groundwork for 64-bit time_t.
Adding a new lowlevellock function in order to tighten up code is too
much scope creep for my taste.
...
> > /* Recompute the timeout time. */
> > - (void) __gettimeofday (&now, NULL);
> > - timeout = end - (now.tv_sec * 1000 + (now.tv_usec + 500) / 1000);
> > + __clock_gettime (CLOCK_REALTIME, &now);
> > + timeout = end - ((now.tv_sec * 1000 +
> > + (now.tv_nsec + 500000) / 1000000));
> > }
> > }
> >
>
> Couldn't we use the support/timespec-* to operate with struct timespec
> along with a helper function to get get current time?
Similarly, although I think we probably _should_ add timespec_add,
timespec_sub, timespec_cmp functions to the public <time.h> API, as
well as using them internally, I do not want to do that in this patch set.
> static inline struct timespec
> timespec_now (void)
> {
> struct timespec r;
> __clock_gettime (CLOCK_REALTIME, &r);
> return r;
> }
It is not obvious whether this function should use CLOCK_REALTIME or
CLOCK_MONOTONIC, so I do not think we should have it at all.
> > /*
> > * Figure out the "time", accounting for any time difference
> > * with the server if necessary.
> > */
> > - __gettimeofday (&tval, (struct timezone *) NULL);
> > - ad->ad_timestamp.tv_sec = tval.tv_sec + ad->ad_timediff.tv_sec;
> > - ad->ad_timestamp.tv_usec = tval.tv_usec + ad->ad_timediff.tv_usec;
> > + __clock_gettime (CLOCK_REALTIME, &now);
> > + ad->ad_timestamp.tv_sec = now.tv_sec + ad->ad_timediff.tv_sec;
> > + ad->ad_timestamp.tv_usec = (now.tv_nsec / 1000) + ad->ad_timediff.tv_usec;
> > if (ad->ad_timestamp.tv_usec >= MILLION)
> > {
> > ad->ad_timestamp.tv_usec -= MILLION;
>
> Ok (I think we probably should handle overflow here).
I don't understand the parenthetical. Are you saying that we already
do handle overflow and that's good, or that overflow handling needs to
be added?
> > printf ("%s: %lld.%06d\n",
> > - what, (long long int) tv.tv_sec, (int) tv.tv_usec);
> > + what, (long long int) tv.tv_sec, (int) tv.tv_nsec / 1000);
> > else
> > printf ("%s: %04d-%02d-%02dT%02d:%02d:%02d.%06d\n",
> > what, 1900 + tm.tm_year, tm.tm_mon + 1, tm.tm_mday,
> > - tm.tm_hour, tm.tm_min, tm.tm_sec, (int) tv.tv_usec);
> > + tm.tm_hour, tm.tm_min, tm.tm_sec, (int) tv.tv_nsec / 1000);
...
> Same as Paul has said about printing timestamps.
OK, will change.
> > +# define GETTIME(low,high) \
> > + { \
> > + struct timespec now; \
> > + uint64_t usecs; \
> > + __clock_gettime (CLOCK_REALTIME, &now); \
> > + usecs = (uint64_t)now.tv_nsec / 1000 + (uint64_t)now.tv_sec * 1000000; \
> > + low = usecs & 0xffffffff; \
> > + high = usecs >> 32; \
> > }
> > #endif
> >
>
> I think it should have a space after the cast type.
Then it wouldn't fit into 80 columns.
> > - if (__gettimeofday (&elapsed, NULL) < 0)
> > + if (__host_get_time (__mach_host_self (), (time_value_t *) &elapsed) < 0)
> > return -1;
>
> Looks ok (I can't really check if Hurd change is fully correct though).
Oh, this is actually wrong, I need to change it to use an actual
time_value_t rather than type punning. GCC 9 complains about it in
other places, dunno why not this one.
The construct
__host_get_time (__mach_host_self (), &tvt)
seems to be Mach's equivalent of gettimeofday(), but I do not actually
know this any better than you do, I copied it out of the former
sysdeps/mach/gettimeofday.c.
> > - if (__gettimeofday (&now, NULL) < 0)
> > + if (__host_get_time (__mach_host_self (), (time_value_t *) &now) < 0)
> > {
> > __spin_unlock (&_hurd_itimer_lock);
> > _hurd_critical_section_unlock (crit);
>
> Looks ok.
Similar issue with time_value_t vs struct timeval, will fix.
> > +# define timespec_sub(a, b, result) \
> > + do { \
> > + (result)->tv_sec = (a)->tv_sec - (b)->tv_sec; \
> > + (result)->tv_nsec = (a)->tv_nsec - (b)->tv_nsec; \
> > + if ((result)->tv_nsec < 0) { \
> > + --(result)->tv_sec; \
> > + (result)->tv_nsec += 1000000000; \
> > + } \
> > + } while (0)
> > +
>
> As before, I would prefer to use the support/timerspec_* routines intead of
> reimplement them.
As before: would really rather not do that in this patch series.
> > # define RANDOM_BITS(Var) \
> > { \
> > - struct timeval tv; \
> > - __gettimeofday (&tv, NULL); \
> > - (Var) = ((uint64_t) tv.tv_usec << 16) ^ tv.tv_sec; \
> > + struct timespec ts; \
> > + clock_gettime (CLOCK_REALTIME, &ts); \
> > + (Var) = ((uint64_t) tv.tv_nsec << 16) ^ tv.tv_sec; \
> > }
> > #endif
>
> It should be __clock_gettime.
Not here. If you look at the whole file, this definition of
RANDOM_BITS is used only when _LIBC is *not* defined.
zw