[PATCH_v2] use CLOCK_MONOTONIC_COARSE posix timer instead of CLOCK_MONOTONIC in Xorg

[ykzhao]

On Fri, 2010-08-27 at 18:51 +0800, Daniel Stone wrote:
> On Fri, Aug 27, 2010 at 05:32:55PM +0800, ykzhao wrote:
> > On Fri, 2010-08-27 at 10:15 +0800, Daniel Stone wrote:
> > > On Fri, Aug 27, 2010 at 09:20:59AM +0800, yakui.zhao at intel.com wrote:
> > > >    Limit the CLOCK_MONOTONIC_COARSE posix timer to linux platform. This is
> > > > to avoid the issue that OS doesn't support CLOCK_MONOTINC_COARSE posix timer
> > > > while the corresponding clock id is for other purpose. At the same time it
> > > > won't be created if the CLOCK_MONOTONIC_COARSE is defined in system header
> > > > file.
> > > 
> > > ... what?? CLOCK_MONOTONIC_COARSE _always_ means the coarse monotonic
> > > clock if defined, on any platform.  It's always safe to use.
> > 
> > Do you mean that it is safe to use the coarse monotonic clock if it is
> > defined? Right?
> 
> Yes.
> 

Ok. I will remove the check of __linux__ macro definition in the
following patch. 

> > But in the discussion of V1 version, it seems that it is not reasonable
> > for other OS. Maybe the CLOCK_MONOTONIC_COARSE posix timer is not
> > supported. But the corresponding clock id is used for other purpose. In
> > such case it will be not safe.
> 
> I don't think the point's getting across, and for a patch that reduces
> the CPU load _less than the measurement error_, it doesn't really seem
> worth discussing more.  Can you please test the attached patch and give
> your Tested-by/Reviewed-by if OK? I'd like to end these threads after,
> what, 60 mails? More?

It is no problem that I will test it and add the "tested-by
or /reviewed-by". 

In fact I will state my point about more check of CLOCK_MONOTONIC_COARSE
posix timer in previous thread.
   It is not wrong to add more check for CLOCK_MONOTONIC_COARSE by using
the clock_gettime again. But IMO it will be redundant. The function of
clock_getres can check whether the CLOCK_MONOTONIC_COARSE is supported
by OS. Why is it worth adding it again?

Another concern is that two things are done in one patch. 
   >To add the strict check of CLOCK_MONOTONIC has nothing to do with
that the CLOCK_MONOTONIC_COARSE posix timer is used.

Best regards.
   Yakui
   
> 
> Cheers,
> Daniel
> 
> From 2ba33ed4671f9034172358ddc8d432b0f1730c85 Mon Sep 17 00:00:00 2001
> From: Daniel Stone <daniel at fooishbar.org>
> Date: Fri, 27 Aug 2010 20:36:37 +1000
> Subject: [PATCH] GetTimeInMillis: Use CLOCK_MONOTONIC_COARSE where available
> 
> On some systems, using CLOCK_MONOTONIC forces a readback of HPET or some
> similarly expensive timer.  CLOCK_MONOTONIC_COARSE can alleviate this,
> at the cost of negligibly-reduced resolution, so prefer that where we
> can.
> 
> Signed-off-by: Daniel Stone <daniel at fooishbar.org>
> ---
>  os/utils.c |   16 +++++++++++++++-
>  1 files changed, 15 insertions(+), 1 deletions(-)
> 
> diff --git a/os/utils.c b/os/utils.c
> index 7aa392a..efca6b4 100644
> --- a/os/utils.c
> +++ b/os/utils.c
> @@ -429,7 +429,21 @@ GetTimeInMillis(void)
>  
>  #ifdef MONOTONIC_CLOCK
>      struct timespec tp;
> -    if (clock_gettime(CLOCK_MONOTONIC, &tp) == 0)
> +    static clockid_t clockid;
> +    if (!clockid) {
> +#ifdef CLOCK_MONOTONIC_COARSE
> +        if (clock_getres(CLOCK_MONOTONIC_COARSE, &tp) == 0 &&
> +            (tp.tv_nsec / 1000) <= 1000 &&
> +            clock_gettime(CLOCK_MONOTONIC_COARSE, &tp) == 0)
> +            clockid = CLOCK_MONOTONIC_COARSE;
> +        else
> +#endif
> +        if (clock_gettime(CLOCK_MONOTONIC, &tp) == 0)
> +            clockid = CLOCK_MONOTONIC;
> +        else
> +            clockid = ~0L;
> +    }
> +    if (clockid != ~0L && clock_gettime(clockid, &tp) == 0)
>          return (tp.tv_sec * 1000) + (tp.tv_nsec / 1000000L);
>  #endif
>