[RFC PATCH v3 04/11] drm, cgroup: Add total GEM buffer allocation limit

[Daniel Vetter]

On Thu, Jun 27, 2019 at 02:42:43PM -0400, Kenny Ho wrote:
> On Thu, Jun 27, 2019 at 1:43 AM Daniel Vetter <daniel at ffwll.ch> wrote:
> >
> > On Wed, Jun 26, 2019 at 06:41:32PM -0400, Kenny Ho wrote:
> > > So without the sharing restriction and some kind of ownership
> > > structure, we will have to migrate/change the owner of the buffer when
> > > the cgroup that created the buffer die before the receiving cgroup(s)
> > > and I am not sure how to do that properly at the moment.  1) Should
> > > each cgroup keep track of all the buffers that belongs to it and
> > > migrate?  (Is that efficient?)  2) which cgroup should be the new
> > > owner (and therefore have the limit applied?)  Having the creator
> > > being the owner is kind of natural, but let say the buffer is shared
> > > with 5 other cgroups, which of these 5 cgroups should be the new owner
> > > of the buffer?
> >
> > Different answers:
> >
> > - Do we care if we leak bos like this in a cgroup, if the cgroup
> >   disappears before all the bo are cleaned up?
> >
> > - Just charge the bo to each cgroup it's in? Will be quite a bit more
> >   tracking needed to get that done ...
> That seems to be the approach memcg takes, but as shown by the lwn
> link you sent me from the last rfc (talk from Roman Gushchin), that
> approach is not problem free either.  And wouldn't this approach
> disconnect resource management from the underlying resource one would
> like to control?  For example, if you have 5 MB of memory, you can
> have 5 users using 1 MB each.  But in the charge-everybody approach, a
> 1 MB usage shared 4 times will make it looks like 5MB is used.  So the
> resource being control is no longer 'real' since the amount of
> resource you have is now dynamic and depends on the amount of sharing
> one does.

The problem with memcg is that it's not just the allocation, but a ton of
memory allocated to track these allocations. At least that's my
understanding of the nature of the memcg leak. Made a lot worse by pages
being small and plentiful and shared extremely widely (e.g. it's really
hard to control who gets charged for pagecache allocations, so those
pagecache entries might outlive the memcg forever if you're unlucky).

For us it's just a counter, plus bo sharing is a lot more controlled: On
any reasonable system if you do kill the compositor, then all the clients
go down. And when you do kill a client, the compositor will release all
the shared buffers (and any other resources).

So I think for drmcg we won't have anything near the same resource leak
problem even in theory, and in practice I think the issue is none.

> > - Also, there's the legacy way of sharing a bo, with the FLINK and
> >   GEM_OPEN ioctls. We need to plug these holes too.
> >
> > Just feels like your current solution is technically well-justified, but
> > it completely defeats the point of cgroups/containers and buffer sharing
> > ...
> Um... I am going to get a bit philosophical here and suggest that the
> idea of sharing (especially uncontrolled sharing) is inherently at odd
> with containment.  It's like, if everybody is special, no one is
> special.  Perhaps an alternative is to make this configurable so that
> people can allow sharing knowing the caveat?  And just to be clear,
> the current solution allows for sharing, even between cgroup.

The thing is, why shouldn't we just allow it (with some documented
caveat)?

I mean if all people do is share it as your current patches allow, then
there's nothing funny going on (at least if we go with just leaking the
allocations). If we allow additional sharing, then that's a plus.

And if you want additional containment, that's a different thing: The
entire linux architecture for containers is that a container doesn't
exist. Instead you get a pile of building blocks that all solve different
aspects of what a container needs to do:
- cgroups for resource limits
- namespaces for resource visibility
- selinux/secomp/lsm for resource isolation and access rights

Let's not try to build a drm cgroup control that tries to do more than
what cgroups are meant to solve. If you have a need to restrict the
sharing, imo that should be done with an lsm security hook.

btw for bo sharing, I've found a 3rd sharing path (besides dma-buf and gem
flink): GETCRTC ioctl can also be used (it's the itended goal actually) to
share buffers across processes.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch