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 21/08/2019 18:09, Zack Weinberg wrote:
> 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.
Alright, I think I will send it as a subsequent cleanup after this patch
has landed.
>
> ...
>>> /* 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.
In fact we are using them just for libsupport, my suggestion would be to
include them on libc internally to help on the clock_gettime usage. The
public inclusion on <time.h> should be in another discussion/patch.
>
>> 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.
Alright, although I think we can still add the helper functions with a
different prototype:
static inline struct timespec
timespec_now_clockid (clockid_t clk_id)
{
_Static_assert (clk_id == CLOCK_REALTIME);
struct timespec r;
__clock_gettime (clk_id, &r);
return r;
}
static inline struct timespec
timespec_now_realtime (void)
{
/* CLOCK_REALTIME is always support. */
return timespec_now_clockid (CLOCK_REALTIME);
}
This allows rule out usage with invalid clocks and simplify its usage
on most common cases.
>
>>> /*
>>> * 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?
I tend to frown upon these kind of operations that blindly adjust values
without some sort of sanitization, but it might be the case where overflow
values can't happen or are not really an issue. The 'ad_timediff' seems
to be adjusted only at this place (other places seems to zero it), and
below there an explicit comment that y2038 might trigger an issue. But
for a mechanical patch to replace gettimeoday I think the patch looks ok.
>
>>> 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.
The information I could get is __host_get_time accessed an mmap area (similar
to vDSO) that contains the system time. And yeah, even though Hurd time_value_t
is a struct with similar layout of struct timeval, it is not really valid to
cast is on __host_get_time (struct timeval also defines as a tv_usec, which
is __suseconds_t/long int).
>
>>> - 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.
I understand you want the patch to mechanically as possible, however I also see
counterproductive to replicate a routine that we already have a better implemented
one in the same code base. These additions tend to linger until someone actively
refactors it.
>
>>> # 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.
Right, I think we should follow Wilco suggestion and just clean this file up
with onyl glibc definitions.