Re: [Xen-devel] [PATCH v4 3/7] add gettimeofday function to time managment
On Mon, Apr 16, 2018 at 12:16:18PM +0200, Paul Semel wrote: > Hi ! > > Thanks a lot for reviewing ! > > On 04/13/2018 03:39 PM, Roger Pau Monné wrote: > > On Tue, Apr 10, 2018 at 09:16:57PM +0200, Paul Semel wrote: > > > this function acts as the POSIX gettimeofday function > > > > > > Signed-off-by: Paul Semel > > > --- > > > > > > Notes: > > > v4: > > > - new patch version > > > > > > common/time.c | 30 ++ > > > include/xtf/time.h | 8 > > > 2 files changed, 38 insertions(+) > > > > > > diff --git a/common/time.c b/common/time.c > > > index c1b7cd1..8489f3b 100644 > > > --- a/common/time.c > > > +++ b/common/time.c > > > @@ -1,6 +1,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > > Sorting. > > > > > #include > > > #include > > > @@ -109,6 +110,35 @@ uint64_t current_time(void) > > > return sec + boot_time; > > > } > > > +/* The POSIX gettimeofday syscall normally takes a second argument, > > > which is > > > + * the timezone (struct timezone). However, it sould be NULL because > > > linux > > > + * doesn't use it anymore. So we need for us to add it in this function > > > + */ > > > +int gettimeofday(struct timeval *tp, void *restrict tzp) > > > +{ > > > +uint64_t boot_time, sec; > > > +uint32_t mod, nsec; > > > + > > > +if ( tzp != NULL ) > > > +return -EOPNOTSUPP; > > > + > > > +if ( tp == NULL ) > > > +return -EINVAL; > > > + > > > +get_time_info(&boot_time, &sec, &nsec); > > > > Why are you using get_time_info here? Shouldn't you use the > > current_time function introduced in the previous patch? > > > > Or else I don't see the need to introduce current_time in the previous > > patch. > > > > Actually, I can't use *only* the current_time function here, because I won't > be able to get the nanoseconds if so. > > Anyway, in the last patch, I am using current_time function for the NOW() > macro, which I think is really helpful. > > Do you think I should drop all of those ? Without having any users of those functions it's hard to tell which ones we should keep and which ones should be removed. IMO, I would keep gettimeofday in order to get the current time. Then I would also keep the m/u/sleep functions and add a small selftest in test/selftest/main.c that makes use of those functions, for example something as simple as: s = gettimeofday msleep(1) if ( s + 1ms < gettimeofday ) xtf_failure("Fail: error in time keeping functions\n") Thanks, Roger. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 3/7] add gettimeofday function to time managment
Hi ! Thanks a lot for reviewing ! On 04/13/2018 03:39 PM, Roger Pau Monné wrote: On Tue, Apr 10, 2018 at 09:16:57PM +0200, Paul Semel wrote: this function acts as the POSIX gettimeofday function Signed-off-by: Paul Semel --- Notes: v4: - new patch version common/time.c | 30 ++ include/xtf/time.h | 8 2 files changed, 38 insertions(+) diff --git a/common/time.c b/common/time.c index c1b7cd1..8489f3b 100644 --- a/common/time.c +++ b/common/time.c @@ -1,6 +1,7 @@ #include #include #include +#include Sorting. #include #include @@ -109,6 +110,35 @@ uint64_t current_time(void) return sec + boot_time; } +/* The POSIX gettimeofday syscall normally takes a second argument, which is + * the timezone (struct timezone). However, it sould be NULL because linux + * doesn't use it anymore. So we need for us to add it in this function + */ +int gettimeofday(struct timeval *tp, void *restrict tzp) +{ +uint64_t boot_time, sec; +uint32_t mod, nsec; + +if ( tzp != NULL ) +return -EOPNOTSUPP; + +if ( tp == NULL ) +return -EINVAL; + +get_time_info(&boot_time, &sec, &nsec); Why are you using get_time_info here? Shouldn't you use the current_time function introduced in the previous patch? Or else I don't see the need to introduce current_time in the previous patch. Actually, I can't use *only* the current_time function here, because I won't be able to get the nanoseconds if so. Anyway, in the last patch, I am using current_time function for the NOW() macro, which I think is really helpful. Do you think I should drop all of those ? +#if defined(__i386__) +mod = divmod64(&boot_time, SEC_TO_NSEC(1)); +#else +mod = boot_time % SEC_TO_NSEC(1); +boot_time /= SEC_TO_NSEC(1); +#endif Please use divmod64 unconditionally. + +tp->sec = sec + boot_time; +tp->nsec = nsec + mod; +return 0; +} + /* * Local variables: * mode: C diff --git a/include/xtf/time.h b/include/xtf/time.h index e33dc8a..ce4d6db 100644 --- a/include/xtf/time.h +++ b/include/xtf/time.h @@ -8,6 +8,12 @@ #include +struct timeval { +uint64_t sec; +uint64_t nsec; +}; + + Extra newline. Thanks, -- Paul ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 3/7] add gettimeofday function to time managment
On Tue, Apr 10, 2018 at 09:16:57PM +0200, Paul Semel wrote: > this function acts as the POSIX gettimeofday function > > Signed-off-by: Paul Semel > --- > > Notes: > v4: > - new patch version > > common/time.c | 30 ++ > include/xtf/time.h | 8 > 2 files changed, 38 insertions(+) > > diff --git a/common/time.c b/common/time.c > index c1b7cd1..8489f3b 100644 > --- a/common/time.c > +++ b/common/time.c > @@ -1,6 +1,7 @@ > #include > #include > #include > +#include Sorting. > > #include > #include > @@ -109,6 +110,35 @@ uint64_t current_time(void) > return sec + boot_time; > } > > +/* The POSIX gettimeofday syscall normally takes a second argument, which is > + * the timezone (struct timezone). However, it sould be NULL because linux > + * doesn't use it anymore. So we need for us to add it in this function > + */ > +int gettimeofday(struct timeval *tp, void *restrict tzp) > +{ > +uint64_t boot_time, sec; > +uint32_t mod, nsec; > + > +if ( tzp != NULL ) > +return -EOPNOTSUPP; > + > +if ( tp == NULL ) > +return -EINVAL; > + > +get_time_info(&boot_time, &sec, &nsec); Why are you using get_time_info here? Shouldn't you use the current_time function introduced in the previous patch? Or else I don't see the need to introduce current_time in the previous patch. > +#if defined(__i386__) > +mod = divmod64(&boot_time, SEC_TO_NSEC(1)); > +#else > +mod = boot_time % SEC_TO_NSEC(1); > +boot_time /= SEC_TO_NSEC(1); > +#endif Please use divmod64 unconditionally. > + > +tp->sec = sec + boot_time; > +tp->nsec = nsec + mod; > +return 0; > +} > + > /* > * Local variables: > * mode: C > diff --git a/include/xtf/time.h b/include/xtf/time.h > index e33dc8a..ce4d6db 100644 > --- a/include/xtf/time.h > +++ b/include/xtf/time.h > @@ -8,6 +8,12 @@ > > #include > > +struct timeval { > +uint64_t sec; > +uint64_t nsec; > +}; > + > + Extra newline. Roger. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v4 3/7] add gettimeofday function to time managment
this function acts as the POSIX gettimeofday function Signed-off-by: Paul Semel --- Notes: v4: - new patch version common/time.c | 30 ++ include/xtf/time.h | 8 2 files changed, 38 insertions(+) diff --git a/common/time.c b/common/time.c index c1b7cd1..8489f3b 100644 --- a/common/time.c +++ b/common/time.c @@ -1,6 +1,7 @@ #include #include #include +#include #include #include @@ -109,6 +110,35 @@ uint64_t current_time(void) return sec + boot_time; } +/* The POSIX gettimeofday syscall normally takes a second argument, which is + * the timezone (struct timezone). However, it sould be NULL because linux + * doesn't use it anymore. So we need for us to add it in this function + */ +int gettimeofday(struct timeval *tp, void *restrict tzp) +{ +uint64_t boot_time, sec; +uint32_t mod, nsec; + +if ( tzp != NULL ) +return -EOPNOTSUPP; + +if ( tp == NULL ) +return -EINVAL; + +get_time_info(&boot_time, &sec, &nsec); + +#if defined(__i386__) +mod = divmod64(&boot_time, SEC_TO_NSEC(1)); +#else +mod = boot_time % SEC_TO_NSEC(1); +boot_time /= SEC_TO_NSEC(1); +#endif + +tp->sec = sec + boot_time; +tp->nsec = nsec + mod; +return 0; +} + /* * Local variables: * mode: C diff --git a/include/xtf/time.h b/include/xtf/time.h index e33dc8a..ce4d6db 100644 --- a/include/xtf/time.h +++ b/include/xtf/time.h @@ -8,6 +8,12 @@ #include +struct timeval { +uint64_t sec; +uint64_t nsec; +}; + + #define SEC_TO_NSEC(x) ((x) * 10ul) @@ -16,6 +22,8 @@ uint64_t since_boot_time(void); uint64_t current_time(void); +int gettimeofday(struct timeval *tp, void *restrict tzp); + #endif /* XTF_TIME_H */ /* -- 2.16.1 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel