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

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-07-02 12:54:18)
> 
> 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 at 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.

Definitely prefer not to assert here if we can help it.
igt_warn should be obvious enough to catch change in tags, but not if
the information changes.
 
> > +}
> > +
> >   /**
> >    * 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.

There was a reason iirc, but I think it was just for debug convenience.

> > +             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?

I expect someone will eventually deem it to be too dangerous information
and remove it. I felt since we had the code here, we might as well try.
Technically /proc doesn't have to be mounted. :|

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

The question we ask is whether or not the test itself will be swapped
out; I don't mind if we swap others out in order to run the test. We do
after all try and purge memory to make it all available to ourselves,
and part of that was in the belief that it would make the filecache
available to us.
-Chris