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 -- 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/CAJfkoWp67rza_LHC_FABPobhsfdr6-tFqShW3xjie015bmR6tg%40mail.gmail.com.
