Re: [RFC PATCH v3 04/11] drm, cgroup: Add total GEM buffer allocation limit
On Fri, Jun 28, 2019 at 02:43:18PM -0400, Kenny Ho wrote: > On Thu, Jun 27, 2019 at 5:24 PM Daniel Vetter wrote: > > On Thu, Jun 27, 2019 at 02:42:43PM -0400, Kenny Ho wrote: > > > 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. > Um... perhaps I was being overly conservative :). So let me > illustrate with an example to add more clarity and get more comments > on it. > > Let say we have the following cgroup hierarchy (The letters are > cgroups with R being the root cgroup. The numbers in brackets are > processes. The processes are placed with the 'No Internal Process > Constraint' in mind.) > R (4, 5) -- A (6) > \ > B C (7,8) > \ >D (9) > > Here is a list of operation and the associated effect on the size > track by the cgroups (for simplicity, each buffer is 1 unit in size.) > With current implementation (charge on buffer creation with > restriction on sharing.) > R A B C D |Ops > > 1 0 0 0 0 |4 allocated a buffer > 1 0 0 0 0 |4 shared a buffer with 5 > 1 0 0 0 0 |4 shared a buffer with 9 > 2 0 1 0 1 |9 allocated a buffer > 3 0 2 1 1 |7 allocated a buffer > 3 0 2 1 1 |7 shared a buffer with 8 > 3 0 2 1 1 |7 sharing with 9 (not allowed) > 3 0 2 1 1 |7 sharing with 4 (not allowed) > 3 0 2 1 1 |7 release a buffer > 2 0 1 0 1 |8 release a buffer from 7 This is your current implementation, right? Let's call it A. > The suggestion as I understand it (charge per buffer reference with > unrestricted sharing.) > R A B C D |Ops > > 1 0 0 0 0 |4 allocated a buffer > 2 0 0 0 0 |4 shared a buffer with 5 > 3 0 0 0 1 |4 shared a buffer with 9 > 4 0 1 0 2 |9 allocated a buffer > 5 0 2 1 1 |7 allocated a buffer > 6 0 3 2 1 |7 shared a buffer with 8 > 7 0 4 2 2 |7 sharing with 9 > 8 0 4 2 2 |7 sharing with 4 > 7 0 3 1 2 |7 release a buffer > 6 0 2 0 2 |8 release a buffer from 7 > > Is this a correct understanding of the suggestion? Yup that's one option I think. The other option (and it's probably simpler), is to go with your current accounting, but drop the sharing restriction. I.e. buffers are accounting to whomever allocates them first, not who's all using them. For memcg this has some serious trouble with cgroups not getting cleaned up due to leaked refrences. But for gem bo we spread the references in a lot more controlled manner, and all the long-lived references are under control of userspace. E.g. if Xorg fails to clean up bo references of clients that dead, that's clearly an Xorg bug and needs to be fixed there. But not something we need to allow as a valid use-cases. For page references/accounting in memcg this is totally different, since pages can survive in the pagecache forever. No such bo-cache or anything similar exists for gem_bo. Personally I prefer option A, but on sharing restriction. If you want that sharing restriction, we need to figure out how to implement it using something else. Plus we need to make sure all possible ways to share a bo are covered (and there are many). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v3 04/11] drm, cgroup: Add total GEM buffer allocation limit
On Thu, Jun 27, 2019 at 5:24 PM Daniel Vetter wrote: > On Thu, Jun 27, 2019 at 02:42:43PM -0400, Kenny Ho wrote: > > 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. Um... perhaps I was being overly conservative :). So let me illustrate with an example to add more clarity and get more comments on it. Let say we have the following cgroup hierarchy (The letters are cgroups with R being the root cgroup. The numbers in brackets are processes. The processes are placed with the 'No Internal Process Constraint' in mind.) R (4, 5) -- A (6) \ B C (7,8) \ D (9) Here is a list of operation and the associated effect on the size track by the cgroups (for simplicity, each buffer is 1 unit in size.) With current implementation (charge on buffer creation with restriction on sharing.) R A B C D |Ops 1 0 0 0 0 |4 allocated a buffer 1 0 0 0 0 |4 shared a buffer with 5 1 0 0 0 0 |4 shared a buffer with 9 2 0 1 0 1 |9 allocated a buffer 3 0 2 1 1 |7 allocated a buffer 3 0 2 1 1 |7 shared a buffer with 8 3 0 2 1 1 |7 sharing with 9 (not allowed) 3 0 2 1 1 |7 sharing with 4 (not allowed) 3 0 2 1 1 |7 release a buffer 2 0 1 0 1 |8 release a buffer from 7 The suggestion as I understand it (charge per buffer reference with unrestricted sharing.) R A B C D |Ops 1 0 0 0 0 |4 allocated a buffer 2 0 0 0 0 |4 shared a buffer with 5 3 0 0 0 1 |4 shared a buffer with 9 4 0 1 0 2 |9 allocated a buffer 5 0 2 1 1 |7 allocated a buffer 6 0 3 2 1 |7 shared a buffer with 8 7 0 4 2 2 |7 sharing with 9 8 0 4 2 2 |7 sharing with 4 7 0 3 1 2 |7 release a buffer 6 0 2 0 2 |8 release a buffer from 7 Is this a correct understanding of the suggestion? Regards, Kenny ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v3 04/11] drm, cgroup: Add total GEM buffer allocation limit
On Thu, Jun 27, 2019 at 02:42:43PM -0400, Kenny Ho wrote: > On Thu, Jun 27, 2019 at 1:43 AM Daniel Vetter 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v3 04/11] drm, cgroup: Add total GEM buffer allocation limit
On Thu, Jun 27, 2019 at 1:43 AM Daniel Vetter 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. > - 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. Regards, Kenny ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v3 04/11] drm, cgroup: Add total GEM buffer allocation limit
On Wed, Jun 26, 2019 at 06:41:32PM -0400, Kenny Ho wrote: > On Wed, Jun 26, 2019 at 5:41 PM Daniel Vetter wrote: > > On Wed, Jun 26, 2019 at 05:27:48PM -0400, Kenny Ho wrote: > > > On Wed, Jun 26, 2019 at 12:05 PM Daniel Vetter wrote: > > > > So what happens when you start a lot of threads all at the same time, > > > > allocating gem bo? Also would be nice if we could roll out at least the > > > > accounting part of this cgroup to all GEM drivers. > > > > > > When there is a large number of allocation, the allocation will be > > > checked in sequence within a device (since I used a per device mutex > > > in the check.) Are you suggesting the overhead here is significant > > > enough to be a bottleneck? The accounting part should be available to > > > all GEM drivers (unless I missed something) since the chg and unchg > > > function is called via the generic drm_gem_private_object_init and > > > drm_gem_object_release. > > > > thread 1: checks limits, still under the total > > > > thread 2: checks limits, still under the total > > > > thread 1: allocates, still under > > > > thread 2: allocates, now over the limit > > > > I think the check and chg need to be one step, or this wont work. Or I'm > > missing something somewhere. > > Ok, I see what you are saying. > > > Wrt rolling out the accounting for all drivers: Since you also roll out > > enforcement in this patch I'm not sure whether the accounting part is > > fully stand-alone. And as discussed a bit on an earlier patch, I think for > > DRIVER_GEM we should set up the accounting cgroup automatically. > I think I should be able to split the commit and restructure things a bit. > > > > > What's the underlying technical reason for not allowing sharing across > > > > cgroups? > > > To be clear, sharing across cgroup is allowed, the buffer just needs > > > to be allocated by a process that is parent to the cgroup. So in the > > > case of xorg allocating buffer for client, the xorg would be in the > > > root cgroup and the buffer can be passed around by different clients > > > (in root or other cgroup.) The idea here is to establish some form of > > > ownership, otherwise there wouldn't be a way to account for or limit > > > the usage. > > > > But why? What's the problem if I allocate something and then hand it to > > someone else. E.g. one popular use of cgroups is to isolate clients, so > > maybe you'd do a cgroup + namespace for each X11 client (ok wayland, with > > X11 this is probably pointless). > > > > But with your current limitation those clients can't pass buffers to the > > compositor anymore, making cgroups useless. Your example here only works > > if Xorg is in the root and allocates all the buffers. That's not even true > > for DRI3 anymore. > > > > So pretty serious limitation on cgroups, and I'm not really understanding > > why we need this. I think if we want to prevent buffer sharing, what we > > need are some selinux hooks and stuff so you can prevent an import/access > > by someone who's not allowed to touch a buffer. But that kind of access > > right management should be separate from resource control imo. > 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 ... - 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 ... Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v3 04/11] drm, cgroup: Add total GEM buffer allocation limit
On Wed, Jun 26, 2019 at 5:41 PM Daniel Vetter wrote: > On Wed, Jun 26, 2019 at 05:27:48PM -0400, Kenny Ho wrote: > > On Wed, Jun 26, 2019 at 12:05 PM Daniel Vetter wrote: > > > So what happens when you start a lot of threads all at the same time, > > > allocating gem bo? Also would be nice if we could roll out at least the > > > accounting part of this cgroup to all GEM drivers. > > > > When there is a large number of allocation, the allocation will be > > checked in sequence within a device (since I used a per device mutex > > in the check.) Are you suggesting the overhead here is significant > > enough to be a bottleneck? The accounting part should be available to > > all GEM drivers (unless I missed something) since the chg and unchg > > function is called via the generic drm_gem_private_object_init and > > drm_gem_object_release. > > thread 1: checks limits, still under the total > > thread 2: checks limits, still under the total > > thread 1: allocates, still under > > thread 2: allocates, now over the limit > > I think the check and chg need to be one step, or this wont work. Or I'm > missing something somewhere. Ok, I see what you are saying. > Wrt rolling out the accounting for all drivers: Since you also roll out > enforcement in this patch I'm not sure whether the accounting part is > fully stand-alone. And as discussed a bit on an earlier patch, I think for > DRIVER_GEM we should set up the accounting cgroup automatically. I think I should be able to split the commit and restructure things a bit. > > > What's the underlying technical reason for not allowing sharing across > > > cgroups? > > To be clear, sharing across cgroup is allowed, the buffer just needs > > to be allocated by a process that is parent to the cgroup. So in the > > case of xorg allocating buffer for client, the xorg would be in the > > root cgroup and the buffer can be passed around by different clients > > (in root or other cgroup.) The idea here is to establish some form of > > ownership, otherwise there wouldn't be a way to account for or limit > > the usage. > > But why? What's the problem if I allocate something and then hand it to > someone else. E.g. one popular use of cgroups is to isolate clients, so > maybe you'd do a cgroup + namespace for each X11 client (ok wayland, with > X11 this is probably pointless). > > But with your current limitation those clients can't pass buffers to the > compositor anymore, making cgroups useless. Your example here only works > if Xorg is in the root and allocates all the buffers. That's not even true > for DRI3 anymore. > > So pretty serious limitation on cgroups, and I'm not really understanding > why we need this. I think if we want to prevent buffer sharing, what we > need are some selinux hooks and stuff so you can prevent an import/access > by someone who's not allowed to touch a buffer. But that kind of access > right management should be separate from resource control imo. 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? Regards, Kenny ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v3 04/11] drm, cgroup: Add total GEM buffer allocation limit
On Wed, Jun 26, 2019 at 05:27:48PM -0400, Kenny Ho wrote: > On Wed, Jun 26, 2019 at 12:05 PM Daniel Vetter wrote: > > > > > drm.buffer.default > > > A read-only flat-keyed file which exists on the root cgroup. > > > Each entry is keyed by the drm device's major:minor. > > > > > > Default limits on the total GEM buffer allocation in bytes. > > > > Don't we need a "0 means no limit" semantics here? > > I believe the convention is to use the 'max' keyword. > > > > > I think we need a new drm-cgroup.rst which contains all this > > documentation. > > Yes I planned to do that when things are more finalized. I am > actually writing the commit message following the current doc format > so I can reuse it in the rst. Awesome. > > With multiple GPUs, do we need an overall GEM bo limit, across all gpus? > > For other stuff later on like vram/tt/... and all that it needs to be > > per-device, but I think one overall limit could be useful. > > This one I am not sure but should be fairly straightforward to add. > I'd love to hear more feedbacks on this as well. > > > > if (!amdgpu_bo_validate_size(adev, size, bp->domain)) > > > return -ENOMEM; > > > > > > + if (!drmcgrp_bo_can_allocate(current, adev->ddev, size)) > > > + return -ENOMEM; > > > > So what happens when you start a lot of threads all at the same time, > > allocating gem bo? Also would be nice if we could roll out at least the > > accounting part of this cgroup to all GEM drivers. > > When there is a large number of allocation, the allocation will be > checked in sequence within a device (since I used a per device mutex > in the check.) Are you suggesting the overhead here is significant > enough to be a bottleneck? The accounting part should be available to > all GEM drivers (unless I missed something) since the chg and unchg > function is called via the generic drm_gem_private_object_init and > drm_gem_object_release. thread 1: checks limits, still under the total thread 2: checks limits, still under the total thread 1: allocates, still under thread 2: allocates, now over the limit I think the check and chg need to be one step, or this wont work. Or I'm missing something somewhere. Wrt rolling out the accounting for all drivers: Since you also roll out enforcement in this patch I'm not sure whether the accounting part is fully stand-alone. And as discussed a bit on an earlier patch, I think for DRIVER_GEM we should set up the accounting cgroup automatically. > > > + /* only allow bo from the same cgroup or its ancestor to be > > > imported */ > > > + if (drmcgrp != NULL && > > > > Quite a serious limitation here ... > > > > > + !drmcgrp_is_self_or_ancestor(drmcgrp, > > > obj->drmcgrp)) { > > > > Also what happens if you actually share across devices? Then importing in > > the 2nd group is suddenly possible, and I think will be double-counted. > > > > What's the underlying technical reason for not allowing sharing across > > cgroups? > > With the current implementation, there shouldn't be double counting as > the counting is done during the buffer init. If you share across devices there will be two drm_gem_obect structures, on on each device. But only one underlying bo. Now the bo limit is per-device too, so that's all fine, but for a global bo limit we'd need to make sure we count these only once. > To be clear, sharing across cgroup is allowed, the buffer just needs > to be allocated by a process that is parent to the cgroup. So in the > case of xorg allocating buffer for client, the xorg would be in the > root cgroup and the buffer can be passed around by different clients > (in root or other cgroup.) The idea here is to establish some form of > ownership, otherwise there wouldn't be a way to account for or limit > the usage. But why? What's the problem if I allocate something and then hand it to someone else. E.g. one popular use of cgroups is to isolate clients, so maybe you'd do a cgroup + namespace for each X11 client (ok wayland, with X11 this is probably pointless). But with your current limitation those clients can't pass buffers to the compositor anymore, making cgroups useless. Your example here only works if Xorg is in the root and allocates all the buffers. That's not even true for DRI3 anymore. So pretty serious limitation on cgroups, and I'm not really understanding why we need this. I think if we want to prevent buffer sharing, what we need are some selinux hooks and stuff so you can prevent an import/access by someone who's not allowed to touch a buffer. But that kind of access right management should be separate from resource control imo. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v3 04/11] drm, cgroup: Add total GEM buffer allocation limit
On Wed, Jun 26, 2019 at 12:05 PM Daniel Vetter wrote: > > > drm.buffer.default > > A read-only flat-keyed file which exists on the root cgroup. > > Each entry is keyed by the drm device's major:minor. > > > > Default limits on the total GEM buffer allocation in bytes. > > Don't we need a "0 means no limit" semantics here? I believe the convention is to use the 'max' keyword. > > I think we need a new drm-cgroup.rst which contains all this > documentation. Yes I planned to do that when things are more finalized. I am actually writing the commit message following the current doc format so I can reuse it in the rst. > > With multiple GPUs, do we need an overall GEM bo limit, across all gpus? > For other stuff later on like vram/tt/... and all that it needs to be > per-device, but I think one overall limit could be useful. This one I am not sure but should be fairly straightforward to add. I'd love to hear more feedbacks on this as well. > > if (!amdgpu_bo_validate_size(adev, size, bp->domain)) > > return -ENOMEM; > > > > + if (!drmcgrp_bo_can_allocate(current, adev->ddev, size)) > > + return -ENOMEM; > > So what happens when you start a lot of threads all at the same time, > allocating gem bo? Also would be nice if we could roll out at least the > accounting part of this cgroup to all GEM drivers. When there is a large number of allocation, the allocation will be checked in sequence within a device (since I used a per device mutex in the check.) Are you suggesting the overhead here is significant enough to be a bottleneck? The accounting part should be available to all GEM drivers (unless I missed something) since the chg and unchg function is called via the generic drm_gem_private_object_init and drm_gem_object_release. > > + /* only allow bo from the same cgroup or its ancestor to be imported > > */ > > + if (drmcgrp != NULL && > > Quite a serious limitation here ... > > > + !drmcgrp_is_self_or_ancestor(drmcgrp, obj->drmcgrp)) { > > Also what happens if you actually share across devices? Then importing in > the 2nd group is suddenly possible, and I think will be double-counted. > > What's the underlying technical reason for not allowing sharing across > cgroups? With the current implementation, there shouldn't be double counting as the counting is done during the buffer init. To be clear, sharing across cgroup is allowed, the buffer just needs to be allocated by a process that is parent to the cgroup. So in the case of xorg allocating buffer for client, the xorg would be in the root cgroup and the buffer can be passed around by different clients (in root or other cgroup.) The idea here is to establish some form of ownership, otherwise there wouldn't be a way to account for or limit the usage. Regards, Kenny ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v3 04/11] drm, cgroup: Add total GEM buffer allocation limit
On Wed, Jun 26, 2019 at 11:05:15AM -0400, Kenny Ho wrote: > The drm resource being measured and limited here is the GEM buffer > objects. User applications allocate and free these buffers. In > addition, a process can allocate a buffer and share it with another > process. The consumer of a shared buffer can also outlive the > allocator of the buffer. > > For the purpose of cgroup accounting and limiting, ownership of the > buffer is deemed to be the cgroup for which the allocating process > belongs to. There is one cgroup limit per drm device. > > In order to prevent the buffer outliving the cgroup that owns it, a > process is prevented from importing buffers that are not own by the > process' cgroup or the ancestors of the process' cgroup. In other > words, in order for a buffer to be shared between two cgroups, the > buffer must be created by the common ancestors of the cgroups. > > drm.buffer.stats > A read-only flat-keyed file which exists on all cgroups. Each > entry is keyed by the drm device's major:minor. > > Total GEM buffer allocation in bytes. > > drm.buffer.default > A read-only flat-keyed file which exists on the root cgroup. > Each entry is keyed by the drm device's major:minor. > > Default limits on the total GEM buffer allocation in bytes. Don't we need a "0 means no limit" semantics here? > drm.buffer.max > A read-write flat-keyed file which exists on all cgroups. Each > entry is keyed by the drm device's major:minor. > > Per device limits on the total GEM buffer allocation in byte. > This is a hard limit. Attempts in allocating beyond the cgroup > limit will result in ENOMEM. Shorthand understood by memparse > (such as k, m, g) can be used. > > Set allocation limit for /dev/dri/card1 to 1GB > echo "226:1 1g" > drm.buffer.total.max > > Set allocation limit for /dev/dri/card0 to 512MB > echo "226:0 512m" > drm.buffer.total.max I think we need a new drm-cgroup.rst which contains all this documentation. With multiple GPUs, do we need an overall GEM bo limit, across all gpus? For other stuff later on like vram/tt/... and all that it needs to be per-device, but I think one overall limit could be useful. > > Change-Id: I4c249d06d45ec709d6481d4cbe87c5168545c5d0 > Signed-off-by: Kenny Ho > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 4 + > drivers/gpu/drm/drm_gem.c | 8 + > drivers/gpu/drm/drm_prime.c| 9 + > include/drm/drm_cgroup.h | 34 ++- > include/drm/drm_gem.h | 11 + > include/linux/cgroup_drm.h | 2 + > kernel/cgroup/drm.c| 321 + > 7 files changed, 387 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c > index 93b2c5a48a71..b4c078b7ad63 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c > @@ -34,6 +34,7 @@ > #include > #include > #include > +#include > #include "amdgpu.h" > #include "amdgpu_trace.h" > #include "amdgpu_amdkfd.h" > @@ -446,6 +447,9 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev, > if (!amdgpu_bo_validate_size(adev, size, bp->domain)) > return -ENOMEM; > > + if (!drmcgrp_bo_can_allocate(current, adev->ddev, size)) > + return -ENOMEM; So what happens when you start a lot of threads all at the same time, allocating gem bo? Also would be nice if we could roll out at least the accounting part of this cgroup to all GEM drivers. > + > *bo_ptr = NULL; > > acc_size = ttm_bo_dma_acc_size(>mman.bdev, size, > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 6a80db077dc6..e20c1034bf2b 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -37,10 +37,12 @@ > #include > #include > #include > +#include > #include > #include > #include > #include > +#include > #include "drm_internal.h" > > /** @file drm_gem.c > @@ -154,6 +156,9 @@ void drm_gem_private_object_init(struct drm_device *dev, > obj->handle_count = 0; > obj->size = size; > drm_vma_node_reset(>vma_node); > + > + obj->drmcgrp = get_drmcgrp(current); > + drmcgrp_chg_bo_alloc(obj->drmcgrp, dev, size); > } > EXPORT_SYMBOL(drm_gem_private_object_init); > > @@ -804,6 +809,9 @@ drm_gem_object_release(struct drm_gem_object *obj) > if (obj->filp) > fput(obj->filp); > > + drmcgrp_unchg_bo_alloc(obj->drmcgrp, obj->dev, obj->size); > + put_drmcgrp(obj->drmcgrp); > + > drm_gem_free_mmap_offset(obj); > } > EXPORT_SYMBOL(drm_gem_object_release); > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index 231e3f6d5f41..eeb612116810 100644 > ---