Re: [igt-dev] [PATCH i-g-t 01/17] lib: Report file cache as available system memory

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 02/07/2018 10:07, Chris Wilson wrote:
> sysinfo() doesn't include all reclaimable memory. In particular it
> excludes the majority of global_node_page_state(NR_FILE_PAGES),
> reclaimable pages that are a copy of on-disk files It seems the only way
> to obtain this counter is by parsing /proc/meminfo. For comparison,
> check vm_enough_memory() which includes NR_FILE_PAGES as available
> (sadly there's no way to call vm_enough_memory() directly either!)
> 
> v2: Pay attention to what one writes.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   lib/intel_os.c | 36 +++++++++++++++++++++++++++++++-----
>   1 file changed, 31 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/intel_os.c b/lib/intel_os.c
> index 885ffdcec..5bd18fc52 100644
> --- a/lib/intel_os.c
> +++ b/lib/intel_os.c
> @@ -84,6 +84,18 @@ intel_get_total_ram_mb(void)
>   	return retval / (1024*1024);
>   }
>   
> +static uint64_t get_meminfo(const char *info, const char *tag)
> +{
> +	const char *str;
> +	unsigned long val;
> +
> +	str = strstr(info, tag);
> +	if (str && sscanf(str + strlen(tag), " %lu", &val) == 1)
> +		return (uint64_t)val << 10;
> +
> +	return 0;

Might be safer to assert format was as expected and keywords we expect 
are there, rather than silently return zero.

> +}
> +
>   /**
>    * intel_get_avail_ram_mb:
>    *
> @@ -96,17 +108,31 @@ intel_get_avail_ram_mb(void)
>   	uint64_t retval;
>   
>   #ifdef HAVE_STRUCT_SYSINFO_TOTALRAM /* Linux */
> -	struct sysinfo sysinf;
> +	char *info;
>   	int fd;
>   
>   	fd = drm_open_driver(DRIVER_INTEL);
>   	intel_purge_vm_caches(fd);
>   	close(fd);
>   
> -	igt_assert(sysinfo(&sysinf) == 0);
> -	retval = sysinf.freeram;
> -	retval += min(sysinf.freeswap, sysinf.bufferram);
> -	retval *= sysinf.mem_unit;
> +	fd = open("/proc", O_RDONLY);
> +	info = igt_sysfs_get(fd, "meminfo");
> +	close(fd);
> +
> +	if (info) {
> +		retval  = get_meminfo(info, "MemAvailable: ");
> +		retval += get_meminfo(info, "Buffers: ");
> +		retval += get_meminfo(info, "Cached: ");
> +		retval += get_meminfo(info, "SwapCached: ");

I think it would be more robust to have no trailing space in tag strings.

> +		free(info);
> +	} else {
> +		struct sysinfo sysinf;
> +
> +		igt_assert(sysinfo(&sysinf) == 0);
> +		retval = sysinf.freeram;
> +		retval += min(sysinf.freeswap, sysinf.bufferram);
> +		retval *= sysinf.mem_unit;
> +	}

Not sure it is worth keeping this path - will we ever not have 
/proc/meminfo?

>   #elif defined(_SC_PAGESIZE) && defined(_SC_AVPHYS_PAGES) /* Solaris */
>   	long pagesize, npages;
>   
> 

Google agrees with you that sysinfo indeed has this limitation. So in 
general no complaints.

One tiny detail might be that this would now return a too large value - 
doesn't count that it should not swap out itself when thinking about 
free memory. But I don't think that is for this level of API to concern 
with - it is definitely way more correct to report page cache.

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev