[PATCH 1/2] Linux: implement getloadavg(3) using sysinfo(2)

[PATCH 1/2] Linux: implement getloadavg(3) using sysinfo(2)

[Cristian Rodríguez]

Signed-off-by: Cristian Rodríguez <crrodriguez@opensuse.org>
---
 sysdeps/unix/sysv/linux/getloadavg.c | 50 ++++++++--------------------
 1 file changed, 14 insertions(+), 36 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/getloadavg.c b/sysdeps/unix/sysv/linux/getloadavg.c
index e50cc396e7..3ea7fd02b7 100644
--- a/sysdeps/unix/sysv/linux/getloadavg.c
+++ b/sysdeps/unix/sysv/linux/getloadavg.c
@@ -1,4 +1,4 @@
-/* Get system load averages.  Linux (/proc/loadavg) version.
+/* Get system load averages.  Linux version.
    Copyright (C) 1999-2021 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
@@ -16,53 +16,31 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
-#include <fcntl.h>
-#include <locale.h>
-#include <stdlib.h>
-#include <unistd.h>
-#include <not-cancel.h>
+#include <array_length.h>
+#include <sys/param.h>
+#include <sys/sysinfo.h>
 
 /* Put the 1 minute, 5 minute and 15 minute load averages
    into the first NELEM elements of LOADAVG.
    Return the number written (never more than 3, but may be less than NELEM),
    or -1 if an error occurred.  */
 
+#define CLAMP(v, lo, hi) MIN (MAX (v, lo), hi)
+
+#define SYSINFO_LOADS_SCALE (1 << SI_LOAD_SHIFT)
+
 int
 getloadavg (double loadavg[], int nelem)
 {
-  int fd;
+  struct sysinfo info = {};
 
-  fd = __open_nocancel ("/proc/loadavg", O_RDONLY);
-  if (fd < 0)
+  if (__sysinfo (&info) != 0)
     return -1;
-  else
-    {
-      char buf[65], *p;
-      ssize_t nread;
-      int i;
 
-      nread = __read_nocancel (fd, buf, sizeof buf - 1);
-      __close_nocancel_nostatus (fd);
-      if (nread <= 0)
-	return -1;
-      buf[nread - 1] = '\0';
+  nelem = CLAMP (nelem, 0, (int)array_length (info.loads));
 
-      if (nelem > 3)
-	nelem = 3;
-      p = buf;
-      for (i = 0; i < nelem; ++i)
-	{
-	  char *endp;
-	  loadavg[i] = __strtod_l (p, &endp, _nl_C_locobj_ptr);
-	  if (endp == p)
-	    /* This should not happen.  The format of /proc/loadavg
-	       must have changed.  Don't return with what we have,
-	       signal an error.  */
-	    return -1;
-	  p = endp;
-	}
+  for (int i = 0; i < nelem; i++)
+    loadavg[i] = (double)info.loads[i] / SYSINFO_LOADS_SCALE;
 
-      return i;
-    }
+  return nelem;
 }
-- 
2.32.0

[PATCH 2/2] resolv: make res_randomid use random_bits()

[Cristian Rodríguez]

It is at least "more random" than 0xffff & __getpid ();

Signed-off-by: Cristian Rodríguez <crrodriguez@opensuse.org>
---
 resolv/res_randomid.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/resolv/res_randomid.c b/resolv/res_randomid.c
index 546daf4c8b..08a52a111a 100644
--- a/resolv/res_randomid.c
+++ b/resolv/res_randomid.c
@@ -84,9 +84,10 @@
 
 #include <resolv.h>
 #include <unistd.h>
+#include <random-bits.h>
 
 unsigned int
 res_randomid (void) {
-  return 0xffff & __getpid ();
+  return random_bits();
 }
 libc_hidden_def (__res_randomid)
-- 
2.32.0

Re: [PATCH 2/2] resolv: make res_randomid use random_bits()

[Cristian Rodríguez]

Ping..

On Fri, Aug 6, 2021 at 3:18 PM Cristian Rodríguez
<crrodriguez@opensuse.org> wrote:
>
> It is at least "more random" than 0xffff & __getpid ();
>
> Signed-off-by: Cristian Rodríguez <crrodriguez@opensuse.org>
> ---
>  resolv/res_randomid.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/resolv/res_randomid.c b/resolv/res_randomid.c
> index 546daf4c8b..08a52a111a 100644
> --- a/resolv/res_randomid.c
> +++ b/resolv/res_randomid.c
> @@ -84,9 +84,10 @@
>
>  #include <resolv.h>
>  #include <unistd.h>
> +#include <random-bits.h>
>
>  unsigned int
>  res_randomid (void) {
> -  return 0xffff & __getpid ();
> +  return random_bits();
>  }
>  libc_hidden_def (__res_randomid)
> --
> 2.32.0

Re: [PATCH 2/2] resolv: make res_randomid use random_bits()

[Florian Weimer]

* Cristian Rodríguez:

> It is at least "more random" than 0xffff & __getpid ();
>
> Signed-off-by: Cristian Rodríguez <crrodriguez@opensuse.org>
> ---
>  resolv/res_randomid.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/resolv/res_randomid.c b/resolv/res_randomid.c
> index 546daf4c8b..08a52a111a 100644
> --- a/resolv/res_randomid.c
> +++ b/resolv/res_randomid.c
> @@ -84,9 +84,10 @@
>  
>  #include <resolv.h>
>  #include <unistd.h>
> +#include <random-bits.h>
>  
>  unsigned int
>  res_randomid (void) {
> -  return 0xffff & __getpid ();
> +  return random_bits();
>  }
>  libc_hidden_def (__res_randomid)

The masking with 0xffff needs to remain, and there should be a space
before ().

Thanks,
Florian

Re: [PATCH 2/2] resolv: make res_randomid use random_bits()

[Cristian Rodríguez]

On Fri, Aug 20, 2021 at 3:00 AM Florian Weimer <fweimer@redhat.com> wrote:

> >  libc_hidden_def (__res_randomid)
>
> The masking with 0xffff needs to remain,

arrgh..damn !  id is an unsigned short..
you are right.

 and there should be a space
> before ().

Ok thanks..

Re: [PATCH 1/2] Linux: implement getloadavg(3) using sysinfo(2)

[Cristian Rodríguez]

ping

On Fri, Aug 6, 2021 at 3:18 PM Cristian Rodríguez
<crrodriguez@opensuse.org> wrote:
>
> Signed-off-by: Cristian Rodríguez <crrodriguez@opensuse.org>
> ---
>  sysdeps/unix/sysv/linux/getloadavg.c | 50 ++++++++--------------------
>  1 file changed, 14 insertions(+), 36 deletions(-)
>
> diff --git a/sysdeps/unix/sysv/linux/getloadavg.c b/sysdeps/unix/sysv/linux/getloadavg.c
> index e50cc396e7..3ea7fd02b7 100644
> --- a/sysdeps/unix/sysv/linux/getloadavg.c
> +++ b/sysdeps/unix/sysv/linux/getloadavg.c
> @@ -1,4 +1,4 @@
> -/* Get system load averages.  Linux (/proc/loadavg) version.
> +/* Get system load averages.  Linux version.
>     Copyright (C) 1999-2021 Free Software Foundation, Inc.
>     This file is part of the GNU C Library.
>
> @@ -16,53 +16,31 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>
> -#include <errno.h>
> -#include <fcntl.h>
> -#include <locale.h>
> -#include <stdlib.h>
> -#include <unistd.h>
> -#include <not-cancel.h>
> +#include <array_length.h>
> +#include <sys/param.h>
> +#include <sys/sysinfo.h>
>
>  /* Put the 1 minute, 5 minute and 15 minute load averages
>     into the first NELEM elements of LOADAVG.
>     Return the number written (never more than 3, but may be less than NELEM),
>     or -1 if an error occurred.  */
>
> +#define CLAMP(v, lo, hi) MIN (MAX (v, lo), hi)
> +
> +#define SYSINFO_LOADS_SCALE (1 << SI_LOAD_SHIFT)
> +
>  int
>  getloadavg (double loadavg[], int nelem)
>  {
> -  int fd;
> +  struct sysinfo info = {};
>
> -  fd = __open_nocancel ("/proc/loadavg", O_RDONLY);
> -  if (fd < 0)
> +  if (__sysinfo (&info) != 0)
>      return -1;
> -  else
> -    {
> -      char buf[65], *p;
> -      ssize_t nread;
> -      int i;
>
> -      nread = __read_nocancel (fd, buf, sizeof buf - 1);
> -      __close_nocancel_nostatus (fd);
> -      if (nread <= 0)
> -       return -1;
> -      buf[nread - 1] = '\0';
> +  nelem = CLAMP (nelem, 0, (int)array_length (info.loads));
>
> -      if (nelem > 3)
> -       nelem = 3;
> -      p = buf;
> -      for (i = 0; i < nelem; ++i)
> -       {
> -         char *endp;
> -         loadavg[i] = __strtod_l (p, &endp, _nl_C_locobj_ptr);
> -         if (endp == p)
> -           /* This should not happen.  The format of /proc/loadavg
> -              must have changed.  Don't return with what we have,
> -              signal an error.  */
> -           return -1;
> -         p = endp;
> -       }
> +  for (int i = 0; i < nelem; i++)
> +    loadavg[i] = (double)info.loads[i] / SYSINFO_LOADS_SCALE;
>
> -      return i;
> -    }
> +  return nelem;
>  }
> --
> 2.32.0

Re: [PATCH 1/2] Linux: implement getloadavg(3) using sysinfo(2)

[Adhemerval Zanella]

On 06/08/2021 16:17, Cristian Rodríguez wrote:
> Signed-off-by: Cristian Rodríguez <crrodriguez@opensuse.org>

Look good, some minor comments below.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  sysdeps/unix/sysv/linux/getloadavg.c | 50 ++++++++--------------------
>  1 file changed, 14 insertions(+), 36 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/getloadavg.c b/sysdeps/unix/sysv/linux/getloadavg.c
> index e50cc396e7..3ea7fd02b7 100644
> --- a/sysdeps/unix/sysv/linux/getloadavg.c
> +++ b/sysdeps/unix/sysv/linux/getloadavg.c
> @@ -1,4 +1,4 @@
> -/* Get system load averages.  Linux (/proc/loadavg) version.
> +/* Get system load averages.  Linux version.
>     Copyright (C) 1999-2021 Free Software Foundation, Inc.
>     This file is part of the GNU C Library.
>  

Ok.

> @@ -16,53 +16,31 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -#include <errno.h>
> -#include <fcntl.h>
> -#include <locale.h>
> -#include <stdlib.h>
> -#include <unistd.h>
> -#include <not-cancel.h>
> +#include <array_length.h>
> +#include <sys/param.h>
> +#include <sys/sysinfo.h>
>  
>  /* Put the 1 minute, 5 minute and 15 minute load averages
>     into the first NELEM elements of LOADAVG.
>     Return the number written (never more than 3, but may be less than NELEM),
>     or -1 if an error occurred.  */
>  
> +#define CLAMP(v, lo, hi) MIN (MAX (v, lo), hi)
> +
> +#define SYSINFO_LOADS_SCALE (1 << SI_LOAD_SHIFT)
> +
>  int
>  getloadavg (double loadavg[], int nelem)
>  {
> -  int fd;
> +  struct sysinfo info = {};

I think there is no need to clear the struct prior the syscall,
I would expect the kernel to fill all the appropriated values
(as it seems to be doing on kernel/sys.c).

>  
> -  fd = __open_nocancel ("/proc/loadavg", O_RDONLY);
> -  if (fd < 0)
> +  if (__sysinfo (&info) != 0)
>      return -1;
> -  else
> -    {
> -      char buf[65], *p;
> -      ssize_t nread;
> -      int i;
>  
> -      nread = __read_nocancel (fd, buf, sizeof buf - 1);
> -      __close_nocancel_nostatus (fd);
> -      if (nread <= 0)
> -	return -1;
> -      buf[nread - 1] = '\0';
> +  nelem = CLAMP (nelem, 0, (int)array_length (info.loads));

I think there is no need to cast to int here.

>  
> -      if (nelem > 3)
> -	nelem = 3;
> -      p = buf;
> -      for (i = 0; i < nelem; ++i)
> -	{
> -	  char *endp;
> -	  loadavg[i] = __strtod_l (p, &endp, _nl_C_locobj_ptr);
> -	  if (endp == p)
> -	    /* This should not happen.  The format of /proc/loadavg
> -	       must have changed.  Don't return with what we have,
> -	       signal an error.  */
> -	    return -1;
> -	  p = endp;
> -	}
> +  for (int i = 0; i < nelem; i++)
> +    loadavg[i] = (double)info.loads[i] / SYSINFO_LOADS_SCALE;
>  
> -      return i;
> -    }
> +  return nelem;
>  }
> 

Ok.

Re: [PATCH 1/2] Linux: implement getloadavg(3) using sysinfo(2)

[Cristian Rodríguez]

On Thu, Aug 19, 2021 at 3:17 PM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 06/08/2021 16:17, Cristian Rodríguez wrote:
> > Signed-off-by: Cristian Rodríguez <crrodriguez@opensuse.org>
>
> Look good, some minor comments below.

First, thanks for looking..

> > +  struct sysinfo info = {};
>
> I think there is no need to clear the struct prior the syscall,
> I would expect the kernel to fill all the appropriated values
> (as it seems to be doing on kernel/sys.c).


Yes, I read the kernel source, I  just decided not to count on the
kernel doing it.


> > +  nelem = CLAMP (nelem, 0, (int)array_length (info.loads));
>
> I think there is no need to cast to int here

IIRC the compiler complained I was comparing signed and unsigned
values.. array_length expands to a size_t value.

Re: [PATCH 1/2] Linux: implement getloadavg(3) using sysinfo(2)

[Adhemerval Zanella]

On 19/08/2021 18:38, Cristian Rodríguez wrote:
> On Thu, Aug 19, 2021 at 3:17 PM Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>>
>> On 06/08/2021 16:17, Cristian Rodríguez wrote:
>>> Signed-off-by: Cristian Rodríguez <crrodriguez@opensuse.org>
>>
>> Look good, some minor comments below.
> 
> First, thanks for looking..
> 
>>> +  struct sysinfo info = {};
>>
>> I think there is no need to clear the struct prior the syscall,
>> I would expect the kernel to fill all the appropriated values
>> (as it seems to be doing on kernel/sys.c).
> 
> 
> Yes, I read the kernel source, I  just decided not to count on the
> kernel doing it.

In this case, clearing the struct does not really improve for the
hypothetical kernel that does not set all the bits because you
won't know if the returned value has meaningful data or not.

> 
> 
>>> +  nelem = CLAMP (nelem, 0, (int)array_length (info.loads));
>>
>> I think there is no need to cast to int here

In this case I think it would be better to make CLAMP a proper
inline function that returns size_t.

> 
> IIRC the compiler complained I was comparing signed and unsigned
> values.. array_length expands to a size_t value.
>