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

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

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
  

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.
  

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.
  

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.
>