Re: [PATCH v3 6/7] drm: Add fdinfo memory stats

[Rob Clark]

On Mon, Apr 17, 2023 at 4:10 AM Tvrtko Ursulin
<tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
>
>
> On 16/04/2023 08:48, Daniel Vetter wrote:
> > On Fri, Apr 14, 2023 at 06:40:27AM -0700, Rob Clark wrote:
> >> On Fri, Apr 14, 2023 at 1:57 AM Tvrtko Ursulin
> >> <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
> >>>
> >>>
> >>> On 13/04/2023 21:05, Daniel Vetter wrote:
> >>>> On Thu, Apr 13, 2023 at 05:40:21PM +0100, Tvrtko Ursulin wrote:
> >>>>>
> >>>>> On 13/04/2023 14:27, Daniel Vetter wrote:
> >>>>>> On Thu, Apr 13, 2023 at 01:58:34PM +0100, Tvrtko Ursulin wrote:
> >>>>>>>
> >>>>>>> On 12/04/2023 20:18, Daniel Vetter wrote:
> >>>>>>>> On Wed, Apr 12, 2023 at 11:42:07AM -0700, Rob Clark wrote:
> >>>>>>>>> On Wed, Apr 12, 2023 at 11:17 AM Daniel Vetter <daniel@xxxxxxxx> wrote:
> >>>>>>>>>>
> >>>>>>>>>> On Wed, Apr 12, 2023 at 10:59:54AM -0700, Rob Clark wrote:
> >>>>>>>>>>> On Wed, Apr 12, 2023 at 7:42 AM Tvrtko Ursulin
> >>>>>>>>>>> <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> On 11/04/2023 23:56, Rob Clark wrote:
> >>>>>>>>>>>>> From: Rob Clark <robdclark@xxxxxxxxxxxx>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Add support to dump GEM stats to fdinfo.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> v2: Fix typos, change size units to match docs, use div_u64
> >>>>>>>>>>>>> v3: Do it in core
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>
> >>>>>>>>>>>>> Reviewed-by: Emil Velikov <emil.l.velikov@xxxxxxxxx>
> >>>>>>>>>>>>> ---
> >>>>>>>>>>>>>       Documentation/gpu/drm-usage-stats.rst | 21 ++++++++
> >>>>>>>>>>>>>       drivers/gpu/drm/drm_file.c            | 76 +++++++++++++++++++++++++++
> >>>>>>>>>>>>>       include/drm/drm_file.h                |  1 +
> >>>>>>>>>>>>>       include/drm/drm_gem.h                 | 19 +++++++
> >>>>>>>>>>>>>       4 files changed, 117 insertions(+)
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
> >>>>>>>>>>>>> index b46327356e80..b5e7802532ed 100644
> >>>>>>>>>>>>> --- a/Documentation/gpu/drm-usage-stats.rst
> >>>>>>>>>>>>> +++ b/Documentation/gpu/drm-usage-stats.rst
> >>>>>>>>>>>>> @@ -105,6 +105,27 @@ object belong to this client, in the respective memory region.
> >>>>>>>>>>>>>       Default unit shall be bytes with optional unit specifiers of 'KiB' or 'MiB'
> >>>>>>>>>>>>>       indicating kibi- or mebi-bytes.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> +- drm-shared-memory: <uint> [KiB|MiB]
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> +The total size of buffers that are shared with another file (ie. have more
> >>>>>>>>>>>>> +than a single handle).
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> +- drm-private-memory: <uint> [KiB|MiB]
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> +The total size of buffers that are not shared with another file.
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> +- drm-resident-memory: <uint> [KiB|MiB]
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> +The total size of buffers that are resident in system memory.
> >>>>>>>>>>>>
> >>>>>>>>>>>> I think this naming maybe does not work best with the existing
> >>>>>>>>>>>> drm-memory-<region> keys.
> >>>>>>>>>>>
> >>>>>>>>>>> Actually, it was very deliberate not to conflict with the existing
> >>>>>>>>>>> drm-memory-<region> keys ;-)
> >>>>>>>>>>>
> >>>>>>>>>>> I wouldn't have preferred drm-memory-{active,resident,...} but it
> >>>>>>>>>>> could be mis-parsed by existing userspace so my hands were a bit tied.
> >>>>>>>>>>>
> >>>>>>>>>>>> How about introduce the concept of a memory region from the start and
> >>>>>>>>>>>> use naming similar like we do for engines?
> >>>>>>>>>>>>
> >>>>>>>>>>>> drm-memory-$CATEGORY-$REGION: ...
> >>>>>>>>>>>>
> >>>>>>>>>>>> Then we document a bunch of categories and their semantics, for instance:
> >>>>>>>>>>>>
> >>>>>>>>>>>> 'size' - All reachable objects
> >>>>>>>>>>>> 'shared' - Subset of 'size' with handle_count > 1
> >>>>>>>>>>>> 'resident' - Objects with backing store
> >>>>>>>>>>>> 'active' - Objects in use, subset of resident
> >>>>>>>>>>>> 'purgeable' - Or inactive? Subset of resident.
> >>>>>>>>>>>>
> >>>>>>>>>>>> We keep the same semantics as with process memory accounting (if I got
> >>>>>>>>>>>> it right) which could be desirable for a simplified mental model.
> >>>>>>>>>>>>
> >>>>>>>>>>>> (AMD needs to remind me of their 'drm-memory-...' keys semantics. If we
> >>>>>>>>>>>> correctly captured this in the first round it should be equivalent to
> >>>>>>>>>>>> 'resident' above. In any case we can document no category is equal to
> >>>>>>>>>>>> which category, and at most one of the two must be output.)
> >>>>>>>>>>>>
> >>>>>>>>>>>> Region names we at most partially standardize. Like we could say
> >>>>>>>>>>>> 'system' is to be used where backing store is system RAM and others are
> >>>>>>>>>>>> driver defined.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Then discrete GPUs could emit N sets of key-values, one for each memory
> >>>>>>>>>>>> region they support.
> >>>>>>>>>>>>
> >>>>>>>>>>>> I think this all also works for objects which can be migrated between
> >>>>>>>>>>>> memory regions. 'Size' accounts them against all regions while for
> >>>>>>>>>>>> 'resident' they only appear in the region of their current placement, etc.
> >>>>>>>>>>>
> >>>>>>>>>>> I'm not too sure how to rectify different memory regions with this,
> >>>>>>>>>>> since drm core doesn't really know about the driver's memory regions.
> >>>>>>>>>>> Perhaps we can go back to this being a helper and drivers with vram
> >>>>>>>>>>> just don't use the helper?  Or??
> >>>>>>>>>>
> >>>>>>>>>> I think if you flip it around to drm-$CATEGORY-memory{-$REGION}: then it
> >>>>>>>>>> all works out reasonably consistently?
> >>>>>>>>>
> >>>>>>>>> That is basically what we have now.  I could append -system to each to
> >>>>>>>>> make things easier to add vram/etc (from a uabi standpoint)..
> >>>>>>>>
> >>>>>>>> What you have isn't really -system, but everything. So doesn't really make
> >>>>>>>> sense to me to mark this -system, it's only really true for integrated (if
> >>>>>>>> they don't have stolen or something like that).
> >>>>>>>>
> >>>>>>>> Also my comment was more in reply to Tvrtko's suggestion.
> >>>>>>>
> >>>>>>> Right so my proposal was drm-memory-$CATEGORY-$REGION which I think aligns
> >>>>>>> with the current drm-memory-$REGION by extending, rather than creating
> >>>>>>> confusion with different order of key name components.
> >>>>>>
> >>>>>> Oh my comment was pretty much just bikeshed, in case someone creates a
> >>>>>> $REGION that other drivers use for $CATEGORY. Kinda Rob's parsing point.
> >>>>>> So $CATEGORY before the -memory.
> >>>>>>
> >>>>>> Otoh I don't think that'll happen, so I guess we can go with whatever more
> >>>>>> folks like :-) I don't really care much personally.
> >>>>>
> >>>>> Okay I missed the parsing problem.
> >>>>>
> >>>>>>> AMD currently has (among others) drm-memory-vram, which we could define in
> >>>>>>> the spec maps to category X, if category component is not present.
> >>>>>>>
> >>>>>>> Some examples:
> >>>>>>>
> >>>>>>> drm-memory-resident-system:
> >>>>>>> drm-memory-size-lmem0:
> >>>>>>> drm-memory-active-vram:
> >>>>>>>
> >>>>>>> Etc.. I think it creates a consistent story.
> >>>>>>>
> >>>>>>> Other than this, my two I think significant opens which haven't been
> >>>>>>> addressed yet are:
> >>>>>>>
> >>>>>>> 1)
> >>>>>>>
> >>>>>>> Why do we want totals (not per region) when userspace can trivially
> >>>>>>> aggregate if they want. What is the use case?
> >>>>>>>
> >>>>>>> 2)
> >>>>>>>
> >>>>>>> Current proposal limits the value to whole objects and fixates that by
> >>>>>>> having it in the common code. If/when some driver is able to support sub-BO
> >>>>>>> granularity they will need to opt out of the common printer at which point
> >>>>>>> it may be less churn to start with a helper rather than mid-layer. Or maybe
> >>>>>>> some drivers already support this, I don't know. Given how important VM BIND
> >>>>>>> is I wouldn't be surprised.
> >>>>>>
> >>>>>> I feel like for drivers using ttm we want a ttm helper which takes care of
> >>>>>> the region printing in hopefully a standard way. And that could then also
> >>>>>> take care of all kinds of of partial binding and funny rules (like maybe
> >>>>>> we want a standard vram region that addds up all the lmem regions on
> >>>>>> intel, so that all dgpu have a common vram bucket that generic tools
> >>>>>> understand?).
> >>>>>
> >>>>> First part yes, but for the second I would think we want to avoid any
> >>>>> aggregation in the kernel which can be done in userspace just as well. Such
> >>>>> total vram bucket would be pretty useless on Intel even since userspace
> >>>>> needs to be region aware to make use of all resources. It could even be
> >>>>> counter productive I think - "why am I getting out of memory when half of my
> >>>>> vram is unused!?".
> >>>>
> >>>> This is not for intel-aware userspace. This is for fairly generic "gputop"
> >>>> style userspace, which might simply have no clue or interest in what lmemX
> >>>> means, but would understand vram.
> >>>>
> >>>> Aggregating makes sense.
> >>>
> >>> Lmem vs vram is now an argument not about aggregation but about
> >>> standardizing regions names.
> >>>
> >>> One detail also is a change in philosophy compared to engine stats where
> >>> engine names are not centrally prescribed and it was expected userspace
> >>> will have to handle things generically and with some vendor specific
> >>> knowledge.
> >>>
> >>> Like in my gputop patches. It doesn't need to understand what is what,
> >>> it just finds what's there and presents it to the user.
> >>>
> >>> Come some accel driver with local memory it wouldn't be vram any more.
> >>> Or even a headless data center GPU. So I really don't think it is good
> >>> to hardcode 'vram' in the spec, or midlayer, or helpers.
> >>>
> >>> And for aggregation.. again, userspace can do it just as well. If we do
> >>> it in kernel then immediately we have multiple sets of keys to output
> >>> for any driver which wants to show the region view. IMO it is just
> >>> pointless work in the kernel and more code in the kernel, when userspace
> >>> can do it.
> >>>
> >>> Proposal A (one a discrete gpu, one category only):
> >>>
> >>> drm-resident-memory: x KiB
> >>> drm-resident-memory-system: x KiB
> >>> drm-resident-memory-vram: x KiB
> >>>
> >>> Two loops in the kernel, more parsing in userspace.
> >>
> >> why would it be more than one loop, ie.
> >>
> >>      mem.resident += size;
> >>      mem.category[cat].resident += size;
> >>
> >> At the end of the day, there is limited real-estate to show a million
> >> different columns of information.  Even the gputop patches I posted
> >> don't show everything of what is currently there.  And nvtop only
> >> shows toplevel resident stat.  So I think the "everything" stat is
> >> going to be what most tools use.
> >
> > Yeah with enough finesse the double-loop isn't needed, it's just the
> > simplest possible approach.
> >
> > Also this is fdinfo, I _really_ want perf data showing that it's a
> > real-world problem when we conjecture about algorithmic complexity.
> > procutils have been algorithmically garbage since decades after all :-)
>
> Just run it. :)
>
> Algorithmic complexity is quite obvious and not a conjecture - to find
> DRM clients you have to walk _all_ pids and _all_ fds under them. So
> amount of work can scale very quickly and even _not_ with the number of
> DRM clients.
>
> It's not too bad on my desktop setup but it is significantly more CPU
> intensive than top(1).
>
> It would be possible to optimise the current code some more by not
> parsing full fdinfo (may become more important as number of keys grow),
> but that's only relevant when number of drm fds is large. It doesn't
> solve the basic pids * open fds search for which we'd need a way to walk
> the list of pids with drm fds directly.

All of which has (almost[1]) nothing to do with one loop or two
(ignoring for a moment that I already pointed out a single loop is all
that is needed).  If CPU overhead is a problem, we could perhaps come
up some sysfs which has one file per drm_file and side-step crawling
of all of the proc * fd.  I'll play around with it some but I'm pretty
sure you are trying to optimize the wrong thing.

BR,
-R

[1] generally a single process using drm has multiple fd's pointing at
the same drm_file.. which makes the current approach of having to read
fdinfo to find the client-id sub-optimal.  But still the total # of
proc * fd is much larger