Re: [PATCH v2 10/13] builtin/gc.c: correct RAM calculation when using sysinfo

[Patrick Steinhardt <ps@pks.im>]

On Sun, Apr 06, 2025 at 08:38:36PM +0100, Ramsay Jones wrote:
> The man page for sysinfo(2) on Linux states that (from v2.3.48) the
> sizes of the memory and swap fields, of the returned structure, are
> given as multiples of 'mem_unit' bytes. In earlier versions (prior to
> v2.3.23 on i386 in particular), the 'mem_unit' field was not part of
> the structure, and all sizes were measured in bytes. The man page does
> not discuss the motivation for this change, but it is possible that the
> change was intended for the, relatively rare, 32-bit platform with more
> than 4GB of memory.
> 
> The total_ram() function makes the assumption that the 'totalram' field
> of the 'struct sysinfo' is measured in bytes, or alternatively that the
> 'mem_unit' field is always equal to one. Having writen a program to call
> the sysinfo() function and print the structure fields, it seems that, on
> Linux x84_64 and i686 anyway, the 'mem_unit' field is indeed set to one
> (note that the 32-bit system had only 2GB ram). However, cygwin also has
> an sysinfo() implementation, which gives the following values:
> 
>   $ ./sysinfo
>   uptime:      21381
>   loads:       0, 0, 0
>   total ram:   2074637
>   free ram:    843237
>   shared ram:  0
>   buffer ram:  0
>   total swap:  327680
>   free swap:   306932
>   procs:       15
>   total high:  0
>   free high:   0
>   mem_unit:    4096
> 
>   total ram: 8497713152
>   $
> 
> [This laptop has 8GB ram, so a little bit seems to be missing. ;) ]

Interesting. I can confirm that `mem_unit` is 1 on my system, so this
does not make a difference here. But my tests on Cygwin show the same
behaviour as on your system, so the patch looks reasonable to me.

> Modify the total_ram() function to allow for the possibility that the
> memory size is not specified in bytes (ie 'mem_unit' is greater than
> one).
> 
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
>  builtin/gc.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 99431fd467..cdcf1dc6e7 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -373,8 +373,13 @@ static uint64_t total_ram(void)
>  #if defined(HAVE_SYSINFO)
>  	struct sysinfo si;
>  
> -	if (!sysinfo(&si))
> -		return si.totalram;
> +	if (!sysinfo(&si)) {
> +		uint64_t total = si.totalram;
> +
> +		if (si.mem_unit > 1)
> +			total *= (uint64_t)si.mem_unit;
> +		return total;
> +	}

I expect that all systems have a proper value for `si.mem_unit` set so
that we could unconditionally multiplicate the fields with one another.
But it doesn't hurt either, so I don't mind the guarding clause.

Patrick