Re: [Xen-devel] [PATCH v4 3/7] add gettimeofday function to time managment

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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 <phentex@xxxxxxxxx>
> > > ---
> > > 
> > > 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 <xtf/types.h>
> > >   #include <xtf/traps.h>
> > >   #include <xtf/time.h>
> > > +#include <xen/errno.h>
> > 
> > Sorting.
> > 
> > >   #include <arch/barrier.h>
> > >   #include <arch/lib.h>
> > > @@ -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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel