| Re: [chrony-dev] [PATCH] sys_posix: support SCHED_FIFO and mlockall on more OSs |
[ Thread Index |
Date Index
| More chrony.tuxfamily.org/chrony-dev Archives
]
On Sat, Apr 13, 2019 at 10:06:09AM -0400, Stefan R. Filipek wrote:
> The following patch centralizes the memory locking and real-time scheduling
> for POSIX systems. It brings support for these features to FreeBSD, NetBSD,
> and Solaris. Note that MacOS does support the pthread_setschedparam() API,
> but it does not function as intended for real-time scheduling. As such,
> MacOS was not included in the patch.
Great. Thanks for working on this.
> Also note that the memory locking behavior changed ever-so-slightly.
> Solaris does not support setrlimit(RLIMIT_MEMLOCK, ...), so this was turned
> into an optional call and an additional configure test. It will no longer
> prevent memory locking, even if the call fails.
Could that not lead into situations where chronyd randomly fails later
after start when it needs to allocate more memory? I'd rather always
fail deterministically on start. If setrlimit is needed on Linux or
other systems to avoid that, mlockall should not be used alone.
> I tested this patch using Linux 4.18 with glibc, FreeBSD 11.2 & 12, NetBSD
> 8, and Solaris 11.
Nice.
> The documentation needs to be updated for the new support as well. I can
> submit this as a separate patch or combine it into one. Your choice.
I'd slightly prefer the latter, but it doesn't really matter as long
as the documentation is updated.
As for sending the patches, have you tried git send-email? That should
avoid problems with formatting.
Some comments below.
> --- a/configure
> +++ b/configure
> @@ -405,14 +405,18 @@ case $OPERATINGSYSTEM in
> try_lockmem=1
> try_phc=1
> add_def LINUX
> + add_def POSIX
I don't like this definition very much. All supported systems follow
POSIX to some extent, so not defining it for macOS would be confusing.
If some code needs to be conditionally enabled/disabled in this patch,
I'd just check for MACOSX, at least for now.
> diff --git a/sys_posix.c b/sys_posix.c
> new file mode 100644
> index 0000000..77720c3
> --- /dev/null
> +++ b/sys_posix.c
> +
> + This is the module is for POSIX compliant operating systems.
An extra "is"?
As the code is moving between files, it's a good opportunity to update
the coding style to match the majority of the code. I could do that
before commiting your patch if you prefer.
> +#if defined(HAVE_PTHREAD_SETSCHEDPARAM)
> +# include <pthread.h>
> +# include <sched.h>
No indentation here.
> + } else {
> + sched.sched_priority = SchedPriority;
> + pmax = sched_get_priority_max(SCHED_FIFO);
> + pmin = sched_get_priority_min(SCHED_FIFO);
> + if ( SchedPriority > pmax ) {
No spaces after "(" and before ")".
> + }
> + else {
"} else {" on one line
> +/* ================================================== */
> +
> +#if defined(HAVE_MLOCKALL)
> +/* Lock the process into RAM so that it will never be swapped out */
> +void SYS_Posix_MemLockAll(int LockAll)
Return type on separate line. Parameter names using lower case
(e.g. lock_all, sched_priority).
> +{
> + if (LockAll == 1 ) {
> +#if defined(HAVE_SETRLIMIT_MEMLOCK)
> + // Try to reserve as much as we can
C-style comments (/* */) should be used.
> + struct rlimit rlim;
> + rlim.rlim_max = RLIM_INFINITY;
Empty line after declarations.
--
Miroslav Lichvar
--
To unsubscribe email chrony-dev-request@xxxxxxxxxxxxxxxxxxxx with "unsubscribe" in the subject.
For help email chrony-dev-request@xxxxxxxxxxxxxxxxxxxx with "help" in the subject.
Trouble? Email listmaster@xxxxxxxxxxxxxxxxxxxx.