[igt-dev] [PATCH igt 1/2] lib/igt_drm_fdinfo: Parse memory usage

[Rob Clark]

On Thu, Apr 13, 2023 at 8:19 AM Tvrtko Ursulin
<tvrtko.ursulin at linux.intel.com> wrote:
>
>
> On 07/04/2023 22:56, Rob Clark wrote:
> > From: Rob Clark <robdclark at chromium.org>
> >
> > Add parsing for the memory usage related fdinfo stats.
> >
> > Signed-off-by: Rob Clark <robdclark at chromium.org>
> > ---
> >   lib/igt_drm_fdinfo.c | 39 +++++++++++++++++++++++++++++++++++++++
> >   lib/igt_drm_fdinfo.h |  9 +++++++++
> >   2 files changed, 48 insertions(+)
> >
> > diff --git a/lib/igt_drm_fdinfo.c b/lib/igt_drm_fdinfo.c
> > index b850d221..6269e166 100644
> > --- a/lib/igt_drm_fdinfo.c
> > +++ b/lib/igt_drm_fdinfo.c
> > @@ -124,6 +124,34 @@ static const char *find_kv(const char *buf, const char *key, size_t keylen)
> >       return *p ? p : NULL;
> >   }
> >
> > +static size_t find_mem_kv(const char *buf, const char *key)
> > +{
> > +     const char *val = find_kv(buf, key, strlen(key));
>
> If you were asking yourself why I made strlen an argument to find_kv I
> have to admit I micro-optimized it a bit based on profiling. Sad fact is
> hunting for drm fdinfo in procfs sucks and gputop uses more CPU time
> than top, to even display less data. More processes with more open files
> there are, even when the 99.9% are not DRM, more taxing it is.

Did a release build really not end up with it inlined??  If so I guess
we can make this a macro, but it seems a bit surprising.  This should
be a thing the compiler can figure out.

BR,
-R

> So I'd suggest sticking to the micro-optimized pattern. :(
>
> > +     char *p, *unit = NULL;
> > +     size_t sz;
> > +
> > +     if (!val)
> > +             return 0;
> > +
> > +     sz = atoi(val);
>
> Maybe atol and unsigned long instead of size_t would be better for the
> counts throughout? Or uint64_t?
>
> > +
> > +     p = index(val, ' ');
>
> Ok, or can use the flexible method from find_kv which skips any amount
> of any whitespace.
>
> > +     if (!p)
> > +             return sz;
> > +
> > +     unit = ++p;
> > +
> > +     if (!strcmp(unit, "KiB")) {
> > +             sz *= 1024;
> > +     } else if (!strcmp(unit, "MiB")) {
> > +             sz *= 1024 * 1024;
> > +     } else if (!strcmp(unit, "GiB")) {
> > +             sz *= 1024 * 1024 * 1024;
> > +     }
> > +
> > +     return sz;
> > +}
> > +
> >   unsigned int
> >   __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
> >                      const char **name_map, unsigned int map_entries)
> > @@ -140,6 +168,7 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
> >       while ((l = strtok_r(_buf, "\n", &ctx))) {
> >               uint64_t val = 0;
> >               const char *v;
> > +             size_t sz;
> >               int idx;
> >
> >               _buf = NULL;
> > @@ -173,6 +202,16 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
> >                               info->capacity[idx] = val;
> >                               num_capacity++;
> >                       }
> > +             } else if ((sz = find_mem_kv(l, "drm-shared-memory"))) {
> > +                     info->mem.shared = sz;
> > +             } else if ((sz = find_mem_kv(l, "drm-private-memory"))) {
> > +                     info->mem.private = sz;
> > +             } else if ((sz = find_mem_kv(l, "drm-resident-memory"))) {
> > +                     info->mem.resident = sz;
> > +             } else if ((sz = find_mem_kv(l, "drm-purgeable-memory"))) {
> > +                     info->mem.purgeable = sz;
> > +             } else if ((sz = find_mem_kv(l, "drm-active-memory"))) {
> > +                     info->mem.active = sz;
>
> Okay there's an open on key naming and if we went with drm-memory-...
> this could be done with just one strncmp on non-matching lines. Depends
> on the open.
>
> >               }
> >       }
> >
> > diff --git a/lib/igt_drm_fdinfo.h b/lib/igt_drm_fdinfo.h
> > index 6284e05e..dd4bdd54 100644
> > --- a/lib/igt_drm_fdinfo.h
> > +++ b/lib/igt_drm_fdinfo.h
> > @@ -32,6 +32,14 @@
> >
> >   #define DRM_CLIENT_FDINFO_MAX_ENGINES 16
> >
> > +struct drm_client_meminfo {
> > +     size_t shared;
> > +     size_t private;
> > +     size_t resident;
> > +     size_t purgeable;
> > +     size_t active;
> > +};
> > +
> >   struct drm_client_fdinfo {
> >       char driver[128];
> >       char pdev[128];
> > @@ -42,6 +50,7 @@ struct drm_client_fdinfo {
> >       unsigned int capacity[DRM_CLIENT_FDINFO_MAX_ENGINES];
> >       char names[DRM_CLIENT_FDINFO_MAX_ENGINES][256];
> >       uint64_t busy[DRM_CLIENT_FDINFO_MAX_ENGINES];
> > +     struct drm_client_meminfo mem;
> >   };
> >
> >   /**
>
> Regards,
>
> Tvrtko