Re: [Minios-devel] [UNIKRAFT PATCH v4 8/9] plat/linuxu: Add monotonic clock

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello,

Please find the comment inline:

On 08/20/2018 01:21 PM, Florian Schmidt wrote:
> From: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
>
> Implement ukplat_monotonic_clock() with clock_gettime()
> system call.
>
> Signed-off-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> Signed-off-by: Florian Schmidt <florian.schmidt@xxxxxxxxx>
> ---
>   plat/linuxu/include/linuxu/syscall-arm_32.h |  1 +
>   plat/linuxu/include/linuxu/syscall-x86_64.h |  1 +
>   plat/linuxu/include/linuxu/syscall.h        |  7 +++++++
>   plat/linuxu/include/linuxu/time.h           |  1 +
>   plat/linuxu/time.c                          | 15 +++++++++++++--
>   5 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/plat/linuxu/include/linuxu/syscall-arm_32.h 
> b/plat/linuxu/include/linuxu/syscall-arm_32.h
> index b393627..c3a684c 100644
> --- a/plat/linuxu/include/linuxu/syscall-arm_32.h
> +++ b/plat/linuxu/include/linuxu/syscall-arm_32.h
> @@ -53,6 +53,7 @@
>   #define __SC_TIMER_GETTIME    259
>   #define __SC_TIMER_GETOVERRUN 260
>   #define __SC_TIMER_DELETE     261
> +#define __SC_CLOCK_GETTIME    263
>   #define __SC_PSELECT6 335
>   
>   /* NOTE: from `man syscall`:
> diff --git a/plat/linuxu/include/linuxu/syscall-x86_64.h 
> b/plat/linuxu/include/linuxu/syscall-x86_64.h
> index fb09dd5..26820dc 100644
> --- a/plat/linuxu/include/linuxu/syscall-x86_64.h
> +++ b/plat/linuxu/include/linuxu/syscall-x86_64.h
> @@ -53,6 +53,7 @@
>   #define __SC_TIMER_GETTIME    224
>   #define __SC_TIMER_GETOVERRUN 225
>   #define __SC_TIMER_DELETE     226
> +#define __SC_CLOCK_GETTIME    228
>   #define __SC_PSELECT6 270
>   
>   /* NOTE: from linux-4.6.3 (arch/x86/entry/entry_64.S):
> diff --git a/plat/linuxu/include/linuxu/syscall.h 
> b/plat/linuxu/include/linuxu/syscall.h
> index 6df3108..edd7458 100644
> --- a/plat/linuxu/include/linuxu/syscall.h
> +++ b/plat/linuxu/include/linuxu/syscall.h
> @@ -70,6 +70,13 @@ static inline int sys_exit(int status)
>                               (long) (status));
>   }
>   
> +static inline int sys_clock_gettime(clockid_t clk_id, struct timespec *tp)
> +{
> +       return (int) syscall2(__SC_CLOCK_GETTIME,
> +                             (long) clk_id,
> +                             (long) tp);
> +}
> +
>   /*
>    * Please note that on failure sys_mmap() is returning -errno
>    */
> diff --git a/plat/linuxu/include/linuxu/time.h 
> b/plat/linuxu/include/linuxu/time.h
> index c4c97e1..94bf91b 100644
> --- a/plat/linuxu/include/linuxu/time.h
> +++ b/plat/linuxu/include/linuxu/time.h
> @@ -41,5 +41,6 @@
>   /* POSIX definitions */
>   
>   #define CLOCK_REALTIME       0
> +#define CLOCK_MONOTONIC      1
>   
>   #endif /* __LINUXU_TIME_H__ */
> diff --git a/plat/linuxu/time.c b/plat/linuxu/time.c
> index e1fae2c..9c9d0bb 100644
> --- a/plat/linuxu/time.c
> +++ b/plat/linuxu/time.c
> @@ -34,6 +34,7 @@
>    */
>   
>   #include <string.h>
> +#include <time.h>
>   #include <uk/plat/time.h>
>   #include <uk/plat/irq.h>
>   #include <uk/assert.h>
> @@ -47,8 +48,18 @@ static timer_t timerid;
>   
>   __nsec ukplat_monotonic_clock(void)
>   {
> -       /* TODO */
> -       return 0;

Why are we using the timespec from the libc here? In the patch 4 of this 
series we defined our uk platform internal data structure for the 
signals. I would suggest we define a single design principle and stick 
with it.
> +       struct timespec tp;
> +       __nsec ret;
> +       int rc;
> +
> +       rc = sys_clock_gettime(CLOCK_MONOTONIC, &tp);
> +       if (unlikely(rc != 0))
> +               return 0;
> +
> +       ret = ukarch_time_sec_to_nsec((__nsec) tp.tv_sec);
> +       ret += (__nsec) tp.tv_nsec;
> +
> +       return ret;
>   }
>   
>   static int timer_handler(void *arg __unused)
Thanks & Regards
Sharan

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel