Thanks. Again, my review inline below. -- Nadav Har'El n...@scylladb.com
On Wed, May 10, 2023 at 11:58 AM luca abeni <lucab...@gmail.com> wrote: > From: Claudio Fontana <claudio.font...@huawei.com> > > we only support CLOCK_MONOTONIC, and we don't support remainder > due to the signal implementation. > > Signed-off-by: Claudio Fontana <claudio.font...@huawei.com> > --- > include/osv/sched.hh | 10 +++++++++ > libc/time.cc | 49 +++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 58 insertions(+), 1 deletion(-) > > diff --git a/include/osv/sched.hh b/include/osv/sched.hh > index c1f414ba..03502c38 100644 > --- a/include/osv/sched.hh > +++ b/include/osv/sched.hh > @@ -513,6 +513,8 @@ public: > inline void wake_with_from_mutex(Action action); > template <class Rep, class Period> > static void sleep(std::chrono::duration<Rep, Period> duration); > + template <class Clock, class Duration> > + static void sleep(std::chrono::time_point<Clock, Duration> > time_point); > The function name sleep() is deeply ingrained in C / Unix / Linux users' minds as something that takes a relative time (duration) as parameter (i.e., sleep for N seconds), not a deadline (time_point). So I suggest giving this new function a different name. A good name is "sleep_until", this is also the name that C++'s thread API uses: https://en.cppreference.com/w/cpp/thread/sleep_until > /** > * Let the other thread on the current CPU run if there is any. > * > @@ -1407,6 +1409,14 @@ void thread::sleep(std::chrono::duration<Rep, > Period> duration) > sleep_impl(t); > } > > +template <class Clock, class Duration> > +void thread::sleep(std::chrono::time_point<Clock, Duration> time_point) > +{ > + timer t(*current()); > + t.set(time_point); > + wait_until([&] { return t.expired(); }); > +} > + > template <class Action> > inline > void thread::wake_with_irq_or_preemption_disabled(Action action) > diff --git a/libc/time.cc b/libc/time.cc > index 2660eb86..c31f31fd 100644 > --- a/libc/time.cc > +++ b/libc/time.cc > @@ -15,7 +15,18 @@ > #include <osv/sched.hh> > #include "pthread.hh" > > -u64 convert(const timespec& ts) > +#define timespecisset(t) ((t)->tv_sec || (t)->tv_nsec) > +#define timespecclear(t) ((t)->tv_sec = (t)->tv_nsec = 0) > +#define timespeccmp(s,t,op) ((s)->tv_sec == (t)->tv_sec ? \ > + (s)->tv_nsec op (t)->tv_nsec : (s)->tv_sec > op (t)->tv_sec) > +#define timespecadd(s,t,a) ( (a)->tv_sec = (s)->tv_sec + (t)->tv_sec, \ > + ((a)->tv_nsec = (s)->tv_nsec + (t)->tv_nsec) >= 1000000000L && \ > + ((a)->tv_nsec -= 1000000000L, (a)->tv_sec++) ) > +#define timespecsub(s,t,a) ( (a)->tv_sec = (s)->tv_sec - (t)->tv_sec, \ > + ((a)->tv_nsec = (s)->tv_nsec - (t)->tv_nsec) < 0 && \ > + ((a)->tv_nsec += 1000000000L, (a)->tv_sec--) ) > Unless I'm missing something, you aren't using any of these macros below, just timespeccmp(), so please remove all the unused ones. In general we should need functions to add or subtract timestamps, because what we normally would do is to convert the timespec to a std::chrono::timepoint, and those we can add and subtract with the "+" and "-" operators which are already implemented by std::chrono. We can also do the same with <=, by the way. By the way, in modern C++ (and C), it's much nicer to replace macros by inline functions, which are easier to understand and also give you some compile-time checking on types, etc. > + > +static u64 convert(const timespec& ts) > { > return ts.tv_sec * 1000000000 + ts.tv_nsec; > } > @@ -36,6 +47,9 @@ int gettimeofday(struct timeval* tv, struct timezone* tz) > OSV_LIBC_API > int nanosleep(const struct timespec* req, struct timespec* rem) > { > + if (!req || req->tv_nsec < 0 || req->tv_nsec >= 1000000000L || > req->tv_sec < 0) > + return libc_error(EINVAL); > + > sched::thread::sleep(std::chrono::nanoseconds(convert(*req))); > return 0; > } > @@ -127,3 +141,36 @@ clock_t clock(void) > clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &ts); > return ts.tv_sec * 1000000000L + ts.tv_nsec; > } > + > +int clock_nanosleep(clockid_t clock_id, int flags, > I think Waldek would like us to put the "OSV_LIBC_API" tag on functions which implement the C library functions - as we do above on nanosleep(). CCing Waldek to agree or correct me. > + const struct timespec *request, > + struct timespec *remain) > +{ > + /* XXX we are implementing really only CLOCK_MONOTONIC, */ > + /* and we don't support remain, due to signals. */ > + if (remain) { > + UNIMPLEMENTED("clock_nanosleep(): remain not supported, due to > signals"); > + } > + if (clock_id != CLOCK_MONOTONIC) { > + UNIMPLEMENTED("clock_nanosleep(): only CLOCK_MONOTONIC is > supported"); > + } > + > + switch (flags) { > + case 0: > + return nanosleep(request, NULL); > + case TIMER_ABSTIME: { > + struct timespec ts; > + if (clock_gettime(clock_id, &ts) != 0) { > + return -1; > + } > + if (timespeccmp(request, &ts, <=)) { > + return 0; > + } > Do we really need to do this check? Won't sched::thread::sleep() simply silently return immediately if the given time point is in the past? Please check, and if it just works, you don't need the above if()s at all, and don't need the timespeccmp function. > + sched::thread::sleep(osv::clock::uptime::time_point( > + > osv::clock::uptime::duration(convert(*request)))); > + return 0; > + } > + default: > + return libc_error(EINVAL); > + } > +} > -- > 2.30.2 > > -- > You received this message because you are subscribed to the Google Groups > "OSv Development" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to osv-dev+unsubscr...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/osv-dev/20230510085745.11416-5-luca.abeni%40santannapisa.it > . > -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/CANEVyjvAPVkwV3HemSAJGK74RGifjKexsMfLV%3D-ywvN1JHTGUw%40mail.gmail.com.
