On 14/04/2025 08:55, Patrick Steinhardt wrote:
> 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@xxxxxxxxxxxxxxxxxxxx>
>> ---
>> 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.
Hmm, you may well be right, but I prefer to be cautious here.
Thanks.
ATB,
Ramsay Jones
