Re: [PATCH v2 10/13] builtin/gc.c: correct RAM calculation when using sysinfo
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Apr 14, 2025, 07:55 UTC
- Message-ID
- <Z_y_XL2C_Za5SB5m@pks.im>
- In-Reply-To
- <a2eb9ab117c9a7ea8723c166739b30243388ea77.1743859985.git.ramsay@ramsayjones.plus.com>
On Sun, Apr 06, 2025 at 08:38:36PM +0100, Ramsay Jones wrote:
Show 35 quoted lines
> 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.
Show 26 quoted lines
> 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