| Message ID | 20230319064353.686226-3-eggert@cs.ucla.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | use gettimeofday for current time | expand |
Paul Eggert <eggert@cs.ucla.edu> wrote: > Use gettimeofday instead of time(NULL) to get current time. > This avoids clock skew on glibc 2.31+ on Linux, where in the > first 1 to 2.5 ms of every second, time(NULL) returns a > value that is one less than the tv_sec part of > higher-resolution timestamps such as those returned by > gettimeofday or timespec_get, or those in the file system. > There are similar clock skew problems on AIX and MS-Windows, > which have problems in the first 5 ms of every second. Wow, this is enlightening... and unfortunate :< I decided to check glibc archives to find more discussion on it. So far, I've found: https://inbox.sourceware.org/libc-alpha/20230306160321.2942372-1-adhemerval.zanella@linaro.org/T/ and the original bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30200 And this is due to the time64 changes in glibc 2.31+? (<= 2.30 isn't affected?) <snip> > Yet another possibility would be to fix the Linux kernel so > that the time syscall is consistent with the other timestamp > syscalls. I suppose this has not been done due to > performance implications. (Git's use of timestamps is rare > enough that performance is not a significant consideration > for git.) However, this wouldn't fix Git's problem on older > Linux kernels, or on AIX or MS-Windows. Agreed on the older kernels and other OSes part.
Eric Wong <e@80x24.org> writes: > Paul Eggert <eggert@cs.ucla.edu> wrote: >> Use gettimeofday instead of time(NULL) to get current time. >> This avoids clock skew on glibc 2.31+ on Linux, where in the >> first 1 to 2.5 ms of every second, time(NULL) returns a >> value that is one less than the tv_sec part of >> higher-resolution timestamps such as those returned by >> gettimeofday or timespec_get, or those in the file system. >> There are similar clock skew problems on AIX and MS-Windows, >> which have problems in the first 5 ms of every second. > > Wow, this is enlightening... and unfortunate :< > > I decided to check glibc archives to find more discussion on it. > So far, I've found: > > https://inbox.sourceware.org/libc-alpha/20230306160321.2942372-1-adhemerval.zanella@linaro.org/T/ > > and the original bug: > https://sourceware.org/bugzilla/show_bug.cgi?id=30200 > > And this is due to the time64 changes in glibc 2.31+? > (<= 2.30 isn't affected?) > > <snip> > >> Yet another possibility would be to fix the Linux kernel so >> that the time syscall is consistent with the other timestamp >> syscalls. I suppose this has not been done due to >> performance implications. (Git's use of timestamps is rare >> enough that performance is not a significant consideration >> for git.) However, this wouldn't fix Git's problem on older >> Linux kernels, or on AIX or MS-Windows. > > Agreed on the older kernels and other OSes part. Yeah, this is interesting. I however wonder if we should follow our usual pattern of implementing git_time() with the identical function signature as what we replace (i.e. system's time()), and #undef/#define the symbol we replace with git_time, though. Wouldn't it make [1/2] a lot smaller and future-proof?
Junio C Hamano <gitster@pobox.com> writes: > Eric Wong <e@80x24.org> writes: > ... >> I decided to check glibc archives to find more discussion on it. >> So far, I've found: >> >> https://inbox.sourceware.org/libc-alpha/20230306160321.2942372-1-adhemerval.zanella@linaro.org/T/ >> >> and the original bug: >> https://sourceware.org/bugzilla/show_bug.cgi?id=30200 >> >> And this is due to the time64 changes in glibc 2.31+? >> (<= 2.30 isn't affected?) >> > ... > > Yeah, this is interesting. I however wonder if we should follow our > usual pattern of implementing git_time() with the identical function > signature as what we replace (i.e. system's time()), and #undef/#define > the symbol we replace with git_time, though. Wouldn't it make [1/2] > a lot smaller and future-proof? And here is how such an approach may look like. The second part is more or less the same as Paul's version so I kept his authorship there (these are mostly for discussion and not for application). ----- >8 ----- [PATCH 1/2] ----- >8 ----- Subject: [PATCH] time(): allow OVERRIDE_TIME to redefine time(2) Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Makefile | 11 +++++++++++ compat/time.c | 11 +++++++++++ config.mak.uname | 1 + git-compat-util.h | 8 ++++++++ 4 files changed, 31 insertions(+) create mode 100644 compat/time.c diff --git a/Makefile b/Makefile index 50ee51fde3..aa6fcd6e04 100644 --- a/Makefile +++ b/Makefile @@ -286,6 +286,12 @@ include shared.mak # crashes due to allocation and free working on different 'heaps'. # It's defined automatically if USE_NED_ALLOCATOR is set. # +# Define OVERRIDE_TIME to override time(2) and replace it with an +# implementation based on gettimeofday(2). THis is necessary when +# glibc 2.31+ on Linux is used, where in the first 1 to 2.5 ms of +# every second, time(NULL) returns a value that is one less than the +# tv_sec part of higher-resolution timestamps used in the file system. +# # Define NO_REGEX if your C library lacks regex support with REG_STARTEND # feature. # @@ -2061,6 +2067,11 @@ ifdef OVERRIDE_STRDUP COMPAT_OBJS += compat/strdup.o endif +ifdef OVERRIDE_TIME + COMPAT_CFLAGS += -DOVERRIDE_TIME + COMPAT_OBJS += compat/time.o +endif + ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT export GIT_TEST_CMP_USE_COPIED_CONTEXT endif diff --git a/compat/time.c b/compat/time.c new file mode 100644 index 0000000000..a3ef5c0e98 --- /dev/null +++ b/compat/time.c @@ -0,0 +1,11 @@ +#include "../git-compat-util.h" + +#undef time +time_t git_time(time_t *tloc) +{ + time_t t = time(NULL); + + if (tloc) + *tloc = t; + return t; +} diff --git a/config.mak.uname b/config.mak.uname index 64c44db805..29398919e8 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -64,6 +64,7 @@ ifeq ($(uname_S),Linux) PROCFS_EXECUTABLE_PATH = /proc/self/exe HAVE_PLATFORM_PROCINFO = YesPlease COMPAT_OBJS += compat/linux/procinfo.o + OVERRIDE_TIME = YesPlease # centos7/rhel7 provides gcc 4.8.5 and zlib 1.2.7. ifneq ($(findstring .el7.,$(uname_R)),) BASIC_CFLAGS += -std=c99 diff --git a/git-compat-util.h b/git-compat-util.h index 1e6592624d..2279f3c90c 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -917,6 +917,14 @@ void *gitmemmem(const void *haystack, size_t haystacklen, char *gitstrdup(const char *s); #endif +#ifdef OVERRIDE_TIME +#ifdef time +#undef time +#endif +#define time git_time +extern time_t git_time(time_t *); +#endif + #ifdef NO_GETPAGESIZE #define getpagesize() sysconf(_SC_PAGESIZE) #endif
On 3/20/23 10:01, Junio C Hamano wrote: >> I however wonder if we should follow our >> usual pattern of implementing git_time() with the identical function >> signature as what we replace (i.e. system's time()), and #undef/#define >> the symbol we replace with git_time, though. Wouldn't it make [1/2] >> a lot smaller and future-proof? Yes, something like that would work too. (Sorry, I didn't know about the usual pattern.) > +# Define OVERRIDE_TIME to override time(2) and replace it with an > +# implementation based on gettimeofday(2). THis is necessary when > +# glibc 2.31+ on Linux is used, where in the first 1 to 2.5 ms of > +# every second, time(NULL) returns a value that is one less than the > +# tv_sec part of higher-resolution timestamps used in the file system. THis -> This It might be simpler to use the gettimeofday workaround on all platforms, rather than having an OVERRIDE_TIME flag and complicating config.mak.uname. gettimeofday should be portable, as it's already used elsewhere in Git without configury. If we're going with the more-complicated solution, config.mak.uname will need changes in its AIX and MS-Windows sections because the problem is known to occur there too. Although AIX configuration is simple, I'm not sure how to go about the MS-Windows part as there seem to be a lot of ifdefs there. Also, because the time problem does not occur with musl libc (used in Alpine Linux 3.17), on the Linux side the workaround could be employed only if glibc 2.31+ is in use.
Paul Eggert <eggert@cs.ucla.edu> writes: > It might be simpler to use the gettimeofday workaround on all > platforms, rather than having an OVERRIDE_TIME flag and complicating > config.mak.uname. gettimeofday should be portable, as it's already > used elsewhere in Git without configury. That is an excellent point.
On Mon, Mar 20, 2023 at 09:33:01AM -0700, Junio C Hamano wrote: > Yeah, this is interesting. Definitely so. You learn something new every day ;-). > I however wonder if we should follow our usual pattern of implementing > git_time() with the identical function signature as what we replace > (i.e. system's time()), and #undef/#define the symbol we replace with > git_time, though. Wouldn't it make [1/2] a lot smaller and > future-proof? Yeah, I agree here, too. It was my first thought when I started reading Paul's patches here. I think that your approach is sound and I would be happy to see you queue it. Thanks, Taylor
On Mon, Mar 20, 2023 at 12:40:07PM -0700, Junio C Hamano wrote: > Paul Eggert <eggert@cs.ucla.edu> writes: > > > It might be simpler to use the gettimeofday workaround on all > > platforms, rather than having an OVERRIDE_TIME flag and complicating > > config.mak.uname. gettimeofday should be portable, as it's already > > used elsewhere in Git without configury. > > That is an excellent point. I'd be happy to assume OVERRIDE_TIME is set everywhere and just drop it entirely (using gettimeofday() unconditionally within git_time()). An alternative approach might be to leave OVERRIDE_TIME time in place, but treat it as opt-out instead of opt-in. I can imagine that some exotic platform might want to use time() instead of gettimeofday() for one reason or another. Thanks, Taylor
diff --git a/git-compat-util.h b/git-compat-util.h index d05f4bac22..dcd9e5ca65 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -341,7 +341,10 @@ int compat_mkdir_wo_trailing_slash(const char*, mode_t); static inline time_t time_now(void) { - return time(NULL); + /* Avoid time(NULL), which can disagree with gettimeofday etc. */ + struct timeval tv; + gettimeofday(&tv, NULL); + return tv.tv_sec; } #ifdef NO_STRUCT_ITIMERVAL
Use gettimeofday instead of time(NULL) to get current time. This avoids clock skew on glibc 2.31+ on Linux, where in the first 1 to 2.5 ms of every second, time(NULL) returns a value that is one less than the tv_sec part of higher-resolution timestamps such as those returned by gettimeofday or timespec_get, or those in the file system. There are similar clock skew problems on AIX and MS-Windows, which have problems in the first 5 ms of every second. Without this patch, users can observe Git issuing a timestamp T+1 before it issues timestamp T, because Git sometimes uses time(NULL) or time(&t) and sometimes uses higher-res methods like gettimeofday. Although strictly speaking users should tolerate this behavuior because a superuser can always change the clock back, this is a quality of implementation issue and users naturally expect Git to issue timestamps in increasing order unless the superuser has fiddled with the system clock. This patch always uses gettimeofday(...) instead of time(...), and I have verified that the resulting .o files never refer to the name 'time'. A trickier patch would change only those calls for which timestamp monotonicity is user-visible. Such a patch would require more expertise about Git internals, though, and would be harder to maintain later. Another possibility would be to change Git's documentation to warn users that Git does not always issue timestamps in increasing order. However, Git users would likely be either dismayed by this possibility, or confused by the level of detail that any such documentation would require. Yet another possibility would be to fix the Linux kernel so that the time syscall is consistent with the other timestamp syscalls. I suppose this has not been done due to performance implications. (Git's use of timestamps is rare enough that performance is not a significant consideration for git.) However, this wouldn't fix Git's problem on older Linux kernels, or on AIX or MS-Windows. Signed-off-by: Paul Eggert <eggert@cs.ucla.edu> --- git-compat-util.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)