Xen project Mailing List

On Mon, Sep 3, 2018 at 12:03 PM Florian Schmidt <Florian.Schmidt@xxxxxxxxx> wrote:

Hi Dafna,

thank you for this patch. I think you are right, rem shouldn't just
always return 0. I went a bit back and forth on whether this is the
right way to check for the condition, though; which is why it took me so
long to get back to you. After all, the standard says that if "the
nanosleep() function returns because it has been interrupted by a
signal, it shall return a value of -1 and set errno to indicate the
interruption." So it looks like we should check for that instead of
whether we slept long enough.

Then again, nanosleep is also required to sleep at least as long as
requested in req, and rem is then used to potentially sleep for the rest
of the time. So semantically, this does what nanosleep is supposed to
do, so I think this is fine.

There is only one thing that I would change:

On 08/27/2018 08:42 AM, Dafna Hirschfeld wrote:
> Calculate the remaining time to sleep and update
> the rem parameter if it is given.
> If the remaining time is larger than 0, it means that
> the thread was waken up explicitly and nanosleep returns -1
> to indicate that. Otherwise nanosleep returns 0
>
> Signed-off-by: Dafna Hirschfeld <dafna3@xxxxxxxxx>
> ---
> lib/nolibc/time.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/lib/nolibc/time.c b/lib/nolibc/time.c
> index 1c058ae..0787559 100644
> --- a/lib/nolibc/time.c
> +++ b/lib/nolibc/time.c
> @@ -59,9 +59,11 @@ static void __spin_wait(__nsec nsec)
>
> int nanosleep(const struct timespec *req, struct timespec *rem)
> {
> + __nsec before, after;
> __nsec nsec = (__nsec) req->tv_sec * 1000000000L;
>
> nsec += req->tv_nsec;
> + before = (__nsec) ukplat_monotonic_clock();

I would make this call...

>
> #if CONFIG_HAVE_SCHED
> uk_sched_thread_sleep(nsec);
> @@ -69,9 +71,15 @@ int nanosleep(const struct timespec *req, struct timespec *rem)
> __spin_wait(nsec);
> #endif
>
> - if (rem) {
> - rem->tv_sec = 0;
> - rem->tv_nsec = 0;
> + after = (__nsec) ukplat_monotonic_clock();
> + __nsec diff = after-before;

...and these conditional on rem being non-NULL. No need to do this if we
don't need the result of these calls.

I tired to implement it similar to the manual, according to the manual, -1 is returned if the call is

interrupted by a signal handler.

If we decide to measure the time only if rem is non NULL, then the function could not return -1 if rem is NULL

and the thread was waken up by another thread.

Dafna

> +
> + if (diff < nsec) {
> + if (rem) {

And this could then be changed to "if (rem && (diff < nsec)).

> + rem->tv_sec = ukarch_time_nsec_to_sec(nsec-diff);
> + rem->tv_nsec = ukarch_time_subsec(nsec-diff);
> + }
> + return -1;
> }
> return 0;
> }
>

Otherwise, looking good.

Thanks,
Florian

--
Dr. Florian Schmidt
フローリアン・シュミット
Research Scientist,
Systems and Machine Learning Group
NEC Laboratories Europe
Kurfürsten-Anlage 36, D-69115 Heidelberg
Tel. +49 (0)6221 4342-265
Fax: +49 (0)6221 4342-155
e-mail: florian.schmidt@xxxxxxxxx
============================================================
Registered at Amtsgericht Mannheim, Germany, HRB728558

Re: [Minios-devel] [UNIKRAFT PATCH] lib/nolibc: Fix remaining time in nanosleep