sysconf: Use conservative default for _SC_NPROCESSORS_ONLN [BZ #21542]
Commit Message
2017-06-02 Florian Weimer <fweimer@redhat.com>
[BZ #21542]
* sysdeps/unix/sysv/linux/getsysstats.c (__get_nprocs): Default to
two if no accurate information is available.
* posix/tst-sysconf-empty-chroot.c: New file.
* posix/Makefile (tests): Add it.
Comments
On 06/02/2017 04:21 PM, Florian Weimer wrote:
> 2017-06-02 Florian Weimer <fweimer@redhat.com>
>
> [BZ #21542]
> * sysdeps/unix/sysv/linux/getsysstats.c (__get_nprocs): Default to
> two if no accurate information is available.
> * posix/tst-sysconf-empty-chroot.c: New file.
> * posix/Makefile (tests): Add it.
Ping?
<https://sourceware.org/ml/libc-alpha/2017-06/msg00101.html>
Thanks,
Florian
On Fri, Jun 9, 2017 at 8:11 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 06/02/2017 04:21 PM, Florian Weimer wrote:
>> 2017-06-02 Florian Weimer <fweimer@redhat.com>
>>
>> [BZ #21542]
>> * sysdeps/unix/sysv/linux/getsysstats.c (__get_nprocs): Default to
>> two if no accurate information is available.
>> * posix/tst-sysconf-empty-chroot.c: New file.
>> * posix/Makefile (tests): Add it.
>
> Ping?
>
> <https://sourceware.org/ml/libc-alpha/2017-06/msg00101.html>
It seems reasonable to me but I'm not sure I understand all of the implications.
On 06/09/2017 02:17 PM, Zack Weinberg wrote:
> On Fri, Jun 9, 2017 at 8:11 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 06/02/2017 04:21 PM, Florian Weimer wrote:
>>> 2017-06-02 Florian Weimer <fweimer@redhat.com>
>>>
>>> [BZ #21542]
>>> * sysdeps/unix/sysv/linux/getsysstats.c (__get_nprocs): Default to
>>> two if no accurate information is available.
>>> * posix/tst-sysconf-empty-chroot.c: New file.
>>> * posix/Makefile (tests): Add it.
>>
>> Ping?
>>
>> <https://sourceware.org/ml/libc-alpha/2017-06/msg00101.html>
>
> It seems reasonable to me but I'm not sure I understand all of the implications.
The implication is that the Hotspot JVM no longer crashes in a chroot on
an SMP host because it assumes a UP host and elides atomics.
The downside is that some other application could choose to use
spinlocks on a UP system because glibc suggests it's SMP, which could
result in terrible performance.
All this is just for the corner case of an improperly setup
chroot/contain (lacking key parts of /sys and /proc).
Thanks,
Florian
On Fri, Jun 9, 2017 at 5:22 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 06/09/2017 02:17 PM, Zack Weinberg wrote:
>> On Fri, Jun 9, 2017 at 8:11 AM, Florian Weimer <fweimer@redhat.com> wrote:
>>> On 06/02/2017 04:21 PM, Florian Weimer wrote:
>>>> 2017-06-02 Florian Weimer <fweimer@redhat.com>
>>>>
>>>> [BZ #21542]
>>>> * sysdeps/unix/sysv/linux/getsysstats.c (__get_nprocs): Default to
>>>> two if no accurate information is available.
>>>> * posix/tst-sysconf-empty-chroot.c: New file.
>>>> * posix/Makefile (tests): Add it.
>>>
>>> Ping?
>>>
>>> <https://sourceware.org/ml/libc-alpha/2017-06/msg00101.html>
>>
>> It seems reasonable to me but I'm not sure I understand all of the implications.
>
> The implication is that the Hotspot JVM no longer crashes in a chroot on
> an SMP host because it assumes a UP host and elides atomics.
>
> The downside is that some other application could choose to use
> spinlocks on a UP system because glibc suggests it's SMP, which could
> result in terrible performance.
>
> All this is just for the corner case of an improperly setup
> chroot/contain (lacking key parts of /sys and /proc).
>
On x86, we compute the number of threads which share L3
cache in a package via CPUID. We should use this info as
default on x86.
On 06/09/2017 02:26 PM, H.J. Lu wrote:
> On x86, we compute the number of threads which share L3
> cache in a package via CPUID. We should use this info as
> default on x86.
That would only allow us to return a larger value than 2 in this case,
wouldn't it?
I don't know if the additional complexity is worth it. The 1/2
distinction is what patterns, I don't think it's possible to get this
data reliably from CPUID.
Thanks,
Florian
On Fri, Jun 9, 2017 at 5:28 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 06/09/2017 02:26 PM, H.J. Lu wrote:
>> On x86, we compute the number of threads which share L3
>> cache in a package via CPUID. We should use this info as
>> default on x86.
>
> That would only allow us to return a larger value than 2 in this case,
> wouldn't it?
>
> I don't know if the additional complexity is worth it. The 1/2
The information is there. We just need to store the result in
cpu_features and return it in __default_nprocs ().
> distinction is what patterns, I don't think it's possible to get this
> data reliably from CPUID.
No. Reliable data needs ACPI table.
On 06/09/2017 02:32 PM, H.J. Lu wrote:
> On Fri, Jun 9, 2017 at 5:28 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 06/09/2017 02:26 PM, H.J. Lu wrote:
>>> On x86, we compute the number of threads which share L3
>>> cache in a package via CPUID. We should use this info as
>>> default on x86.
>>
>> That would only allow us to return a larger value than 2 in this case,
>> wouldn't it?
>>
>> I don't know if the additional complexity is worth it. The 1/2
>
> The information is there. We just need to store the result in
> cpu_features and return it in __default_nprocs ().
It still needs another sysdeps override for __default_nprocs (). This
is what I meant.
>> distinction is what patterns, I don't think it's possible to get this
>> data reliably from CPUID.
>
> No. Reliable data needs ACPI table.
Exactly. A single-core, single-thread processor might still be running
in a multi-socket configuration. And with virtualization, it's not safe
to list CPU names, either.
Florian
On Fri, Jun 9, 2017 at 5:35 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 06/09/2017 02:32 PM, H.J. Lu wrote:
>> On Fri, Jun 9, 2017 at 5:28 AM, Florian Weimer <fweimer@redhat.com> wrote:
>>> On 06/09/2017 02:26 PM, H.J. Lu wrote:
>>>> On x86, we compute the number of threads which share L3
>>>> cache in a package via CPUID. We should use this info as
>>>> default on x86.
>>>
>>> That would only allow us to return a larger value than 2 in this case,
>>> wouldn't it?
>>>
>>> I don't know if the additional complexity is worth it. The 1/2
>>
>> The information is there. We just need to store the result in
>> cpu_features and return it in __default_nprocs ().
>
> It still needs another sysdeps override for __default_nprocs (). This
> is what I meant.
>
>>> distinction is what patterns, I don't think it's possible to get this
>>> data reliably from CPUID.
>>
>> No. Reliable data needs ACPI table.
>
> Exactly. A single-core, single-thread processor might still be running
> in a multi-socket configuration. And with virtualization, it's not safe
> to list CPU names, either.
>
> Florian
Just a suggestion. I have no strong opinion.
"H.J. Lu" <hjl.tools@gmail.com> writes:
> On Fri, Jun 9, 2017 at 5:35 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 06/09/2017 02:32 PM, H.J. Lu wrote:
>>> On Fri, Jun 9, 2017 at 5:28 AM, Florian Weimer <fweimer@redhat.com> wrote:
>>>> On 06/09/2017 02:26 PM, H.J. Lu wrote:
>>>>> On x86, we compute the number of threads which share L3
>>>>> cache in a package via CPUID. We should use this info as
>>>>> default on x86.
>>>>
>>>> That would only allow us to return a larger value than 2 in this case,
>>>> wouldn't it?
>>>>
>>>> I don't know if the additional complexity is worth it. The 1/2
>>>
>>> The information is there. We just need to store the result in
>>> cpu_features and return it in __default_nprocs ().
>>
>> It still needs another sysdeps override for __default_nprocs (). This
>> is what I meant.
> Just a suggestion. I have no strong opinion.
Okay, in this case, I will wait a bit for other comments and check in
what I have eventually.
Thanks,
Florian
On 06/02/2017 04:21 PM, Florian Weimer wrote:
> 2017-06-02 Florian Weimer <fweimer@redhat.com>
>
> [BZ #21542]
> * sysdeps/unix/sysv/linux/getsysstats.c (__get_nprocs): Default to
> two if no accurate information is available.
> * posix/tst-sysconf-empty-chroot.c: New file.
> * posix/Makefile (tests): Add it.
Any further comments? I'll commit this next week otherwise.
Thanks,
Florian
@@ -92,7 +92,8 @@ tests := test-errno tstgetopt testfnm runtests runptests \
tst-pathconf tst-getaddrinfo4 tst-rxspencer-no-utf8 \
tst-fnmatch3 bug-regex36 tst-getaddrinfo5 \
tst-posix_spawn-fd tst-posix_spawn-setsid \
- tst-posix_fadvise tst-posix_fadvise64
+ tst-posix_fadvise tst-posix_fadvise64 \
+ tst-sysconf-empty-chroot
tests-internal := bug-regex5 bug-regex20 bug-regex33 \
tst-rfc3484 tst-rfc3484-2 tst-rfc3484-3
xtests := bug-ga2
new file mode 100644
@@ -0,0 +1,95 @@
+/* Test sysconf with an empty chroot.
+ Copyright (C) 2017 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 <stdio.h>
+#include <stdlib.h>
+#include <support/check.h>
+#include <support/namespace.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/test-driver.h>
+#include <support/xunistd.h>
+#include <unistd.h>
+
+/* Check for an SMP system in a forked process, so that the parent
+ process does not cache the value. */
+static void
+is_smp_callback (void *closure)
+{
+ bool *result = closure;
+
+ long cpus = sysconf (_SC_NPROCESSORS_ONLN);
+ TEST_VERIFY_EXIT (cpus > 0);
+ *result = cpus != 1;
+}
+
+static bool
+is_smp (void)
+{
+ bool *result = support_shared_allocate (sizeof (*result));
+ support_isolate_in_subprocess (is_smp_callback, result);
+ bool result_copy = *result;
+ support_shared_free (result);
+ return result_copy;
+}
+
+static char *path_chroot;
+
+/* Prepare an empty directory, to be used as a chroot. */
+static void
+prepare (int argc, char **argv)
+{
+ path_chroot = xasprintf ("%s/tst-resolv-res_init-XXXXXX", test_dir);
+ if (mkdtemp (path_chroot) == NULL)
+ FAIL_EXIT1 ("mkdtemp (\"%s\"): %m", path_chroot);
+ add_temp_file (path_chroot);
+}
+
+/* The actual test. Run it in a subprocess, so that the test harness
+ can remove the temporary directory in --direct mode. */
+static void
+chroot_callback (void *closure)
+{
+ xchroot (path_chroot);
+ long cpus = sysconf (_SC_NPROCESSORS_ONLN);
+ printf ("info: sysconf (_SC_NPROCESSORS_ONLN) in chroot: %ld\n", cpus);
+ TEST_VERIFY (cpus > 0);
+ TEST_VERIFY (cpus != 1);
+ _exit (0);
+}
+
+static int
+do_test (void)
+{
+ if (!is_smp ())
+ {
+ printf ("warning: test not supported on uniprocessor system\n");
+ return EXIT_UNSUPPORTED;
+ }
+
+ support_become_root ();
+ if (!support_can_chroot ())
+ return EXIT_UNSUPPORTED;
+
+ support_isolate_in_subprocess (chroot_callback, NULL);
+
+ return 0;
+}
+
+#define PREPARE prepare
+#include <support/test-driver.c>
@@ -188,7 +188,10 @@ __get_nprocs (void)
cp = buffer_end;
re = buffer_end;
- result = 1;
+
+ /* Default to an SMP system in case we cannot obtain an accurate
+ number. */
+ result = 2;
/* The /proc/stat format is more uniform, use it by default. */
fd = open_not_cancel_2 ("/proc/stat", flags);