| Message ID | 20250310-dmem-cgroups-v1-0-2984c1bc9312@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | dma: Enable dmem cgroup tracking | expand |
On Mon, Mar 10, 2025 at 01:06:06PM +0100, Maxime Ripard wrote: > Here's preliminary work to enable dmem tracking for heavy users of DMA > allocations on behalf of userspace: v4l2, DRM, and dma-buf heaps. > > It's not really meant for inclusion at the moment, because I really > don't like it that much, and would like to discuss solutions on how to > make it nicer. > > In particular, the dma dmem region accessors don't feel that great to > me. It duplicates the logic to select the proper accessor in > dma_alloc_attrs(), and it looks fragile and potentially buggy to me. > > One solution I tried is to do the accounting in dma_alloc_attrs() > directly, depending on a flag being set, similar to what __GFP_ACCOUNT > is doing. > > It didn't work because dmem initialises a state pointer when charging an > allocation to a region, and expects that state pointer to be passed back > when uncharging. Since dma_alloc_attrs() returns a void pointer to the > allocated buffer, we need to put that state into a higher-level > structure, such as drm_gem_object, or dma_buf. > > Since we can't share the region selection logic, we need to get the > region through some other mean. Another thing I consider was to return > the region as part of the allocated buffer (through struct page or > folio), but those are lost across the calls and dma_alloc_attrs() will > only get a void pointer. So that's not doable without some heavy > rework, if it's a good idea at all. One thing I forgot to mention is that it makes it harder than it could for subsystems that can allocate from multiple allocators (like... all the ones included in this series at least). I only added proper tracking in the backends using dma_alloc_attrs(), but they also support vmalloc. In what region vmalloc allocations should be tracked (if any) is an open-question to me. Similarly, some use dma_alloc_noncontiguous(). Also, I've set the size of the "default" DMA allocation region to U64_MAX, but that's obviously wrong and will break any relative metric. I'm not sure what would be the correct size though. Maxime
[Adding Ben since we are currently in the middle of a discussion regarding exactly that problem] Just for my understanding before I deep dive into the code: This uses a separate dmem cgroup and does not account against memcg, don't it? Thanks, Christian. Am 10.03.25 um 13:06 schrieb Maxime Ripard: > Hi, > > Here's preliminary work to enable dmem tracking for heavy users of DMA > allocations on behalf of userspace: v4l2, DRM, and dma-buf heaps. > > It's not really meant for inclusion at the moment, because I really > don't like it that much, and would like to discuss solutions on how to > make it nicer. > > In particular, the dma dmem region accessors don't feel that great to > me. It duplicates the logic to select the proper accessor in > dma_alloc_attrs(), and it looks fragile and potentially buggy to me. > > One solution I tried is to do the accounting in dma_alloc_attrs() > directly, depending on a flag being set, similar to what __GFP_ACCOUNT > is doing. > > It didn't work because dmem initialises a state pointer when charging an > allocation to a region, and expects that state pointer to be passed back > when uncharging. Since dma_alloc_attrs() returns a void pointer to the > allocated buffer, we need to put that state into a higher-level > structure, such as drm_gem_object, or dma_buf. > > Since we can't share the region selection logic, we need to get the > region through some other mean. Another thing I consider was to return > the region as part of the allocated buffer (through struct page or > folio), but those are lost across the calls and dma_alloc_attrs() will > only get a void pointer. So that's not doable without some heavy > rework, if it's a good idea at all. > > So yeah, I went for the dumbest possible solution with the accessors, > hoping you could suggest a much smarter idea :) > > Thanks, > Maxime > > Signed-off-by: Maxime Ripard <mripard@kernel.org> > --- > Maxime Ripard (12): > cma: Register dmem region for each cma region > cma: Provide accessor to cma dmem region > dma: coherent: Register dmem region for each coherent region > dma: coherent: Provide accessor to dmem region > dma: contiguous: Provide accessor to dmem region > dma: direct: Provide accessor to dmem region > dma: Create default dmem region for DMA allocations > dma: Provide accessor to dmem region > dma-buf: Clear cgroup accounting on release > dma-buf: cma: Account for allocations in dmem cgroup > drm/gem: Add cgroup memory accounting > media: videobuf2: Track buffer allocations through the dmem cgroup > > drivers/dma-buf/dma-buf.c | 7 ++++ > drivers/dma-buf/heaps/cma_heap.c | 18 ++++++++-- > drivers/gpu/drm/drm_gem.c | 5 +++ > drivers/gpu/drm/drm_gem_dma_helper.c | 6 ++++ > .../media/common/videobuf2/videobuf2-dma-contig.c | 19 +++++++++++ > include/drm/drm_device.h | 1 + > include/drm/drm_gem.h | 2 ++ > include/linux/cma.h | 9 +++++ > include/linux/dma-buf.h | 5 +++ > include/linux/dma-direct.h | 2 ++ > include/linux/dma-map-ops.h | 32 ++++++++++++++++++ > include/linux/dma-mapping.h | 11 ++++++ > kernel/dma/coherent.c | 26 +++++++++++++++ > kernel/dma/direct.c | 8 +++++ > kernel/dma/mapping.c | 39 ++++++++++++++++++++++ > mm/cma.c | 21 +++++++++++- > mm/cma.h | 3 ++ > 17 files changed, 211 insertions(+), 3 deletions(-) > --- > base-commit: 55a2aa61ba59c138bd956afe0376ec412a7004cf > change-id: 20250307-dmem-cgroups-73febced0989 > > Best regards,
Hi, On Mon, Mar 10, 2025 at 03:16:53PM +0100, Christian König wrote: > [Adding Ben since we are currently in the middle of a discussion > regarding exactly that problem] > > Just for my understanding before I deep dive into the code: This uses > a separate dmem cgroup and does not account against memcg, don't it? Yes. The main rationale being that it doesn't always make sense to register against memcg: a lot of devices are going to allocate from dedicated chunks of memory that are either carved out from the main memory allocator, or not under Linux supervision at all. And if there's no way to make it consistent across drivers, it's not the right tool. Maxime
Hi, Here's preliminary work to enable dmem tracking for heavy users of DMA allocations on behalf of userspace: v4l2, DRM, and dma-buf heaps. It's not really meant for inclusion at the moment, because I really don't like it that much, and would like to discuss solutions on how to make it nicer. In particular, the dma dmem region accessors don't feel that great to me. It duplicates the logic to select the proper accessor in dma_alloc_attrs(), and it looks fragile and potentially buggy to me. One solution I tried is to do the accounting in dma_alloc_attrs() directly, depending on a flag being set, similar to what __GFP_ACCOUNT is doing. It didn't work because dmem initialises a state pointer when charging an allocation to a region, and expects that state pointer to be passed back when uncharging. Since dma_alloc_attrs() returns a void pointer to the allocated buffer, we need to put that state into a higher-level structure, such as drm_gem_object, or dma_buf. Since we can't share the region selection logic, we need to get the region through some other mean. Another thing I consider was to return the region as part of the allocated buffer (through struct page or folio), but those are lost across the calls and dma_alloc_attrs() will only get a void pointer. So that's not doable without some heavy rework, if it's a good idea at all. So yeah, I went for the dumbest possible solution with the accessors, hoping you could suggest a much smarter idea :) Thanks, Maxime Signed-off-by: Maxime Ripard <mripard@kernel.org> --- Maxime Ripard (12): cma: Register dmem region for each cma region cma: Provide accessor to cma dmem region dma: coherent: Register dmem region for each coherent region dma: coherent: Provide accessor to dmem region dma: contiguous: Provide accessor to dmem region dma: direct: Provide accessor to dmem region dma: Create default dmem region for DMA allocations dma: Provide accessor to dmem region dma-buf: Clear cgroup accounting on release dma-buf: cma: Account for allocations in dmem cgroup drm/gem: Add cgroup memory accounting media: videobuf2: Track buffer allocations through the dmem cgroup drivers/dma-buf/dma-buf.c | 7 ++++ drivers/dma-buf/heaps/cma_heap.c | 18 ++++++++-- drivers/gpu/drm/drm_gem.c | 5 +++ drivers/gpu/drm/drm_gem_dma_helper.c | 6 ++++ .../media/common/videobuf2/videobuf2-dma-contig.c | 19 +++++++++++ include/drm/drm_device.h | 1 + include/drm/drm_gem.h | 2 ++ include/linux/cma.h | 9 +++++ include/linux/dma-buf.h | 5 +++ include/linux/dma-direct.h | 2 ++ include/linux/dma-map-ops.h | 32 ++++++++++++++++++ include/linux/dma-mapping.h | 11 ++++++ kernel/dma/coherent.c | 26 +++++++++++++++ kernel/dma/direct.c | 8 +++++ kernel/dma/mapping.c | 39 ++++++++++++++++++++++ mm/cma.c | 21 +++++++++++- mm/cma.h | 3 ++ 17 files changed, 211 insertions(+), 3 deletions(-) --- base-commit: 55a2aa61ba59c138bd956afe0376ec412a7004cf change-id: 20250307-dmem-cgroups-73febced0989 Best regards,