Re: [RFC][PATCH] generic hp_timing

[Mike Frysinger <vapier at gentoo dot org>]

On 31 Aug 2015 17:07, Maria Guseva wrote:
> The attached patch is a draft implementation of generic timing: via
> clock_gettime, vdso or system call, where available and via gettimeofday
> function call otherwise.
> 
> In order to eliminate performance impact in existing usages of HP_TIMING_NOW
> Âthe HP_TIMING_AVAIL is still set to 0. The generic timing is enabled only
> in DL_DEBUG_STATISTICS cases where performance is not critical. 
> However I suggest it may be wrong and we can use at least vdso clock_gettime
> as real HP_TIMING. And the rest probably shouldn't be even considered as
> high-precision timers and should be moved from hp-timing header to some new
> one. 

i tend to agree -- vdso+clock_gettime support should be sufficient for HP timing

> -#ifndef HP_TIMING_NONAVAIL
> +#if HP_TIMING_AVAIL

i wouldn't mind renaming this as a sep patch.  i dislike negative names.

> +#if HP_TIMING_AVAIL
>    if (HP_SMALL_TIMING_AVAIL)
>      HP_TIMING_NOW (_dl_cpuclock_offset);
> +#endif

we should avoid CPP here imo.  if !HP_TIMING_AVAIL, then shouldn't it
already be !HP_SMALL_TIMING_AVAIL ?  so you don't need the #if here.

> +/* Macros to define timing in case of DL_DEBUG_STATISTICS set */

your comment style needs fixing in many places.  should be (note the end):
/* Macros to define timing in case of DL_DEBUG_STATISTICS set.  */

> +# if HP_TIMING_AVAIL || HP_GENERIC_TIMING_AVAIL
> +#define DL_STATISTICS_TIMING_NOW(tval) \
> +if (__glibc_unlikely (HP_SMALL_TIMING_AVAIL &&                      \
> +                      (GLRO(dl_debug_mask) & DL_DEBUG_STATISTICS))) \
> +  HP_TIMING_NOW (tval)

these macros are dangerous as written.  they should be wrapped in a:
	do { ... } while (0)

> +#define HP_TIMING_NOW(var)                                              \
> +do                                                                      \
> +  {                                                                     \
> +    struct timespec __t;                                                \
> +    INTERNAL_SYSCALL_DECL (__err);                                      \
> +    __INTERNAL_SYSCALL (clock_gettime, __err, 2, CLOCK_MONOTONIC, &(__t));\

don't need the paren around the __t

> +#define HP_TIMING_NOW(var)                                              \
> +do                                                                      \
> +  {                                                                     \
> +    struct timeval __t;                                                 \
> +    (void) __gettimeofday (&__t, NULL)                                  \

why do you need the (void) ?

also, this fails to compile -- there's a missing ; at the end.
-mike

Attachment: signature.asc
Description: Digital signature