[PATCH] sysconf: Use conservative default for _SC_NPROCESSORS_ONLN [BZ #21542]

[PATCH] sysconf: Use conservative default for _SC_NPROCESSORS_ONLN [BZ #21542]

[Florian Weimer]

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.

diff --git a/posix/Makefile b/posix/Makefile
index 52b022c..4246f24 100644
--- a/posix/Makefile
+++ b/posix/Makefile
@@ -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
diff --git a/posix/tst-sysconf-empty-chroot.c b/posix/tst-sysconf-empty-chroot.c
new file mode 100644
index 0000000..dd3b94b
--- /dev/null
+++ b/posix/tst-sysconf-empty-chroot.c
@@ -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>
diff --git a/sysdeps/unix/sysv/linux/getsysstats.c b/sysdeps/unix/sysv/linux/getsysstats.c
index 63e4110..a0dd6eb 100644
--- a/sysdeps/unix/sysv/linux/getsysstats.c
+++ b/sysdeps/unix/sysv/linux/getsysstats.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);

Re: [PATCH] sysconf: Use conservative default for _SC_NPROCESSORS_ONLN [BZ #21542]

[Florian Weimer]

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

Re: [PATCH] sysconf: Use conservative default for _SC_NPROCESSORS_ONLN [BZ #21542]

[Zack Weinberg]

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.

Re: [PATCH] sysconf: Use conservative default for _SC_NPROCESSORS_ONLN [BZ #21542]

[Florian Weimer]

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

Re: [PATCH] sysconf: Use conservative default for _SC_NPROCESSORS_ONLN [BZ #21542]

[H.J. Lu]

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.

-- 
H.J.

Re: [PATCH] sysconf: Use conservative default for _SC_NPROCESSORS_ONLN [BZ #21542]

[Florian Weimer]

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

Re: [PATCH] sysconf: Use conservative default for _SC_NPROCESSORS_ONLN [BZ #21542]

[H.J. Lu]

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.


-- 
H.J.

Re: [PATCH] sysconf: Use conservative default for _SC_NPROCESSORS_ONLN [BZ #21542]

[Florian Weimer]

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

Re: [PATCH] sysconf: Use conservative default for _SC_NPROCESSORS_ONLN [BZ #21542]

[H.J. Lu]

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.

Re: [PATCH] sysconf: Use conservative default for _SC_NPROCESSORS_ONLN [BZ #21542]

[Florian Weimer]

"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

Re: [PATCH] sysconf: Use conservative default for _SC_NPROCESSORS_ONLN [BZ #21542]

[Florian Weimer]

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