Created attachment 20192 [details] [review]
updated CLOCK_MONOTONIC patch

This patch is a rewrite of the first; we use the POSIX clock_* APIs, and both detect bits in configure and do a runtime check (exactly once, instead of every time a cond var is created).

Humm; what's the status of this patch, is it possibly going in? That being asked, _dbus_get_current_time seems affected as well, might make sense to patch that as well. Care if I rework that patch a little and propose a bit wider one?

Created attachment 25224 [details] [review]
switch to monotonic clock

Another attempt to use monotonic time source if available.

Created attachment 25787 [details] [review]
take 3 of monotonic clock patch

Quickly tested switch to monotonic timesource. Also fixes cases where transactions are incorrectly declared as 'timed out' when system clock is being set during transaction. Vital for mobile devices..

Hmm, no changing dbus_get_current_time globally is too big of a hammer.  In some cases it'd be OK, but at least one usage in dbus-keyring.c looks like it expects it to be wall-clock.

Other uses of dbus_get_current_time are broken though - I'm looking in particular at _dbus_connection_block_pending_call.  One thing is unclear to me - is it defined whether CLOCK_MONOTONIC increments when the OS is suspended, or not?  It seems to me one could meet the MONOTONIC guarantee by either 

1) Taking a snapshot of time at boot, and ignoring any backwards changes
2) Some division of jiffies

Reading the Linux source right now, it's looking to me like it's #1.  And we actually need something closer to 2), i.e. we don't want to time out method calls just because you suspended.

But...that could be a separate bug.  If the pthread timeouts are what are biting you we can get that in now.

Also, the Makefile.am changes are wrong, we should only link to -lrt if we need to, otherwise people running NetBSD 0.3 on their SPARCstation or some other random crap will probably complain.

Related to this bug:

http://bugzilla.gnome.org/show_bug.cgi?id=540545

commit ae24bb35e2ee3ecde990f55852982b573754ec43
Author: Colin Walters <walters@verbum.org>
Date:   Fri Jul 10 22:27:55 2009 -0400

    Bug 18121 - Use a monotonic clock for pthread timeouts
    
    Patch based on one from Keith Mok <ek9852@gmail.com>, some
    followup work from Janne Karhunen <Janne.Karhunen@gmail.com>.
    
    We don't want condition variable timeouts to be affected by the system clock.
    Use the POSIX CLOCK_MONOTONIC if available.

Comment on attachment 25787 [details] [review]
take 3 of monotonic clock patch

The removal of the comma in AC_CHECK_FUNC seems wrong: we're now running AC_CHECK_LIB as a result of *finding* pthread_cond_timedwait. (as opposed to not finding it)

Also, the code should check for _POSIX_MONOTONIC_CLOCK set to 0: if that's the case, we need to call sysconf(_SC_MONOTONIC_CLOCK) to find out if the system does support monotonic clocks. If that macro is set to 0, it means the system interfaces are present, but the system may not support monotonic clocks at runtime.

The code in dbus-sysdeps-pthread.c seems to have some kind of protection, but dbus-sysdeps-unix.c is unprotected.

I also agree that clock_gettime() should be searched in libc first, before librt.

(In reply to comment #9)
> The removal of the comma in AC_CHECK_FUNC seems wrong: we're now running
> AC_CHECK_LIB as a result of *finding* pthread_cond_timedwait. (as opposed to
> not finding it)
> 
> Also, the code should check for _POSIX_MONOTONIC_CLOCK set to 0: if that's the
> case, we need to call sysconf(_SC_MONOTONIC_CLOCK) to find out if the system
> does support monotonic clocks. If that macro is set to 0, it means the system
> interfaces are present, but the system may not support monotonic clocks at
> runtime.
> 
> The code in dbus-sysdeps-pthread.c seems to have some kind of protection, but
> dbus-sysdeps-unix.c is unprotected.

Any chance you could put together a patch for this, if it affects systems you're interested in?

(In reply to comment #10)
> (In reply to comment #9)
> > The removal of the comma in AC_CHECK_FUNC seems wrong: we're now running
> > AC_CHECK_LIB as a result of *finding* pthread_cond_timedwait. (as opposed to
> > not finding it)

Fixed over on Bug #47239, FWIW.

> > Also, the code should check for _POSIX_MONOTONIC_CLOCK set to 0: if that's the
> > case, we need to call sysconf(_SC_MONOTONIC_CLOCK) to find out if the system
> > does support monotonic clocks.

Not done, patches welcome.

Comment on attachment 20192 [details] [review]
updated CLOCK_MONOTONIC patch

Review of attachment 20192 [details] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +893,2 @@
>                                                      [THREAD_LIBS="-lpthread"])])
> +save_libs="$LIBS"

This check is unnecessary. There's a POSIX macro for this already.

::: dbus/dbus-sysdeps-pthread.c
@@ +200,5 @@
>    if (pcond == NULL)
>      return NULL;
>  
> +  pthread_condattr_init (&attr);
> +#ifdef HAVE_CLOCK_MONOTONIC

#if _POSIX_MONOTONIC_CLOCK >= 0

@@ +265,5 @@
>    
>    _dbus_assert (pmutex->count > 0);
>    _dbus_assert (pthread_equal (pmutex->holder, pthread_self ()));  
> +
> +#ifdef HAVE_CLOCK_MONOTONIC

Idem: #if _POSIX_MONOTONIC_CLOCK >= 0

@@ +348,5 @@
>  
> +static void
> +check_monotonic_clock (void)
> +{
> +#ifdef HAVE_CLOCK_MONOTONIC

Replace this function body with:

#if _POSIX_MONOTONIC_CLOCK == 0
  if (sysconf(_SC_MONOTONIC_CLOCK)) <= 0)
    return;
#endif
#if _POSIX_MONOTONIC_CLOCK >= 0
  have_monotonic_clock = TRUE;
#endif

I believe the patches here have already partially been applied, or something pretty similar has been applied, or something; see Comment #8. So reviewing them isn't much use - if there are flaws in what's currently in libdbus, please fix that instead and attach a patch, and I'll be happy to review it.

To avoid conflicts in configure.ac, please consider reviewing Bug #47239 first.

> #if _POSIX_MONOTONIC_CLOCK >= 0

That does sound like a better way to do the check.

(In reply to comment #5)
> Hmm, no changing dbus_get_current_time globally is too big of a hammer.  In
> some cases it'd be OK, but at least one usage in dbus-keyring.c looks like it
> expects it to be wall-clock.

This has in fact been broken since the patches from this bug were applied. Bug #48580.

Retitling to describe what's left, and removing the patch tag (patches fixing the original bug have already been applied).

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/9.