Patches for running cyclictest on OSv

[Luca Abeni]

Hi all,

I attach the patches I am using to run a simple real-time test
(cyclictest) on OSv.
The patches are based on github issue 386
(https://github.com/cloudius-systems/osv/issues/386) and are a forward
port of patches by Nadav Har'El and Claudio Fontana; I only applied a
few changes to them.

With these patches, cyclictest can start and measure some latencies...
In the next few days, I'll run some more accurate tests, and I'll
report on the latencies introduced by OSv.


Luca

P.S.: please cc me if you reply because I am not subscribed to the mailing list

[Waldek Kozaczuk]

Hi,

Thanks for the patches.

Could you please follow https://github.com/cloudius-systems/osv/wiki/Formatting-and-sending-patches and submit them using `git send-email` so that they are easier to review? I think you can skip the 0001-* - we already have a github pull request - https://github.com/cloudius-systems/osv/pull/1223 - where I left some of my comments. Alternatively, you can create pull requests for 0002-* and 0003-*.

Thanks,

Waldemar Kozaczuk

--
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+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/CAJfkoWpFDkukjNb%3DR2kxugjo2YBqeMgpoCuM26QiVdwaB6k5pw%40mail.gmail.com.

[Nadav Har'El]

Hi, thanks!

It's hard for me to review the patches this way so next time please either send them with git send-email or as github pull requests.

In the mean time, I read your patches and have a few comments and will try to awkwardly write them here:

If I understand correctly, the first patch is the same as my https://github.com/cloudius-systems/osv/pull/1223.

In the second patch:

1. You change thread::set_realtime_priority to take a "policy" parameter. I don't think it's a good idea, thread::set_realtime_priority is the low-level function. If you need such a wrapper for some reason you can create it (it can call set_realtime_priority instead of changing it) but I'm not sure you even need such a wrapper. We don't need to split every 10-line function into 3 levels of function calls.

2. There is no reason I can see for adding #define __NEED_size_t in include/api/sched.hh

3. The include/api/sched.hh is supposed to be the Linux API, don't add to it random conveniences like SCHED_PRIO_MIN. It should be in a different header file, if at all (do you need it from several source files except the single source file implementing the sched_* functions?)

4. I would like to reconsider the file name libc/rt/sched.cc. Do we have a reason to expect we will have other files in the rt/ directory? Maybe we can just call it libc/sched.cc? Musl, by the way, has a libc/sched directory, and in it files for each individual function (as is customary in Musl).

In the third patch:

5. Again, please don't add convenience functions to include/api/sys/time.h - which is the Linux API. If you only need from from a single source file add them (if at all...) to that source file.

6. Please verify the changes to clock_gettime() and nonsleep() to return EINVAL when given a null pointer is the same in Linux (i.e., that it doesn't return EFAULT) in that case. It's a bit sad we need to waste time on these checks :-(

7. In clock_nanosleep(), please don't call WARN_STUBBED() when called in a *supported* case. I suggest to call UNIMPLEMENTED("some explanation of what's unimplemented") instead of "assert(0)" - only in the cases which you don't support. In the case we do properly support, there is no need to print a message at all.

--
Nadav Har'El

n...@scylladb.com

[Luca Abeni]

Hi,

sorry for the silence; I've been busy with other stuff...
In the next three days I'll be out of town, but on Thursday (or Friday
at the latest) I'll resend the patches with git send-email

Luca

[Luca Abeni]

Hi,

On Sun, 16 Apr 2023 at 18:18, Nadav Har'El <n...@scylladb.com> wrote:
[...]

> It's hard for me to review the patches this way so next time please either send them with git send-email or as github pull requests.

Sorry; I'll resend with git send-email when I get some time.

> In the mean time, I read your patches and have a few comments and will try to awkwardly write them here:
>
> If I understand correctly, the first patch is the same as my https://github.com/cloudius-systems/osv/pull/1223.

Yes, I used your patch without changes

> In the second patch:
>
> 1. You change thread::set_realtime_priority to take a "policy" parameter. I don't think it's a good idea, thread::set_realtime_priority is the low-level function. If you need such a wrapper for some reason you can create it (it can call set_realtime_priority instead of changing it) but I'm not sure you even need such a wrapper. We don't need to split every 10-line function into 3 levels of function calls.

Sorry, I do not know much about OSv internals... I got Claudio's patch
and I tried to forward-port it to the current OSv version; I saw that
the original patch used set_realtime(), while the current code uses
set_realtime_priority() so I tried to adapt the patch to modify
set_realtime().
I'll try to rewrite this part of the patch as you suggest.

> 2. There is no reason I can see for adding #define __NEED_size_t in include/api/sched.hh

I think this comes from Claudio's original patch; I'll remove it anyway

> 3. The include/api/sched.hh is supposed to be the Linux API, don't add to it random conveniences like SCHED_PRIO_MIN. It should be in a different header file, if at all (do you need it from several source files except the single source file implementing the sched_* functions?)

I do not know... Again, I think these defines come from the original
patch by Claudio... I'll remove them, and use the "1" and "99" values
directly

> 4. I would like to reconsider the file name libc/rt/sched.cc. Do we have a reason to expect we will have other files in the rt/ directory? Maybe we can just call it libc/sched.cc? Musl, by the way, has a libc/sched directory, and in it files for each individual function (as is customary in Musl).

Same as above; I'll try to make these changes. The same applies to item 5 too.

> 6. Please verify the changes to clock_gettime() and nonsleep() to return EINVAL when given a null pointer is the same in Linux (i.e., that it doesn't return EFAULT) in that case. It's a bit sad we need to waste time on these checks :-(

I just tested, and on Linux a program passing NULL to clock_gettime()
just segfaults... So maybe these checks could be really avoided :)
What is the policy in this case? Should I just remove the checks?

> 7. In clock_nanosleep(), please don't call WARN_STUBBED() when called in a *supported* case. I suggest to call UNIMPLEMENTED("some explanation of what's unimplemented") instead of "assert(0)" - only in the cases which you don't support. In the case we do properly support, there is no need to print a message at all.

This WARN_STUBBED() is something I copied from the original patch...
After posting the patches, I moved it in the "default:" case, because
it was increasing the measured latency. I'll look at UNIMPLEMENTED()
and I'll try removing the "assert(0)"


Luca

[Nadav Har'El]

On Mon, Apr 17, 2023 at 12:30 AM Luca Abeni <luca...@gmail.com> wrote:

Hi,

On Sun, 16 Apr 2023 at 18:18, Nadav Har'El <n...@scylladb.com> wrote:
[...]
> It's hard for me to review the patches this way so next time please either send them with git send-email or as github pull requests.

Sorry; I'll resend with git send-email when I get some time.

Thanks. I appreciate this (and your work on this issue).

 

> In the second patch:
>
> 1. You change thread::set_realtime_priority to take a "policy" parameter. I don't think it's a good idea, thread::set_realtime_priority is the low-level function. If you need such a wrapper for some reason you can create it (it can call set_realtime_priority instead of changing it) but I'm not sure you even need such a wrapper. We don't need to split every 10-line function into 3 levels of function calls.

Sorry, I do not know much about OSv internals... I got Claudio's patch
and I tried to forward-port it to the current OSv version; I saw that
the original patch used set_realtime(), while the current code uses
set_realtime_priority() so I tried to adapt the patch to modify
set_realtime().
I'll try to rewrite this part of the patch as you suggest.

Thanks. I understand now a lot of my comments are also relevant to Claudio's old patches,

which I don't think I ever reviewed (?). It's fine. I don't think there was anything wrong with

those patches per-se, I just thought they could be improved.

 

> 3. The include/api/sched.hh is supposed to be the Linux API, don't add to it random conveniences like SCHED_PRIO_MIN. It should be in a different header file, if at all (do you need it from several source files except the single source file implementing the sched_* functions?)

I do not know... Again, I think these defines come from the original
patch by Claudio... I'll remove them, and use the "1" and "99" values
directly

If you need these numbers multiple times in one source file (sched.cc), you can also define them in sched.cc -

you don't need to repeat "99" five times in one file, and you don't need to put stuff in a header file (let alone

the wrong header file) to avoid that.

 

> 6. Please verify the changes to clock_gettime() and nonsleep() to return EINVAL when given a null pointer is the same in Linux (i.e., that it doesn't return EFAULT) in that case. It's a bit sad we need to waste time on these checks :-(

I just tested, and on Linux a program passing NULL to clock_gettime()
just segfaults...

Interesting. Maybe even though clock_gettime() is in section 2 of the manual, it isn't a real system call

and is a C wrapper - which segfaults? Real system calls are supposed to return EFAULT, not segfault.

 

So maybe these checks could be really avoided :)
What is the policy in this case? Should I just remove the checks?

The best policy is to do something similar to what Linux does. If Linux doesn't protect the application

in this case, you don't need to either - it just slows down this function with no real benefit.

If (hypothetically) Linux does check this case and returns EFAULT, you should return that and not EINVAL.

I personally think that part of the "OSv uses the same address space for kernel and user space" philosophy

is that we shouldn't ever worry about EFAULT - if a user passes a broken pointer to a system function, they

will get a segfault, I think that's fine and expected.

By the way, IIRC in Linux (I didn't try this in OSv!), you can mmap() the address 0 (remember to use MAP_FIXED),

and then you should be able to give the pointer 0 to system calls as a pointer 0. It's a silly case that nobody will

ever use, but it's possible :-)

 

> 7. In clock_nanosleep(), please don't call WARN_STUBBED() when called in a *supported* case. I suggest to call UNIMPLEMENTED("some explanation of what's unimplemented") instead of "assert(0)" - only in the cases which you don't support. In the case we do properly support, there is no need to print a message at all.

This WARN_STUBBED() is something I copied from the original patch...
After posting the patches, I moved it in the "default:" case, because
it was increasing the measured latency. I'll look at UNIMPLEMENTED()
and I'll try removing the "assert(0)"

include/osv/stubbing.hh has those different functions. UNIMPLEMENTED(msg)

simply prints this message and aborts the kernel - so is the  same as your assert(0)

but gives a clearer message. But the idea is to call it only when you reach a case

that we really didn't implement. The case that we did implement (i.e., monotonic

clock with no remain) will run normally without any messages.

[Luca Abeni]

Hi again,

first of all, sorry for the very long delay... Some issues at work
distracted me and I did not find time to work on these patches.
Anyway, I am (slowly) catching up.

Here is a small update (I'll send more emails, and maybe some updated
patches, later)

On Sun, 16 Apr 2023 at 23:29, Luca Abeni <luca...@gmail.com> wrote:
[...]

> > 2. There is no reason I can see for adding #define __NEED_size_t in include/api/sched.hh
>
> I think this comes from Claudio's original patch; I'll remove it anyway

I investigated this, and I now have some more information...

include/api/sched.hh contains definitions like
int __sched_cpucount (size_t __setsize, const cpu_set_t *__setp)
that need time_t.

Since the new "libc/rt/sched.cc" file includes api/sched.hh, the build
fails. I see that the only other file including api/sched.hh is
libc/pthread.cc, which includes osv/sched.hh before api/sched.hh...
And osv/sched.hh probably ends up defining __NEED_size_t; this is why
the #define was not needed before this patch.

Now, I do not know if including api/sched.hh without having included
osv/sched.hh is correct... If it is, then the #define is needed (of
course, not in this patch... I'll separate the change in a different
patch, with a proper commit message); if it is not, then I'll add an
"#include <osv/sched.hh>" in the new sched.cc file.

Let me know what is the correct thing to do here...


Thanks,
Luca

[Luca Abeni]

Hi again,

On Mon, 17 Apr 2023 at 10:30, Nadav Har'El <n...@scylladb.com> wrote:
[...]

>> > 6. Please verify the changes to clock_gettime() and nonsleep() to return EINVAL when given a null pointer is the same in Linux (i.e., that it doesn't return EFAULT) in that case. It's a bit sad we need to waste time on these checks :-(
>>
>> I just tested, and on Linux a program passing NULL to clock_gettime()
>> just segfaults...
>
>
> Interesting. Maybe even though clock_gettime() is in section 2 of the manual, it isn't a real system call
> and is a C wrapper - which segfaults? Real system calls are supposed to return EFAULT, not segfault.

I verified this happens because clock_gettime() is a vsyscall
implemented through the vDSO. This is the backtrace:
Program received signal SIGSEGV, Segmentation fault.
do_hres (ts=<optimized out>, clk=<optimized out>, vd=<optimized out>)
at /build/linux-lowlatency-UVnJHF/linux-lowlatency-6.2.0/include/vdso/math64.h:21
Downloading source file
/build/linux-lowlatency-UVnJHF/linux-lowlatency-6.2.0/include/vdso/math64.h
21 /build/linux-lowlatency-UVnJHF/linux-lowlatency-6.2.0/include/vdso/math64.h:
Directory not empty.
(gdb) bt
#0 do_hres (ts=<optimized out>, clk=<optimized out>, vd=<optimized out>)
at /build/linux-lowlatency-UVnJHF/linux-lowlatency-6.2.0/include/vdso/math64.h:21
#1 __cvdso_clock_gettime_common (ts=0x0, clock=1, vd=<optimized out>)
at /build/linux-lowlatency-UVnJHF/linux-lowlatency-6.2.0/arch/x86/entry/vdso/../../../../lib/vdso/gettimeofday.c:251
#2 __cvdso_clock_gettime_data (clock=1, ts=0x0, vd=<optimized out>)
at /build/linux-lowlatency-UVnJHF/linux-lowlatency-6.2.0/arch/x86/entry/vdso/../../../../lib/vdso/gettimeofday.c:258
#3 0x0000000000421f66 in clock_gettime ()
#4 0x0000000000401791 in test (ts=0x0) at gettime_test.c:11
#5 0x0000000000401802 in main () at gettime_test.c:23

And the kernel documentation explains that
"
...if you pass a bad pointer to a vDSO function, you might get SIGSEGV
instead of -EFAULT.
"


So, I guess we can remove the NULL check :)

Luca

[Waldek Kozaczuk]

Luca,

I am not sure you have seen newly updated PR by Nadav that adds low level support - 

https://github.com/cloudius-systems/osv/pull/1223. You may want to use it update your branch. 

I have reviewed his PR and it looks good to me. But ideally I want to run more tests on my side and I would like you to run some tests on your side that involve the high level code before I merge this PR into master. It obviously touches very critical part of the kernel - thread scheduler. 

Regards and thanks for your work Luca. 

Waldek

--
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+u...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/CAJfkoWq7HqAv1drjf54vr0buwfavY3mLT%2BUQfFrfLTntNXcg5w%40mail.gmail.com.

[Luca Abeni]

Hi,

On Wed, 10 May 2023 at 02:34, Waldek Kozaczuk <jwkoz...@gmail.com> wrote:
>
> Luca,
>
> I am not sure you have seen newly updated PR by Nadav that adds low level support -
> https://github.com/cloudius-systems/osv/pull/1223. You may want to use it update your branch.

Sorry, I did not see the update... I am going to look at it right now.
BTW, I have a new version of the patches almost ready, but I still do
not understand what to do about __NEED_size_t; probably I will chose
one of the possible solution and I'll send an updated patchset today,
after looking at Nadav's updated patch.


Thanks,
Luca

[Nadav Har'El]

On Wed, May 3, 2023 at 7:40 PM Luca Abeni <luca...@gmail.com> wrote:

On Sun, 16 Apr 2023 at 23:29, Luca Abeni <luca...@gmail.com> wrote:
[...]
> > 2. There is no reason I can see for adding #define __NEED_size_t in include/api/sched.hh
>
> I think this comes from Claudio's original patch; I'll remove it anyway

I investigated this, and I now have some more information...

include/api/sched.hh contains definitions like
       int __sched_cpucount (size_t __setsize, const cpu_set_t *__setp)
that need time_t.

Since the new "libc/rt/sched.cc" file includes api/sched.hh, the build
fails. I see that the only other file including api/sched.hh is
libc/pthread.cc, which includes osv/sched.hh before api/sched.hh...
And osv/sched.hh probably ends up defining __NEED_size_t; this is why
the #define was not needed before this patch.

Ok, I understand now. You're right. I forgot how we do these things.

 

[Nadav Har'El]

On Wed, May 10, 2023 at 9:20 AM Luca Abeni <luca...@gmail.com> wrote:

Hi,

On Wed, 10 May 2023 at 02:34, Waldek Kozaczuk <jwkoz...@gmail.com> wrote:
>
> Luca,
>
> I am not sure you have seen newly updated PR by Nadav that adds low level support -
> https://github.com/cloudius-systems/osv/pull/1223. You may want to use it update your branch.

Sorry, I did not see the update... I am going to look at it right now.
BTW, I have a new version of the patches almost ready, but I still do
not understand what to do about __NEED_size_t; probably I will chose
one of the possible solution and I'll send an updated patchset today,
after looking at Nadav's updated patch.

I think I misunderstood / misremembered how we use __NEED_size_t, and withdraw my objection there.

[Luca Abeni]

Hi,

I just sent a new version of the patchset (at least, I hope I sent
it... Using git-send-email with gmail is not fun :).
I tried to address all your previous comments (I hope I did not
misunderstand anything).

Thanks,
Luca

[Nadav Har'El]

On Wed, May 10, 2023 at 12:02 PM Luca Abeni <luca...@gmail.com> wrote:

Hi,

I just sent a new version of the patchset (at least, I hope I sent
it... Using git-send-email with gmail is not fun :).

You can also do a github pull request, should be more fun :-)

[Luca Abeni]

Eh... Another new thing I need to learn... :)


Luca

[Nadav Har'El]

Yes, but arguably github pull requests is a slightly more "fashionable" skill to learn than

git-send-email. And both aren't very difficult to learn - it's not like learning quantum field theory ;-)

In any case, both ways are perfectly fine, and each allows me to do reviews in a reasonable way,

so thanks.