[2/6] linux: Assume clock_getres CLOCK_{PROCESS,THREAD}_CPUTIME_ID
Commit Message
On 11/02/2019 11:22, Carlos O'Donell wrote:
> On 2/11/19 7:54 AM, Florian Weimer wrote:
>> * Adhemerval Zanella:
>>
>>> On 11/02/2019 09:43, Florian Weimer wrote:
>>>> * Adhemerval Zanella:
>>>>
>>>>> nscd uses _SC_MONOTONIC_CLOCK and if I understood correctly both IA64
>>>>> manual and kernel code, the CLOCK_MONOTONIC implementation on SMP IA64
>>>>> system with ITC that may drift is a best effort...
>>>>
>>>> Are you sure about that? Wouldn't that be a bug in kernel
>>>> clock_gettime, and not something we should paper over in userspace?
>>>
>>> Not sure, I was just my impression checking kernel ia64 code but I am
>>> not an architecture expert. To be safe I kept the original code in
>>> patch proposal.
>>
>> I think it's not worth maintaing the ia64 divergence for this. If it is
>> a problem with the kernel's CLOCK_MONOTONIC implementation, it has to be
>> fixed there.
>
> I tend to agree. Standard interfaces should behave in standard ways.
>
> We have the same problem in HPPA where a similar clock (and my feeling was
> always that IA64 derived a lot from HP PARISC 64-bit) that needs synchronization
> at SMP rendevous at boot time, followed by bias and drift adjustment.
>
> For HPPA we made sue that the everything was accurately compensated at the
> kernel timer source level, and never in userspace. You *can* do it in userspace
> and it's faster and lower latency, and all of those things, but it's more complex
> and prone to breakage for odd reasons (compiler optimizations etc.).
>
> For HPPA you can access the tick counter in userspace, raw, and it's
> useful for timing small sequences of code, but if you move to a new CPU
> mid-measurement, you'll get wrong values because the value and bias is per-cpu.
>
Simplified patch below. I decided to keep the syscall issue so kernel can
correctly report its supports.
* sysdeps/unix/sysv/linux/ia64/has_cpuclock.c: Remove file.
* sysdeps/unix/sysv/linux/ia64/sysconf.c: Likewise.
* sysdeps/unix/sysv/linux/sysconf.c (has_cpuclock): Remove function.
(check_clock_getres): New function.
(__sysconf): Use check_clock_getres instead of has_cpuclock.
---
sysdeps/unix/sysv/linux/ia64/has_cpuclock.c | 51 ---------------------
sysdeps/unix/sysv/linux/ia64/sysconf.c | 30 ------------
sysdeps/unix/sysv/linux/sysconf.c | 45 +++++-------------
3 files changed, 11 insertions(+), 115 deletions(-)
delete mode 100644 sysdeps/unix/sysv/linux/ia64/has_cpuclock.c
delete mode 100644 sysdeps/unix/sysv/linux/ia64/sysconf.c
Comments
* Adhemerval Zanella:
> On 11/02/2019 11:22, Carlos O'Donell wrote:
>> On 2/11/19 7:54 AM, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> On 11/02/2019 09:43, Florian Weimer wrote:
>>>>> * Adhemerval Zanella:
>>>>>
>>>>>> nscd uses _SC_MONOTONIC_CLOCK and if I understood correctly both IA64
>>>>>> manual and kernel code, the CLOCK_MONOTONIC implementation on SMP IA64
>>>>>> system with ITC that may drift is a best effort...
>>>>>
>>>>> Are you sure about that? Wouldn't that be a bug in kernel
>>>>> clock_gettime, and not something we should paper over in userspace?
>>>>
>>>> Not sure, I was just my impression checking kernel ia64 code but I am
>>>> not an architecture expert. To be safe I kept the original code in
>>>> patch proposal.
>>>
>>> I think it's not worth maintaing the ia64 divergence for this. If it is
>>> a problem with the kernel's CLOCK_MONOTONIC implementation, it has to be
>>> fixed there.
>>
>> I tend to agree. Standard interfaces should behave in standard ways.
>>
>> We have the same problem in HPPA where a similar clock (and my feeling was
>> always that IA64 derived a lot from HP PARISC 64-bit) that needs synchronization
>> at SMP rendevous at boot time, followed by bias and drift adjustment.
>>
>> For HPPA we made sue that the everything was accurately compensated at the
>> kernel timer source level, and never in userspace. You *can* do it in userspace
>> and it's faster and lower latency, and all of those things, but it's more complex
>> and prone to breakage for odd reasons (compiler optimizations etc.).
>>
>> For HPPA you can access the tick counter in userspace, raw, and it's
>> useful for timing small sequences of code, but if you move to a new CPU
>> mid-measurement, you'll get wrong values because the value and bias is per-cpu.
>>
>
> Simplified patch below. I decided to keep the syscall issue so kernel can
> correctly report its supports.
>
> * sysdeps/unix/sysv/linux/ia64/has_cpuclock.c: Remove file.
> * sysdeps/unix/sysv/linux/ia64/sysconf.c: Likewise.
> * sysdeps/unix/sysv/linux/sysconf.c (has_cpuclock): Remove function.
> (check_clock_getres): New function.
> (__sysconf): Use check_clock_getres instead of has_cpuclock.
__sysconf now has:
case _SC_MONOTONIC_CLOCK:
return check_clock_getres (CLOCK_MONOTONIC);
case _SC_CPUTIME:
return check_clock_getres (CLOCK_PROCESS_CPUTIME_ID);
case _SC_THREAD_CPUTIME:
return check_clock_getres (CLOCK_THREAD_CPUTIME_ID);
I suggest to remove that blank line.
It may make sense to add a note to check_clock_getres that we use
INTERNAL_SYSCALL_CALL to avoid setting errno, so that the call can use
errno to check whether sysconf recognized the parameter constant at all.
Otherwise, looks good to me.
Thanks,
Florian
On 12/02/2019 11:17, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> On 11/02/2019 11:22, Carlos O'Donell wrote:
>>> On 2/11/19 7:54 AM, Florian Weimer wrote:
>>>> * Adhemerval Zanella:
>>>>
>>>>> On 11/02/2019 09:43, Florian Weimer wrote:
>>>>>> * Adhemerval Zanella:
>>>>>>
>>>>>>> nscd uses _SC_MONOTONIC_CLOCK and if I understood correctly both IA64
>>>>>>> manual and kernel code, the CLOCK_MONOTONIC implementation on SMP IA64
>>>>>>> system with ITC that may drift is a best effort...
>>>>>>
>>>>>> Are you sure about that? Wouldn't that be a bug in kernel
>>>>>> clock_gettime, and not something we should paper over in userspace?
>>>>>
>>>>> Not sure, I was just my impression checking kernel ia64 code but I am
>>>>> not an architecture expert. To be safe I kept the original code in
>>>>> patch proposal.
>>>>
>>>> I think it's not worth maintaing the ia64 divergence for this. If it is
>>>> a problem with the kernel's CLOCK_MONOTONIC implementation, it has to be
>>>> fixed there.
>>>
>>> I tend to agree. Standard interfaces should behave in standard ways.
>>>
>>> We have the same problem in HPPA where a similar clock (and my feeling was
>>> always that IA64 derived a lot from HP PARISC 64-bit) that needs synchronization
>>> at SMP rendevous at boot time, followed by bias and drift adjustment.
>>>
>>> For HPPA we made sue that the everything was accurately compensated at the
>>> kernel timer source level, and never in userspace. You *can* do it in userspace
>>> and it's faster and lower latency, and all of those things, but it's more complex
>>> and prone to breakage for odd reasons (compiler optimizations etc.).
>>>
>>> For HPPA you can access the tick counter in userspace, raw, and it's
>>> useful for timing small sequences of code, but if you move to a new CPU
>>> mid-measurement, you'll get wrong values because the value and bias is per-cpu.
>>>
>>
>> Simplified patch below. I decided to keep the syscall issue so kernel can
>> correctly report its supports.
>>
>> * sysdeps/unix/sysv/linux/ia64/has_cpuclock.c: Remove file.
>> * sysdeps/unix/sysv/linux/ia64/sysconf.c: Likewise.
>> * sysdeps/unix/sysv/linux/sysconf.c (has_cpuclock): Remove function.
>> (check_clock_getres): New function.
>> (__sysconf): Use check_clock_getres instead of has_cpuclock.
>
> __sysconf now has:
>
> case _SC_MONOTONIC_CLOCK:
> return check_clock_getres (CLOCK_MONOTONIC);
>
> case _SC_CPUTIME:
> return check_clock_getres (CLOCK_PROCESS_CPUTIME_ID);
> case _SC_THREAD_CPUTIME:
> return check_clock_getres (CLOCK_THREAD_CPUTIME_ID);
>
> I suggest to remove that blank line.
Ack, I changed it locally.
>
> It may make sense to add a note to check_clock_getres that we use
> INTERNAL_SYSCALL_CALL to avoid setting errno, so that the call can use
> errno to check whether sysconf recognized the parameter constant at all.
Ack, I added:
/* Avoid setting errno so it can check whether the kernel supports
the CLK_ID. */
As a side note I think we can also use INLINE_SYCALL_CALL and check for EINVAL.
>
> Otherwise, looks good to me.
>
> Thanks,
> Florian
>
* Adhemerval Zanella:
>> It may make sense to add a note to check_clock_getres that we use
>> INTERNAL_SYSCALL_CALL to avoid setting errno, so that the call can use
>> errno to check whether sysconf recognized the parameter constant at all.
>
> Ack, I added:
>
> /* Avoid setting errno so it can check whether the kernel supports
> the CLK_ID. */
>
> As a side note I think we can also use INLINE_SYCALL_CALL and check
> for EINVAL.
Sorry, what I meant is that the caller might want to set errno to 0
and check this way if sysconf actually recognizes the passed _SC_*
parameter.
At least that's why I think INLINE_SYCALL_CALL is used here.
On 12/02/2019 19:11, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>>> It may make sense to add a note to check_clock_getres that we use
>>> INTERNAL_SYSCALL_CALL to avoid setting errno, so that the call can use
>>> errno to check whether sysconf recognized the parameter constant at all.
>>
>> Ack, I added:
>>
>> /* Avoid setting errno so it can check whether the kernel supports
>> the CLK_ID. */
>>
>> As a side note I think we can also use INLINE_SYCALL_CALL and check
>> for EINVAL.
>
> Sorry, what I meant is that the caller might want to set errno to 0
> and check this way if sysconf actually recognizes the passed _SC_*
> parameter.
>
> At least that's why I think INLINE_SYCALL_CALL is used here.
>
Ah right, anyway I kept INTERNAL_SYSCALL_CALL.
deleted file mode 100644
@@ -1,51 +0,0 @@
-/* Copyright (C) 2000-2019 Free Software Foundation, Inc.
- This file is part of the GNU C Library.
-
- The GNU C Library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
- License as published by the Free Software Foundation; either
- version 2.1 of the License, or (at your option) any later version.
-
- The GNU C Library is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- Lesser General Public License for more details.
-
- You should have received a copy of the GNU Lesser General Public
- License along with the GNU C Library; if not, see
- <http://www.gnu.org/licenses/>. */
-
-#include <errno.h>
-#include <fcntl.h>
-#include <unistd.h>
-#include <sys/stat.h>
-#include <sys/types.h>
-#include <not-cancel.h>
-
-static int itc_usable;
-
-static int
-has_cpuclock (void)
-{
- if (__builtin_expect (itc_usable == 0, 0))
- {
- int newval = 1;
- int fd = __open_nocancel ("/proc/sal/itc_drift", O_RDONLY);
- if (__builtin_expect (fd != -1, 1))
- {
- char buf[16];
- /* We expect the file to contain a single digit followed by
- a newline. If the format changes we better not rely on
- the file content. */
- if (__read_nocancel (fd, buf, sizeof buf) != 2
- || buf[0] != '0' || buf[1] != '\n')
- newval = -1;
-
- __close_nocancel_nostatus (fd);
- }
-
- itc_usable = newval;
- }
-
- return itc_usable;
-}
deleted file mode 100644
@@ -1,30 +0,0 @@
-/* Get file-specific information about a file. Linux/ia64 version.
- Copyright (C) 2003-2019 Free Software Foundation, Inc.
- This file is part of the GNU C Library.
-
- The GNU C Library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
- License as published by the Free Software Foundation; either
- version 2.1 of the License, or (at your option) any later version.
-
- The GNU C Library is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- Lesser General Public License for more details.
-
- You should have received a copy of the GNU Lesser General Public
- License along with the GNU C Library; if not, see
- <http://www.gnu.org/licenses/>. */
-
-#include <assert.h>
-#include <stdbool.h>
-#include <stdlib.h>
-#include <unistd.h>
-
-
-#include "has_cpuclock.c"
-#define HAS_CPUCLOCK(name) (has_cpuclock () ? _POSIX_VERSION : -1)
-
-
-/* Now the generic Linux version. */
-#include <sysdeps/unix/sysv/linux/sysconf.c>
@@ -35,33 +35,16 @@
static long int posix_sysconf (int name);
-#ifndef HAS_CPUCLOCK
static long int
-has_cpuclock (int name)
+check_clock_getres (clockid_t clk_id)
{
-# if defined __NR_clock_getres || HP_TIMING_AVAIL
- /* If we have HP_TIMING, we will fall back on that if the system
- call does not work, so we support it either way. */
-# if !HP_TIMING_AVAIL
- /* Check using the clock_getres system call. */
struct timespec ts;
INTERNAL_SYSCALL_DECL (err);
- int r = INTERNAL_SYSCALL (clock_getres, err, 2,
- (name == _SC_CPUTIME
- ? CLOCK_PROCESS_CPUTIME_ID
- : CLOCK_THREAD_CPUTIME_ID),
- &ts);
+ int r = INTERNAL_SYSCALL_CALL (clock_getres, err, clk_id, &ts);
if (INTERNAL_SYSCALL_ERROR_P (r, err))
return -1;
-# endif
return _POSIX_VERSION;
-# else
- return -1;
-# endif
}
-# define HAS_CPUCLOCK(name) has_cpuclock (name)
-#endif
-
/* Get the value of the system variable NAME. */
long int
@@ -71,29 +54,22 @@ __sysconf (int name)
switch (name)
{
- struct rlimit rlimit;
-#ifdef __NR_clock_getres
case _SC_MONOTONIC_CLOCK:
- /* Check using the clock_getres system call. */
- {
- struct timespec ts;
- INTERNAL_SYSCALL_DECL (err);
- int r;
- r = INTERNAL_SYSCALL (clock_getres, err, 2, CLOCK_MONOTONIC, &ts);
- return INTERNAL_SYSCALL_ERROR_P (r, err) ? -1 : _POSIX_VERSION;
- }
-#endif
+ return check_clock_getres (CLOCK_MONOTONIC);
case _SC_CPUTIME:
+ return check_clock_getres (CLOCK_PROCESS_CPUTIME_ID);
case _SC_THREAD_CPUTIME:
- return HAS_CPUCLOCK (name);
+ return check_clock_getres (CLOCK_THREAD_CPUTIME_ID);
- case _SC_ARG_MAX:
+ case _SC_ARG_MAX: {
+ struct rlimit rlimit;
/* Use getrlimit to get the stack limit. */
if (__getrlimit (RLIMIT_STACK, &rlimit) == 0)
return MAX (legacy_ARG_MAX, rlimit.rlim_cur / 4);
return legacy_ARG_MAX;
+ } break;
case _SC_NGROUPS_MAX:
/* Try to read the information from the /proc/sys/kernel/ngroups_max
@@ -101,13 +77,14 @@ __sysconf (int name)
procfname = "/proc/sys/kernel/ngroups_max";
break;
- case _SC_SIGQUEUE_MAX:
+ case _SC_SIGQUEUE_MAX: {
+ struct rlimit rlimit;
if (__getrlimit (RLIMIT_SIGPENDING, &rlimit) == 0)
return rlimit.rlim_cur;
/* The /proc/sys/kernel/rtsig-max file contains the answer. */
procfname = "/proc/sys/kernel/rtsig-max";
- break;
+ } break;
default:
break;