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

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