This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [glibc] Fix Linux sysconf(_SC_NPROCESSORS_[CONF|ONLN])performance problem


On Thu, Jun 16, 2011 at 12:07 PM, Ulrich Drepper <drepper@gmail.com> wrote:
>
> I'm not opposed to any improvements. ?Do you think I like this
> implementation while every other OS which has these interfaces
> implement sysconf() using a simple syscall?

The thing is, I'd be more impressed by your statement if I actually
believed it. Every time I report a problem with glibc, it does seem
like you're actually against improvements, and instead of improving
glibc code, you _invariably_ blame other things instead.

It might be the kernel interfaces, or it might be "broken programs"
like adobe flash. But never is it glibc itself that is the problem.

So I sent a simple patch that I actually think will improve
performance enormously, and nobody will ever complain. But no, not
acceptable.

We can do it other ways. You already parse /sys/devices/system/cpu for
_SC_NPROCESSORS_CONF, although you do it in an odd way (by iterating
over directory entries). You could just open
/sys/devices/system/cpu/online, and parse the result from there
instead. If that file doesn't exist, the system doesn't support
hotplug, so you can do the caching.

The fact is, the kernel interfaces are ALREADY much better than the
ones you actually use.

Here's a new patch. The code worked when it was a test-program, I
haven't actually tested it within the confines of the glibc thing.
There may be special magical internal glibc names it should use for
fopen/fscanf etc.

                                Linus
 sysdeps/unix/sysv/linux/getsysstats.c |   84 +++++++++++++++++++++++++++++++--
 1 files changed, 80 insertions(+), 4 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/getsysstats.c b/sysdeps/unix/sysv/linux/getsysstats.c
index af454b650de9..cda92fc4f8c8 100644
--- a/sysdeps/unix/sysv/linux/getsysstats.c
+++ b/sysdeps/unix/sysv/linux/getsysstats.c
@@ -124,9 +124,44 @@ next_line (int fd, char *const buffer, char **cp, char **re,
   return res == *re ? NULL : res;
 }
 
+static int __get_sysfs_cpus(const char *path)
+{
+  FILE *file;
+  int nr_cpus = 0;
+  int prev = -1;
+
+  file = fopen(path, "r");
+  if (!file)
+    return -1;
+  for (;;) {
+    char sep;
+    int cpu;
+    int n = fscanf(file, "%u%c", &cpu, &sep);
+    if (n <= 0)
+      break;
+
+    /* EOF == EOLN */
+    if (n == 1)
+      sep = '\n';
+
+    /* Was the previous CPU a range? */
+    if (prev >= 0) {
+      nr_cpus += cpu - prev + 1;
+      prev = -1;
+    } else if (sep == '-')
+      prev = cpu;
+    else
+      nr_cpus++;
+
+    if (sep == '\n')
+      break;
+  }
+  fclose(file);
+  return nr_cpus;
+}
 
-int
-__get_nprocs ()
+static int
+__get_nprocs_from_proc (void)
 {
   /* XXX Here will come a test for the new system call.  */
 
@@ -171,13 +206,37 @@ __get_nprocs ()
 
   return result;
 }
+
+int
+__get_nprocs ()
+{
+  long ret;
+  static int cached = -1;
+
+  ret = cached;
+  if (ret < 0)
+    {
+      ret = __get_sysfs_cpus("/sys/devices/system/cpu/online");
+
+      /*
+       * If that failed, we don't support hotplug, and we will
+       * instead cache the result from reading /proc/stat.
+       */
+      if (ret < 0)
+	{
+	  ret = __get_nprocs_from_proc();
+	  cached = ret;
+	}
+    }
+  return ret;
+}
 weak_alias (__get_nprocs, get_nprocs)
 
 
 /* On some architectures it is possible to distinguish between configured
    and active cpus.  */
-int
-__get_nprocs_conf ()
+static int
+__get_nprocs_conf_internal (void)
 {
   /* XXX Here will come a test for the new system call.  */
 
@@ -224,6 +283,23 @@ __get_nprocs_conf ()
 
   return result;
 }
+
+int
+__get_nprocs_conf ()
+{
+  long ret;
+  static int cached = -1;
+
+  ret = cached;
+  if (ret < 0)
+    {
+      ret = __get_sysfs_cpus("/sys/devices/system/cpu/possible");
+      if (ret < 0)
+        ret = __get_nprocs_conf_internal();
+      cached = ret;
+    }
+  return ret;
+}
 weak_alias (__get_nprocs_conf, get_nprocs_conf)
 
 /* General function to get information about memory status from proc

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]