[v2,3/4] malloc: Use hp-timing on libmemusage

Instead of reimplemeting on GETTIME macro.
---
 malloc/memusage.c            | 15 ++++++++++++---
 sysdeps/generic/memusage.h   | 12 ------------
 sysdeps/i386/i686/memusage.h |  1 -
 sysdeps/ia64/memusage.h      |  7 -------
 sysdeps/x86_64/memusage.h    |  1 -
 5 files changed, 12 insertions(+), 24 deletions(-)

On Thu, Dec 23, 2021 at 02:37:05PM -0300, Adhemerval Zanella via Libc-alpha wrote:
> Instead of reimplemeting on GETTIME macro.
> ---
>  malloc/memusage.c            | 15 ++++++++++++---
>  sysdeps/generic/memusage.h   | 12 ------------
>  sysdeps/i386/i686/memusage.h |  1 -
>  sysdeps/ia64/memusage.h      |  7 -------
>  sysdeps/x86_64/memusage.h    |  1 -
>  5 files changed, 12 insertions(+), 24 deletions(-)
> 
> diff --git a/malloc/memusage.c b/malloc/memusage.c
> index de39ad1c1a..179dd1c683 100644
> --- a/malloc/memusage.c
> +++ b/malloc/memusage.c
> @@ -34,6 +34,7 @@
>  #include <sys/time.h>
>  
>  #include <memusage.h>
> +#include <hp-timing.h>
>  
>  /* Pointer to the real functions.  These are determined used `dlsym'
>     when really needed.  */
> @@ -114,6 +115,14 @@ static struct entry buffer[2 * DEFAULT_BUFFER_SIZE];
>  static uint32_t buffer_cnt;
>  static struct entry first;
>  
> +static void
> +gettime (struct entry *e)
> +{
> +  hp_timing_t now;
> +  HP_TIMING_NOW (now);
> +  e->time_low = now & 0xffffffff;
> +  e->time_high = now >> 32;
> +}

OK. this matches the generic definitions.

>  /* Update the global data after a successful function call.  */
>  static void
> @@ -177,7 +186,7 @@ update_data (struct header *result, size_t len, size_t old_len)
>  
>        buffer[idx].heap = current_heap;
>        buffer[idx].stack = current_stack;
> -      GETTIME (buffer[idx].time_low, buffer[idx].time_high);
> +      gettime (&buffer[idx]);
>  
>        /* Write out buffer if it is full.  */
>        if (idx + 1 == buffer_size)
> @@ -267,7 +276,7 @@ me (void)
>                /* Write the first entry.  */
>                first.heap = 0;
>                first.stack = 0;
> -              GETTIME (first.time_low, first.time_high);
> +              gettime (&first);
>                /* Write it two times since we need the starting and end time. */
>                write (fd, &first, sizeof (first));
>                write (fd, &first, sizeof (first));
> @@ -818,7 +827,7 @@ dest (void)
>           stack.  */
>        first.heap = peak_heap;
>        first.stack = peak_stack;
> -      GETTIME (first.time_low, first.time_high);
> +      gettime (&first);
>        write (fd, &first, sizeof (struct entry));
>  
>        /* Close the file.  */

OK.

> diff --git a/sysdeps/generic/memusage.h b/sysdeps/generic/memusage.h
> index c9bde5cd11..514bd058d2 100644
> --- a/sysdeps/generic/memusage.h
> +++ b/sysdeps/generic/memusage.h
> @@ -23,15 +23,3 @@
>  # warning "GETSP is not defined for this architecture."
>  # define GETSP 0
>  #endif
> -
> -#ifndef GETTIME
> -# define GETTIME(low,high)						   \
> -  {									   \
> -    struct __timespec64 now;						   \
> -    uint64_t usecs;							   \
> -    __clock_gettime64 (CLOCK_REALTIME, &now);				   \
> -    usecs = (uint64_t)now.tv_nsec / 1000 + (uint64_t)now.tv_sec * 1000000; \
> -    low = usecs & 0xffffffff;						   \
> -    high = usecs >> 32;							   \
> -  }
> -#endif

Replacing with genric definition of GET_TIME_NOW...

    #ifdef _ISOMAC
    # define HP_TIMING_NOW(var) \
    ({                                                              \
      struct timespec tv;                                           \
      clock_gettime (CLOCK_MONOTONIC, &tv);                         \
      (var) = (tv.tv_nsec + UINT64_C(1000000000) * tv.tv_sec);      \
    })
    #else
    # define HP_TIMING_NOW(var) \
    ({                                                              \
      struct __timespec64 tv;                                               \
      __clock_gettime64 (CLOCK_MONOTONIC, &tv);                     \
      (var) = (tv.tv_nsec + UINT64_C(1000000000) * tv.tv_sec);      \
    })
    #endif

This is now CLOCK_MONOTONIC rather than CLOCK_REALTIME.  Also it returns
nanoseconds rather than microseconds.

I think this is OK though as it seems the time unit in memusage is not
defined.

> diff --git a/sysdeps/i386/i686/memusage.h b/sysdeps/i386/i686/memusage.h
> index 08b7831e12..07b241263c 100644
> --- a/sysdeps/i386/i686/memusage.h
> +++ b/sysdeps/i386/i686/memusage.h
> @@ -16,6 +16,5 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #define GETSP() ({ register uintptr_t stack_ptr asm ("esp"); stack_ptr; })
> -#define GETTIME(low,high) asm ("rdtsc" : "=a" (low), "=d" (high))
>  
>  #include <sysdeps/generic/memusage.h>
> diff --git a/sysdeps/ia64/memusage.h b/sysdeps/ia64/memusage.h
> index 3c3a526007..33fd6ec899 100644
> --- a/sysdeps/ia64/memusage.h
> +++ b/sysdeps/ia64/memusage.h
> @@ -18,12 +18,5 @@
>  #include <hp-timing.h>
>  
>  #define GETSP() ({ register uintptr_t stack_ptr asm ("%r12"); stack_ptr; })
> -#define GETTIME(low, high) \
> -  {									      \
> -    hp_timing_t __now;							      \
> -    HP_TIMING_NOW (__now);						      \
> -    low = __now & 0xffffffff;						      \
> -    high = __now >> 32;							      \
> -  }
>  
>  #include <sysdeps/generic/memusage.h>
> diff --git a/sysdeps/x86_64/memusage.h b/sysdeps/x86_64/memusage.h
> index 568dd5512d..6652fc5da1 100644
> --- a/sysdeps/x86_64/memusage.h
> +++ b/sysdeps/x86_64/memusage.h
> @@ -16,6 +16,5 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #define GETSP() ({ register uintptr_t stack_ptr asm ("rsp"); stack_ptr; })
> -#define GETTIME(low,high) asm ("rdtsc" : "=a" (low), "=d" (high))
>  
>  #include <sysdeps/generic/memusage.h>

-Stafford

On 27/12/2021 09:40, Stafford Horne wrote:
>> diff --git a/sysdeps/generic/memusage.h b/sysdeps/generic/memusage.h
>> index c9bde5cd11..514bd058d2 100644
>> --- a/sysdeps/generic/memusage.h
>> +++ b/sysdeps/generic/memusage.h
>> @@ -23,15 +23,3 @@
>>  # warning "GETSP is not defined for this architecture."
>>  # define GETSP 0
>>  #endif
>> -
>> -#ifndef GETTIME
>> -# define GETTIME(low,high)						   \
>> -  {									   \
>> -    struct __timespec64 now;						   \
>> -    uint64_t usecs;							   \
>> -    __clock_gettime64 (CLOCK_REALTIME, &now);				   \
>> -    usecs = (uint64_t)now.tv_nsec / 1000 + (uint64_t)now.tv_sec * 1000000; \
>> -    low = usecs & 0xffffffff;						   \
>> -    high = usecs >> 32;							   \
>> -  }
>> -#endif
> 
> Replacing with genric definition of GET_TIME_NOW...
> 
>     #ifdef _ISOMAC
>     # define HP_TIMING_NOW(var) \
>     ({                                                              \
>       struct timespec tv;                                           \
>       clock_gettime (CLOCK_MONOTONIC, &tv);                         \
>       (var) = (tv.tv_nsec + UINT64_C(1000000000) * tv.tv_sec);      \
>     })
>     #else
>     # define HP_TIMING_NOW(var) \
>     ({                                                              \
>       struct __timespec64 tv;                                               \
>       __clock_gettime64 (CLOCK_MONOTONIC, &tv);                     \
>       (var) = (tv.tv_nsec + UINT64_C(1000000000) * tv.tv_sec);      \
>     })
>     #endif
> 
> This is now CLOCK_MONOTONIC rather than CLOCK_REALTIME.  Also it returns
> nanoseconds rather than microseconds.
> 
> I think this is OK though as it seems the time unit in memusage is not
> defined.

Right, I did not realize the clock id change. I will change to keep current
behavior:

  static void
  gettime (struct entry *e)
  {
  #if HP_TIMING_INLINE
    hp_timing_t now;
    HP_TIMING_NOW (now);
    e->time_low = now & 0xffffffff;
    e->time_high = now >> 32;
  #else
    struct __timespec64 now;
    uint64_t usecs;
    __clock_gettime64 (CLOCK_REALTIME, &now);
    usecs = (uint64_t)now.tv_nsec / 1000 + (uint64_t)now.tv_sec * 1000000;
    e->time_low = usecs & 0xffffffff;
    e->time_high = usecs >> 32;
  #endif
  }