* [PATCH 1/7] kernel/cgroup: Add "dev" memory accounting cgroup
2024-10-23 7:52 [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup Maarten Lankhorst
@ 2024-10-23 7:52 ` Maarten Lankhorst
2024-10-23 15:26 ` Waiman Long
` (2 more replies)
2024-10-23 7:52 ` [PATCH 2/7] drm/drv: Add drmm cgroup registration for dev cgroups Maarten Lankhorst
` (6 subsequent siblings)
7 siblings, 3 replies; 27+ messages in thread
From: Maarten Lankhorst @ 2024-10-23 7:52 UTC (permalink / raw)
To: intel-xe, linux-kernel, dri-devel, Tejun Heo, Zefan Li,
Johannes Weiner, Andrew Morton
Cc: Friedrich Vock, cgroups, linux-mm, Maxime Ripard, Maarten Lankhorst
The initial version was based roughly on the rdma and misc cgroup
controllers, with a lot of the accounting code borrowed from rdma.
The current version is a complete rewrite with page counter; it uses
the same min/low/max semantics as the memory cgroup as a result.
There's a small mismatch as TTM uses u64, and page_counter long pages.
In practice it's not a problem. 32-bits systems don't really come with
>=4GB cards and as long as we're consistently wrong with units, it's
fine. The device page size may not be in the same units as kernel page
size, and each region might also have a different page size (VRAM vs GART
for example).
The interface is simple:
- populate dev_cgroup_try_charge->regions[..] name and size for each active
region, set num_regions accordingly.
- Call (dev,drmm)_cgroup_register_device()
- Use dev_cgroup_try_charge to check if you can allocate a chunk of memory,
use dev_cgroup__uncharge when freeing it. This may return an error code,
or -EAGAIN when the cgroup limit is reached. In that case a reference
to the limiting pool is returned.
- The limiting cs can be used as compare function for
dev_cgroup_state_evict_valuable.
- After having evicted enough, drop reference to limiting cs with
dev_cgroup_pool_state_put.
This API allows you to limit device resources with cgroups.
You can see the supported cards in /sys/fs/cgroup/dev.region.capacity
You need to echo +dev to cgroup.subtree_control, and then you can
partition memory.
Co-developed-by: Friedrich Vock <friedrich.vock@gmx.de>
Signed-off-by: Friedrich Vock <friedrich.vock@gmx.de>
Co-developed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
Documentation/admin-guide/cgroup-v2.rst | 51 ++
Documentation/core-api/cgroup.rst | 9 +
Documentation/core-api/index.rst | 1 +
Documentation/gpu/drm-compute.rst | 54 ++
include/linux/cgroup_dev.h | 91 +++
include/linux/cgroup_subsys.h | 4 +
include/linux/page_counter.h | 2 +-
init/Kconfig | 7 +
kernel/cgroup/Makefile | 1 +
kernel/cgroup/dev.c | 893 ++++++++++++++++++++++++
mm/page_counter.c | 4 +-
11 files changed, 1114 insertions(+), 3 deletions(-)
create mode 100644 Documentation/core-api/cgroup.rst
create mode 100644 Documentation/gpu/drm-compute.rst
create mode 100644 include/linux/cgroup_dev.h
create mode 100644 kernel/cgroup/dev.c
diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
index 69af2173555fb..e8fe79244af9c 100644
--- a/Documentation/admin-guide/cgroup-v2.rst
+++ b/Documentation/admin-guide/cgroup-v2.rst
@@ -2612,6 +2612,57 @@ RDMA Interface Files
mlx4_0 hca_handle=1 hca_object=20
ocrdma1 hca_handle=1 hca_object=23
+DEV
+----
+
+The "dev" controller regulates the distribution and accounting of
+device resources, currently only memory regions. Because each memory
+region may have its own page size, which does not have to be equal
+to the system page size. the units are in bytes.
+
+DEV Interface Files
+~~~~~~~~~~~~~~~~~~~~
+
+ dev.region.max, dev.region.min, dev.region.low
+ A readwrite nested-keyed file that exists for all the cgroups
+ except root that describes current configured resource limit
+ for a device.
+
+ Lines are keyed by device name and are not ordered.
+ Each line contains space separated resource name and its configured
+ limit that can be distributed.
+
+ The following nested keys are defined.
+
+ ========== =======================================================
+ * Maximum amount of bytes that allocatable in this region
+ ========== =======================================================
+
+ An example for xe follows::
+
+ drm/0000:03:00.0 vram0=1073741824 stolen=max
+
+ The semantics are the same as for the memory cgroup controller, and are
+ calculated in the same way.
+
+ dev.region.capacity
+ A read-only file that describes maximum region capacity.
+ It only exists on the root cgroup. Not all memory can be
+ allocated by cgroups, as the kernel reserves some for
+ internal use.
+
+ An example for xe follows::
+
+ drm/0000:03:00.0 vram0=8514437120 stolen=67108864
+
+ dev.region.current
+ A read-only file that describes current resource usage.
+ It exists for all the cgroup except root.
+
+ An example for xe follows::
+
+ drm/0000:03:00.0 vram0=12550144 stolen=8650752
+
HugeTLB
-------
diff --git a/Documentation/core-api/cgroup.rst b/Documentation/core-api/cgroup.rst
new file mode 100644
index 0000000000000..475b32255bd68
--- /dev/null
+++ b/Documentation/core-api/cgroup.rst
@@ -0,0 +1,9 @@
+==================
+Cgroup Kernel APIs
+==================
+
+Device Cgroup API (devcg)
+=========================
+.. kernel-doc:: kernel/cgroup/dev.c
+ :export:
+
diff --git a/Documentation/core-api/index.rst b/Documentation/core-api/index.rst
index 6a875743dd4b7..dbd6c4f9a6313 100644
--- a/Documentation/core-api/index.rst
+++ b/Documentation/core-api/index.rst
@@ -108,6 +108,7 @@ more memory-management documentation in Documentation/mm/index.rst.
dma-isa-lpc
swiotlb
mm-api
+ cgroup
genalloc
pin_user_pages
boot-time-mm
diff --git a/Documentation/gpu/drm-compute.rst b/Documentation/gpu/drm-compute.rst
new file mode 100644
index 0000000000000..116270976ef7a
--- /dev/null
+++ b/Documentation/gpu/drm-compute.rst
@@ -0,0 +1,54 @@
+==================================
+Long running workloads and compute
+==================================
+
+Long running workloads (compute) are workloads that will not complete in 10
+seconds. (The time let the user wait before he reaches for the power button).
+This means that other techniques need to be used to manage those workloads,
+that cannot use fences.
+
+Some hardware may schedule compute jobs, and have no way to pre-empt them, or
+have their memory swapped out from them. Or they simply want their workload
+not to be preempted or swapped out at all.
+
+This means that it differs from what is described in driver-api/dma-buf.rst.
+
+As with normal compute jobs, dma-fence may not be used at all. In this case,
+not even to force preemption. The driver with is simply forced to unmap a BO
+from the long compute job's address space on unbind immediately, not even
+waiting for the workload to complete. Effectively this terminates the workload
+when there is no hardware support to recover.
+
+Since this is undesirable, there need to be mitigations to prevent a workload
+from being terminated. There are several possible approach, all with their
+advantages and drawbacks.
+
+The first approach you will likely try is to pin all buffers used by compute.
+This guarantees that the job will run uninterrupted, but also allows a very
+denial of service attack by pinning as much memory as possible, hogging the
+all GPU memory, and possibly a huge chunk of CPU memory.
+
+A second approach that will work slightly better on its own is adding an option
+not to evict when creating a new job (any kind). If all of userspace opts in
+to this flag, it would prevent cooperating userspace from forced terminating
+older compute jobs to start a new one.
+
+If job preemption and recoverable pagefaults are not available, those are the
+only approaches possible. So even with those, you want a separate way of
+controlling resources. The standard kernel way of doing so is cgroups.
+
+This creates a third option, using cgroups to prevent eviction. Both GPU and
+driver-allocated CPU memory would be accounted to the correct cgroup, and
+eviction would be made cgroup aware. This allows the GPU to be partitioned
+into cgroups, that will allow jobs to run next to each other without
+interference.
+
+The interface to the cgroup would be similar to the current CPU memory
+interface, with similar semantics for min/low/high/max, if eviction can
+be made cgroup aware. For now only max is implemented.
+
+What should be noted is that each memory region (tiled memory for example)
+should have its own accounting, using $card key0 = value0 key1 = value1.
+
+The key is set to the regionid set by the driver, for example "tile0".
+For the value of $card, we use drmGetUnique().
diff --git a/include/linux/cgroup_dev.h b/include/linux/cgroup_dev.h
new file mode 100644
index 0000000000000..c6311d1d3ce48
--- /dev/null
+++ b/include/linux/cgroup_dev.h
@@ -0,0 +1,91 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+
+#ifndef _CGROUP_DEV_H
+#define _CGROUP_DEV_H
+
+#include <linux/types.h>
+#include <linux/llist.h>
+
+struct dev_cgroup_pool_state;
+
+/*
+ * Use 8 as max, because of N^2 lookup when setting things, can be bumped if needed
+ * Identical to TTM_NUM_MEM_TYPES to allow simplifying that code.
+ */
+#define DEVICE_CGROUP_MAX_REGIONS 8
+
+/* Public definition of cgroup device, should not be modified after _register() */
+struct dev_cgroup_device {
+ struct {
+ u64 size;
+ const char *name;
+ } regions[DEVICE_CGROUP_MAX_REGIONS];
+
+ int num_regions;
+
+ /* used by cgroups, do not use */
+ void *priv;
+};
+
+#if IS_ENABLED(CONFIG_CGROUP_DEV)
+int dev_cgroup_register_device(struct dev_cgroup_device *cgdev,
+ const char *name);
+void dev_cgroup_unregister_device(struct dev_cgroup_device *cgdev);
+int dev_cgroup_try_charge(struct dev_cgroup_device *cgdev,
+ u32 index, u64 size,
+ struct dev_cgroup_pool_state **ret_pool,
+ struct dev_cgroup_pool_state **ret_limit_pool);
+void dev_cgroup_uncharge(struct dev_cgroup_pool_state *pool,
+ u32 index, u64 size);
+bool dev_cgroup_state_evict_valuable(struct dev_cgroup_device *dev, int index,
+ struct dev_cgroup_pool_state *limit_pool,
+ struct dev_cgroup_pool_state *test_pool,
+ bool ignore_low, bool *ret_hit_low);
+
+void dev_cgroup_pool_state_put(struct dev_cgroup_pool_state *pool);
+#else
+static inline int
+dev_cgroup_register_device(struct dev_cgroup_device *cgdev,
+ const char *name)
+{
+ return 0;
+}
+
+static inline void dev_cgroup_unregister_device(struct dev_cgroup_device *cgdev)
+{
+}
+
+static int int dev_cgroup_try_charge(struct dev_cgroup_device *cgdev,
+ u32 index, u64 size,
+ struct dev_cgroup_pool_state **ret_pool,
+ struct dev_cgroup_pool_state **ret_limit_pool);
+{
+ *ret_pool = NULL;
+
+ if (ret_limit_pool)
+ *ret_limit_pool = NULL;
+
+ return 0;
+}
+
+static inline void dev_cgroup_uncharge(struct dev_cgroup_pool_state *pool,
+ u32 index, u64 size)
+{ }
+
+static inline
+bool dev_cgroup_state_evict_valuable(struct dev_cgroup_device *dev, int index,
+ struct dev_cgroup_pool_state *limit_pool,
+ struct dev_cgroup_pool_state *test_pool,
+ bool ignore_low, bool *ret_hit_low)
+{
+ return true;
+}
+
+static inline void dev_cgroup_pool_state_put(struct dev_cgroup_pool_state *pool)
+{ }
+
+#endif
+#endif /* _CGROUP_DEV_H */
diff --git a/include/linux/cgroup_subsys.h b/include/linux/cgroup_subsys.h
index 4452354872307..898340cfe5843 100644
--- a/include/linux/cgroup_subsys.h
+++ b/include/linux/cgroup_subsys.h
@@ -65,6 +65,10 @@ SUBSYS(rdma)
SUBSYS(misc)
#endif
+#if IS_ENABLED(CONFIG_CGROUP_DEV)
+SUBSYS(dev)
+#endif
+
/*
* The following subsystems are not supported on the default hierarchy.
*/
diff --git a/include/linux/page_counter.h b/include/linux/page_counter.h
index 79dbd8bc35a72..d75376a1694ee 100644
--- a/include/linux/page_counter.h
+++ b/include/linux/page_counter.h
@@ -96,7 +96,7 @@ static inline void page_counter_reset_watermark(struct page_counter *counter)
counter->watermark = usage;
}
-#ifdef CONFIG_MEMCG
+#if IS_ENABLED(CONFIG_MEMCG) || IS_ENABLED(CONFIG_CGROUP_DEVICE)
void page_counter_calculate_protection(struct page_counter *root,
struct page_counter *counter,
bool recursive_protection);
diff --git a/init/Kconfig b/init/Kconfig
index 530a382ee0feb..2da595facd97f 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1123,6 +1123,13 @@ config CGROUP_RDMA
Attaching processes with active RDMA resources to the cgroup
hierarchy is allowed even if can cross the hierarchy's limit.
+config CGROUP_DEV
+ bool "Device controller"
+ help
+ Provides the device subsystem controller.
+
+ ...
+
config CGROUP_FREEZER
bool "Freezer controller"
help
diff --git a/kernel/cgroup/Makefile b/kernel/cgroup/Makefile
index a5c9359d516f8..441d346fdc51f 100644
--- a/kernel/cgroup/Makefile
+++ b/kernel/cgroup/Makefile
@@ -7,4 +7,5 @@ obj-$(CONFIG_CGROUP_RDMA) += rdma.o
obj-$(CONFIG_CPUSETS) += cpuset.o
obj-$(CONFIG_CPUSETS_V1) += cpuset-v1.o
obj-$(CONFIG_CGROUP_MISC) += misc.o
+obj-$(CONFIG_CGROUP_DEV) += dev.o
obj-$(CONFIG_CGROUP_DEBUG) += debug.o
diff --git a/kernel/cgroup/dev.c b/kernel/cgroup/dev.c
new file mode 100644
index 0000000000000..e422ccbfbc444
--- /dev/null
+++ b/kernel/cgroup/dev.c
@@ -0,0 +1,893 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2023-2024 Intel Corporation (Maarten Lankhorst <dev@lankhorst.se>)
+ * Copyright 2024 Red Hat (Maxime Ripard <mripard@kernel.org>)
+ * Partially based on the rdma and misc controllers, which bear the following copyrights:
+ *
+ * Copyright 2020 Google LLC
+ * Copyright (C) 2016 Parav Pandit <pandit.parav@gmail.com>
+ */
+
+#include <linux/cgroup.h>
+#include <linux/cgroup_dev.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/page_counter.h>
+#include <linux/parser.h>
+#include <linux/slab.h>
+
+struct devcg_device {
+ /**
+ * @ref: References keeping the device alive.
+ * Keeps the device reference alive after a succesful RCU lookup.
+ */
+ struct kref ref;
+
+ /** @rcu: RCU head for freeing */
+ struct rcu_head rcu;
+
+ /**
+ * @dev_node: Linked into &devcg_devices list.
+ * Protected by RCU and global spinlock.
+ */
+ struct list_head dev_node;
+
+ /**
+ * @pools: List of pools linked to this device.
+ * Protected by global spinlock only
+ */
+ struct list_head pools;
+
+ /**
+ * @base: Copy of the struct passed on register.
+ * A copy is made to prevent lifetime issues. devcg_device may
+ * be kept alive when changing cgroups values concurrently through
+ * rcu lookups.
+ */
+ struct dev_cgroup_device base;
+
+ /** @name: Name describing the node, set by dev_cgroup_register_device */
+ const char *name;
+
+ /**
+ * @unregistered: Whether the device is unregistered by its caller.
+ * No new pools should be added to the device afterwards.
+ */
+ bool unregistered;
+};
+
+struct devcg_state {
+ struct cgroup_subsys_state css;
+
+ struct list_head pools;
+};
+
+struct dev_cgroup_pool_state {
+ struct devcg_device *device;
+ struct devcg_state *cs;
+
+ /* css node, RCU protected against device teardown */
+ struct list_head css_node;
+
+ /* dev node, no RCU protection required */
+ struct list_head dev_node;
+
+ int num_res, inited;
+ struct rcu_head rcu;
+
+ struct devcg_pool_res {
+ struct page_counter cnt;
+ } resources[];
+};
+
+/*
+ * 3 operations require locking protection:
+ * - Registering and unregistering device to/from list, requires global lock.
+ * - Adding a dev_cgroup_pool_state to a CSS, removing when CSS is freed.
+ * - Adding a dev_cgroup_pool_state to a device list.
+ *
+ * Since for the most common operations RCU provides enough protection, I
+ * do not think more granular locking makes sense. Most protection is offered
+ * by RCU and the lockless operating page_counter.
+ */
+static DEFINE_SPINLOCK(devcg_lock);
+static LIST_HEAD(devcg_devices);
+
+static inline struct devcg_state *
+css_to_devcs(struct cgroup_subsys_state *css)
+{
+ return container_of(css, struct devcg_state, css);
+}
+
+static inline struct devcg_state *get_current_devcs(void)
+{
+ return css_to_devcs(task_get_css(current, dev_cgrp_id));
+}
+
+static struct devcg_state *parent_devcs(struct devcg_state *cg)
+{
+ return cg->css.parent ? css_to_devcs(cg->css.parent) : NULL;
+}
+
+static void free_cg_pool(struct dev_cgroup_pool_state *pool)
+{
+ list_del(&pool->dev_node);
+ kfree(pool);
+}
+
+static void
+set_resource_min(struct dev_cgroup_pool_state *pool, int i, u64 val)
+{
+ page_counter_set_min(&pool->resources[i].cnt, val);
+}
+
+static void
+set_resource_low(struct dev_cgroup_pool_state *pool, int i, u64 val)
+{
+ page_counter_set_low(&pool->resources[i].cnt, val);
+}
+
+static void
+set_resource_max(struct dev_cgroup_pool_state *pool, int i, u64 val)
+{
+ page_counter_set_max(&pool->resources[i].cnt, val);
+}
+
+static u64 get_resource_low(struct dev_cgroup_pool_state *pool, int idx)
+{
+ return pool ? READ_ONCE(pool->resources[idx].cnt.low) : 0;
+}
+
+static u64 get_resource_min(struct dev_cgroup_pool_state *pool, int idx)
+{
+ return pool ? READ_ONCE(pool->resources[idx].cnt.min) : 0;
+}
+
+static u64 get_resource_max(struct dev_cgroup_pool_state *pool, int idx)
+{
+ return pool ? READ_ONCE(pool->resources[idx].cnt.max) : PAGE_COUNTER_MAX;
+}
+
+static u64 get_resource_current(struct dev_cgroup_pool_state *pool, int idx)
+{
+ return pool ? page_counter_read(&pool->resources[idx].cnt) : 0;
+}
+
+static void reset_all_resource_limits(struct dev_cgroup_pool_state *rpool)
+{
+ int i;
+
+ for (i = 0; i < rpool->num_res; i++) {
+ set_resource_min(rpool, i, 0);
+ set_resource_low(rpool, i, 0);
+ set_resource_max(rpool, i, PAGE_COUNTER_MAX);
+ }
+}
+
+static void devcs_offline(struct cgroup_subsys_state *css)
+{
+ struct devcg_state *devcs = css_to_devcs(css);
+ struct dev_cgroup_pool_state *pool;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(pool, &devcs->pools, css_node)
+ reset_all_resource_limits(pool);
+ rcu_read_unlock();
+}
+
+static void devcs_free(struct cgroup_subsys_state *css)
+{
+ struct devcg_state *devcs = css_to_devcs(css);
+ struct dev_cgroup_pool_state *pool, *next;
+
+ spin_lock(&devcg_lock);
+ list_for_each_entry_safe(pool, next, &devcs->pools, css_node) {
+ /*
+ *The pool is dead and all references are 0,
+ * no need for RCU protection with list_del_rcu or freeing.
+ */
+ list_del(&pool->css_node);
+ free_cg_pool(pool);
+ }
+ spin_unlock(&devcg_lock);
+
+ kfree(devcs);
+}
+
+static struct cgroup_subsys_state *
+devcs_alloc(struct cgroup_subsys_state *parent_css)
+{
+ struct devcg_state *devcs = kzalloc(sizeof(*devcs), GFP_KERNEL);
+ if (!devcs)
+ return ERR_PTR(-ENOMEM);
+
+ INIT_LIST_HEAD(&devcs->pools);
+ return &devcs->css;
+}
+
+static struct dev_cgroup_pool_state *
+find_cg_pool_locked(struct devcg_state *devcs, struct devcg_device *dev)
+{
+ struct dev_cgroup_pool_state *pool;
+
+ list_for_each_entry_rcu(pool, &devcs->pools, css_node, spin_is_locked(&devcg_lock))
+ if (pool->device == dev)
+ return pool;
+
+ return NULL;
+}
+
+static struct dev_cgroup_pool_state *pool_parent(struct dev_cgroup_pool_state *pool)
+{
+ if (!pool->resources[0].cnt.parent)
+ return NULL;
+
+ return container_of(pool->resources[0].cnt.parent, typeof(*pool), resources[0].cnt);
+}
+
+/**
+ * dev_cgroup_state_evict_valuable() - Check if we should evict from test_pool
+ * @dev: &dev_cgroup_device
+ * @index: The index number of the region being tested.
+ * @limit_pool: The pool for which we hit limits
+ * @test_pool: The pool for which to test
+ * @ignore_low: Whether we have to respect low watermarks.
+ * @ret_hit_low: Pointer to whether it makes sense to consider low watermark.
+ *
+ * This function returns true if we can evict from @test_pool, false if not.
+ * When returning false and @ignore_low is false, @ret_hit_low may
+ * be set to true to indicate this function can be retried with @ignore_low
+ * set to true.
+ *
+ * Return: bool
+ */
+bool dev_cgroup_state_evict_valuable(struct dev_cgroup_device *dev, int index,
+ struct dev_cgroup_pool_state *limit_pool,
+ struct dev_cgroup_pool_state *test_pool,
+ bool ignore_low, bool *ret_hit_low)
+{
+ struct dev_cgroup_pool_state *pool = test_pool;
+ struct page_counter *climit, *ctest;
+ u64 used, min, low;
+
+ /* Can always evict from current pool, despite limits */
+ if (limit_pool == test_pool)
+ return true;
+
+ if (limit_pool) {
+ if (!parent_devcs(limit_pool->cs))
+ return true;
+
+ for (pool = test_pool; pool && limit_pool != pool; pool = pool_parent(pool))
+ {}
+
+ if (!pool)
+ return false;
+ } else {
+ /*
+ * If there is no cgroup limiting memory usage, use the root
+ * cgroup instead for limit calculations.
+ */
+ for (limit_pool = test_pool; pool_parent(limit_pool); limit_pool = pool_parent(limit_pool))
+ {}
+ }
+
+ climit = &limit_pool->resources[index].cnt;
+ ctest = &test_pool->resources[index].cnt;
+
+ page_counter_calculate_protection(climit, ctest, true);
+
+ used = page_counter_read(ctest);
+ min = READ_ONCE(ctest->emin);
+
+ if (used <= min)
+ return false;
+
+ if (!ignore_low) {
+ low = READ_ONCE(ctest->elow);
+ if (used > low)
+ return true;
+
+ *ret_hit_low = true;
+ return false;
+ }
+ return true;
+}
+EXPORT_SYMBOL_GPL(dev_cgroup_state_evict_valuable);
+
+static struct dev_cgroup_pool_state *
+alloc_pool_single(struct devcg_state *devcs, struct devcg_device *dev,
+ struct dev_cgroup_pool_state **allocpool)
+{
+ struct devcg_state *parent = parent_devcs(devcs);
+ struct dev_cgroup_pool_state *pool, *ppool = NULL;
+ int i;
+
+ if (!*allocpool) {
+ pool = kzalloc(offsetof(struct dev_cgroup_pool_state, resources[dev->base.num_regions]), GFP_NOWAIT);
+ if (!pool)
+ return ERR_PTR(-ENOMEM);
+ } else {
+ pool = *allocpool;
+ *allocpool = NULL;
+ }
+
+ pool->device = dev;
+ pool->num_res = dev->base.num_regions;
+ pool->cs = devcs;
+
+ if (parent)
+ ppool = find_cg_pool_locked(parent, dev);
+
+ for (i = 0; i < pool->num_res; i++)
+ page_counter_init(&pool->resources[i].cnt,
+ ppool ? &ppool->resources[i].cnt : NULL, true);
+ reset_all_resource_limits(pool);
+
+ list_add_tail_rcu(&pool->css_node, &devcs->pools);
+ list_add_tail(&pool->dev_node, &dev->pools);
+
+ if (!parent)
+ pool->inited = true;
+ else
+ pool->inited = ppool ? ppool->inited : false;
+ return pool;
+}
+
+static struct dev_cgroup_pool_state *
+get_cg_pool_locked(struct devcg_state *devcs, struct devcg_device *dev,
+ struct dev_cgroup_pool_state **allocpool)
+{
+ struct dev_cgroup_pool_state *pool, *ppool, *retpool;
+ struct devcg_state *p, *pp;
+ int i;
+
+ /*
+ * Recursively create pool, we may not initialize yet on
+ * recursion, this is done as a separate step.
+ */
+ for (p = devcs; p; p = parent_devcs(p)) {
+ pool = find_cg_pool_locked(p, dev);
+ if (!pool)
+ pool = alloc_pool_single(p, dev, allocpool);
+
+ if (IS_ERR(pool))
+ return pool;
+
+ if (p == devcs && pool->inited)
+ return pool;
+
+ if (pool->inited)
+ break;
+ }
+
+ retpool = pool = find_cg_pool_locked(devcs, dev);
+ for (p = devcs, pp = parent_devcs(devcs); pp; p = pp, pp = parent_devcs(p)) {
+ if (pool->inited)
+ break;
+
+ /* ppool was created if it didn't exist by above loop. */
+ ppool = find_cg_pool_locked(pp, dev);
+
+ /* Fix up parent links, mark as inited. */
+ for (i = 0; i < pool->num_res; i++)
+ pool->resources[i].cnt.parent = &ppool->resources[i].cnt;
+ pool->inited = true;
+
+ pool = ppool;
+ }
+
+ return retpool;
+}
+
+static void devcg_free_rcu(struct rcu_head *rcu)
+{
+ struct devcg_device *dev = container_of(rcu, typeof(*dev), rcu);
+ struct dev_cgroup_pool_state *pool, *next;
+
+ list_for_each_entry_safe(pool, next, &dev->pools, dev_node)
+ free_cg_pool(pool);
+ kfree(dev->name);
+ kfree(dev);
+}
+
+static void devcg_free_device(struct kref *ref)
+{
+ struct devcg_device *cgdev = container_of(ref, typeof(*cgdev), ref);
+
+ call_rcu(&cgdev->rcu, devcg_free_rcu);
+}
+
+/**
+ * dev_cgroup_unregister_device() - Unregister a previously registered device.
+ * @cgdev: The device to unregister.
+ *
+ * This function undoes dev_cgroup_register_device.
+ */
+void dev_cgroup_unregister_device(struct dev_cgroup_device *cgdev)
+{
+ struct devcg_device *dev;
+ struct list_head *entry;
+
+ if (!cgdev || !cgdev->priv)
+ return;
+
+ dev = cgdev->priv;
+ cgdev->priv = NULL;
+
+ spin_lock(&devcg_lock);
+
+ /* Remove from global device list */
+ list_del_rcu(&dev->dev_node);
+
+ list_for_each_rcu(entry, &dev->pools) {
+ struct dev_cgroup_pool_state *pool =
+ container_of(entry, typeof(*pool), dev_node);
+
+ list_del_rcu(&pool->css_node);
+ }
+
+ /*
+ * Ensure any RCU based lookups fail. Additionally,
+ * no new pools should be added to the dead device
+ * by get_cg_pool_unlocked.
+ */
+ dev->unregistered = true;
+ spin_unlock(&devcg_lock);
+
+ kref_put(&dev->ref, devcg_free_device);
+}
+EXPORT_SYMBOL_GPL(dev_cgroup_unregister_device);
+
+/**
+ * dev_cgroup_register_device() - Register a devices for dev cgroup.
+ * @cgdev: A filled &dev_cgroup_device structure.
+ * @name: A name for the device.
+ *
+ * This function registers a node in the dev cgroup with the
+ * name given. After calling this function, the device can be
+ * used for allocations.
+ *
+ * Return: 0 on success, -ERRNO on failure.
+ */
+int dev_cgroup_register_device(struct dev_cgroup_device *cgdev,
+ const char *name)
+{
+ struct devcg_device *dev;
+ char *dev_name;
+
+ cgdev->priv = NULL;
+ if (!cgdev->num_regions)
+ return 0;
+
+ cgdev->priv = dev = kzalloc(sizeof (*dev), GFP_KERNEL);
+ if (!dev)
+ return -ENOMEM;
+
+ dev_name = kstrdup(name, GFP_KERNEL);
+ if (!dev_name) {
+ kfree(dev);
+ cgdev->priv = NULL;
+ return -ENOMEM;
+ }
+
+ INIT_LIST_HEAD(&dev->pools);
+ dev->name = dev_name;
+ dev->base = *cgdev;
+ kref_init(&dev->ref);
+
+ spin_lock(&devcg_lock);
+ list_add_tail_rcu(&dev->dev_node, &devcg_devices);
+ spin_unlock(&devcg_lock);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(dev_cgroup_register_device);
+
+static struct devcg_device *devcg_get_device_by_name(const char *name)
+{
+ struct devcg_device *dev;
+
+ list_for_each_entry_rcu(dev, &devcg_devices, dev_node, spin_is_locked(&devcg_lock))
+ if (!strcmp(name, dev->name) &&
+ kref_get_unless_zero(&dev->ref))
+ return dev;
+
+ return NULL;
+}
+
+/**
+ * dev_cgroup_pool_state_put() - Drop a reference to a dev_cgroup_pool_state
+ * @pool: &dev_cgroup_pool_state
+ *
+ * Called to drop a reference to the limiting pool returned by
+ * dev_cgroup_try_charge().
+ */
+void dev_cgroup_pool_state_put(struct dev_cgroup_pool_state *pool)
+{
+ if (pool)
+ css_put(&pool->cs->css);
+}
+EXPORT_SYMBOL_GPL(dev_cgroup_pool_state_put);
+
+static struct dev_cgroup_pool_state *
+get_cg_pool_unlocked(struct devcg_state *cg, struct devcg_device *dev)
+{
+ struct dev_cgroup_pool_state *pool, *allocpool = NULL;
+
+ /* fastpath lookup? */
+ rcu_read_lock();
+ pool = find_cg_pool_locked(cg, dev);
+ if (pool && !READ_ONCE(pool->inited))
+ pool = NULL;
+ rcu_read_unlock();
+
+ while (!pool) {
+ spin_lock(&devcg_lock);
+ if (!dev->unregistered)
+ pool = get_cg_pool_locked(cg, dev, &allocpool);
+ else
+ pool = ERR_PTR(-ENODEV);
+ spin_unlock(&devcg_lock);
+
+ if (pool == ERR_PTR(-ENOMEM)) {
+ pool = NULL;
+ if (WARN_ON(allocpool))
+ continue;
+
+ allocpool = kzalloc(offsetof(struct dev_cgroup_pool_state, resources[dev->base.num_regions]), GFP_KERNEL);
+ if (allocpool) {
+ pool = NULL;
+ continue;
+ }
+ }
+ }
+
+ kfree(allocpool);
+ return pool;
+}
+
+/**
+ * dev_cgroup_uncharge() - Uncharge a pool.
+ * @pool: Pool to uncharge.
+ * @index: Index of region to uncharge.
+ * @size: Size to uncharge.
+ *
+ * Undoes the effects of dev_cgroup_try_charge.
+ * Must be called with the returned pool as argument,
+ * and same @index and @size.
+ */
+void dev_cgroup_uncharge(struct dev_cgroup_pool_state *pool,
+ u32 index, u64 size)
+{
+ struct dev_cgroup_device *cgdev;
+
+ if (!pool)
+ return;
+
+ cgdev = &pool->device->base;
+ if (index >= cgdev->num_regions)
+ return;
+
+ page_counter_uncharge(&pool->resources[index].cnt, size);
+ css_put(&pool->cs->css);
+}
+EXPORT_SYMBOL_GPL(dev_cgroup_uncharge);
+
+/**
+ * dev_cgroup_try_charge() - Try charging a new allocation to a region.
+ * @dev: Device to charge
+ * @index: Index of the region to charge
+ * @size: Size (in bytes) to charge.
+ * @ret_pool: On succesfull allocation, the pool that is charged.
+ * @ret_limit_pool: On a failed allocation, the limiting pool.
+ *
+ * This function charges the current pool for @dev with region at @index for a
+ * size of @size bytes.
+ *
+ * If the function succeeds, @ret_pool is set, which must be passed to
+ * dev_cgroup_uncharge() when undoing the allocation.
+ *
+ * When this function fails with -EAGAIN and @ret_limit_pool is non-null, it
+ * will be set to the pool for which the limit is hit. This can be used for
+ * eviction as argument to dev_cgroup_evict_valuable(). This reference must be freed
+ * with @dev_cgroup_pool_state_put().
+ *
+ * Return: 0 on success, -EAGAIN on hitting a limit, or a negative errno on failure.
+ */
+int dev_cgroup_try_charge(struct dev_cgroup_device *dev,
+ u32 index, u64 size,
+ struct dev_cgroup_pool_state **ret_pool,
+ struct dev_cgroup_pool_state **ret_limit_pool)
+{
+ struct devcg_device *cgdev = dev->priv;
+ struct devcg_state *cg;
+ struct dev_cgroup_pool_state *pool;
+ struct page_counter *fail;
+ int ret;
+
+ *ret_pool = NULL;
+ if (ret_limit_pool)
+ *ret_limit_pool = NULL;
+
+ /* Early init or device unregistered */
+ if (!cgdev)
+ return 0;
+
+ if (index >= cgdev->base.num_regions)
+ return -EINVAL;
+
+ /*
+ * hold on to css, as cgroup can be removed but resource
+ * accounting happens on css.
+ */
+ cg = get_current_devcs();
+
+ pool = get_cg_pool_unlocked(cg, cgdev);
+ if (IS_ERR(pool)) {
+ ret = PTR_ERR(pool);
+ goto err;
+ }
+
+ if (!page_counter_try_charge(&pool->resources[index].cnt, size, &fail)) {
+ if (ret_limit_pool) {
+ *ret_limit_pool = container_of(fail, struct dev_cgroup_pool_state, resources[index].cnt);
+ css_get(&(*ret_limit_pool)->cs->css);
+ }
+ ret = -EAGAIN;
+ goto err;
+ }
+
+ /* On success, reference is transferred to *ret_pool */
+ *ret_pool = pool;
+ return 0;
+
+err:
+ css_put(&cg->css);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dev_cgroup_try_charge);
+
+static int devcg_region_capacity_show(struct seq_file *sf, void *v)
+{
+ struct devcg_device *dev;
+ int i;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(dev, &devcg_devices, dev_node) {
+ seq_puts(sf, dev->name);
+ for (i = 0; i < dev->base.num_regions; i++)
+ seq_printf(sf, " %s=%llu",
+ dev->base.regions[i].name,
+ dev->base.regions[i].size);
+ seq_putc(sf, '\n');
+ }
+ rcu_read_unlock();
+ return 0;
+}
+
+static s64 parse_resource(char *c, char **retname)
+{
+ substring_t argstr;
+ char *name, *value = c;
+ size_t len;
+ int ret;
+ u64 retval;
+
+ name = strsep(&value, "=");
+ if (!name || !value)
+ return -EINVAL;
+
+ *retname = name;
+ len = strlen(value);
+
+ argstr.from = value;
+ argstr.to = value + len;
+
+ ret = match_u64(&argstr, &retval);
+ if (ret >= 0) {
+ if (retval > S64_MAX)
+ return -EINVAL;
+ if (retval > PAGE_COUNTER_MAX)
+ return PAGE_COUNTER_MAX;
+ return retval;
+ }
+ if (!strncmp(value, "max", len))
+ return PAGE_COUNTER_MAX;
+
+ /* Not u64 or max, error */
+ return -EINVAL;
+}
+
+static int devcg_parse_limits(char *options, struct devcg_device *dev,
+ u64 *new_limits, unsigned long *set_mask)
+{
+ char *c, *region;
+
+ /* parse resource options */
+ while ((c = strsep(&options, " \t")) != NULL) {
+ s64 limit;
+ int i;
+
+ limit = parse_resource(c, ®ion);
+ if (limit < 0)
+ return limit;
+
+
+ for (i = 0; i < dev->base.num_regions; i++)
+ if (!strcmp(dev->base.regions[i].name, region))
+ break;
+
+ if (i == dev->base.num_regions)
+ return -EINVAL;
+
+ new_limits[i] = limit;
+ *set_mask |= BIT(i);
+ }
+ return 0;
+}
+
+static ssize_t devcg_limit_write(struct kernfs_open_file *of,
+ char *buf, size_t nbytes, loff_t off,
+ void (*apply)(struct dev_cgroup_pool_state *, int, u64))
+{
+ struct devcg_state *devcs = css_to_devcs(of_css(of));
+ int err = 0;
+
+ while (buf && !err) {
+ struct dev_cgroup_pool_state *pool = NULL;
+ char *options, *dev_name;
+ unsigned long set_mask = 0;
+ struct devcg_device *dev;
+ u64 new_limits[DEVICE_CGROUP_MAX_REGIONS];
+ int i;
+
+ options = buf;
+ buf = strchr(buf, '\n');
+ if (buf)
+ *buf++ = '\0';
+
+ options = strstrip(options);
+
+ /* eat empty lines */
+ if (!options[0])
+ continue;
+
+ dev_name = strsep(&options, " \t");
+ if (!dev_name[0])
+ continue;
+
+ rcu_read_lock();
+ dev = devcg_get_device_by_name(dev_name);
+ rcu_read_unlock();
+
+ if (!dev)
+ return -EINVAL;
+
+ err = devcg_parse_limits(options, dev, new_limits, &set_mask);
+ if (err < 0)
+ goto out_put;
+
+ pool = get_cg_pool_unlocked(devcs, dev);
+ if (IS_ERR(pool)) {
+ err = PTR_ERR(pool);
+ goto out_put;
+ }
+
+ /* And commit */
+ for_each_set_bit(i, &set_mask, DEVICE_CGROUP_MAX_REGIONS)
+ apply(pool, i, new_limits[i]);
+
+out_put:
+ kref_put(&dev->ref, devcg_free_device);
+ }
+
+
+ return err ?: nbytes;
+}
+
+static int devcg_limit_show(struct seq_file *sf, void *v,
+ u64 (*fn)(struct dev_cgroup_pool_state *, int))
+{
+ struct devcg_state *devcs = css_to_devcs(seq_css(sf));
+ struct devcg_device *dev;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(dev, &devcg_devices, dev_node) {
+ struct dev_cgroup_pool_state *pool = find_cg_pool_locked(devcs, dev);
+
+ seq_puts(sf, dev->name);
+
+ for (int i = 0; i < dev->base.num_regions; i++) {
+ u64 val = fn(pool, i);
+
+ if (val < PAGE_COUNTER_MAX)
+ seq_printf(sf, " %s=%lld",
+ dev->base.regions[i].name, val);
+ else
+ seq_printf(sf, " %s=max", dev->base.regions[i].name);
+ }
+
+ seq_putc(sf, '\n');
+ }
+ rcu_read_unlock();
+
+ return 0;
+}
+
+static int devcg_region_current_show(struct seq_file *sf, void *v)
+{
+ return devcg_limit_show(sf, v, get_resource_current);
+}
+
+static int devcg_region_min_show(struct seq_file *sf, void *v)
+{
+ return devcg_limit_show(sf, v, get_resource_min);
+}
+
+static ssize_t devcg_region_min_write(struct kernfs_open_file *of,
+ char *buf, size_t nbytes, loff_t off)
+{
+ return devcg_limit_write(of, buf, nbytes, off, set_resource_min);
+}
+
+static int devcg_region_low_show(struct seq_file *sf, void *v)
+{
+ return devcg_limit_show(sf, v, get_resource_low);
+}
+
+static ssize_t devcg_region_low_write(struct kernfs_open_file *of,
+ char *buf, size_t nbytes, loff_t off)
+{
+ return devcg_limit_write(of, buf, nbytes, off, set_resource_low);
+}
+
+static int devcg_region_max_show(struct seq_file *sf, void *v)
+{
+ return devcg_limit_show(sf, v, get_resource_max);
+}
+
+static ssize_t devcg_region_max_write(struct kernfs_open_file *of,
+ char *buf, size_t nbytes, loff_t off)
+{
+ return devcg_limit_write(of, buf, nbytes, off, set_resource_max);
+}
+
+static struct cftype files[] = {
+ {
+ .name = "region.capacity",
+ .seq_show = devcg_region_capacity_show,
+ .flags = CFTYPE_ONLY_ON_ROOT,
+ },
+ {
+ .name = "region.current",
+ .seq_show = devcg_region_current_show,
+ },
+ {
+ .name = "region.min",
+ .write = devcg_region_min_write,
+ .seq_show = devcg_region_min_show,
+ .flags = CFTYPE_NOT_ON_ROOT,
+ },
+ {
+ .name = "region.low",
+ .write = devcg_region_low_write,
+ .seq_show = devcg_region_low_show,
+ .flags = CFTYPE_NOT_ON_ROOT,
+ },
+ {
+ .name = "region.max",
+ .write = devcg_region_max_write,
+ .seq_show = devcg_region_max_show,
+ .flags = CFTYPE_NOT_ON_ROOT,
+ },
+ { } /* Zero entry terminates. */
+};
+
+struct cgroup_subsys dev_cgrp_subsys = {
+ .css_alloc = devcs_alloc,
+ .css_free = devcs_free,
+ .css_offline = devcs_offline,
+ .legacy_cftypes = files,
+ .dfl_cftypes = files,
+};
diff --git a/mm/page_counter.c b/mm/page_counter.c
index b249d15af9dd8..fbb0cb34415d7 100644
--- a/mm/page_counter.c
+++ b/mm/page_counter.c
@@ -288,7 +288,7 @@ int page_counter_memparse(const char *buf, const char *max,
}
-#ifdef CONFIG_MEMCG
+#if IS_ENABLED(CONFIG_MEMCG) || IS_ENABLED(CONFIG_CGROUP_DEVICE)
/*
* This function calculates an individual page counter's effective
* protection which is derived from its own memory.min/low, its
@@ -460,4 +460,4 @@ void page_counter_calculate_protection(struct page_counter *root,
atomic_long_read(&parent->children_low_usage),
recursive_protection));
}
-#endif /* CONFIG_MEMCG */
+#endif /* CONFIG_MEMCG || CONFIG_CGROUP_DEVICE */
--
2.45.2
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/7] kernel/cgroup: Add "dev" memory accounting cgroup
2024-10-23 7:52 ` [PATCH 1/7] kernel/cgroup: " Maarten Lankhorst
@ 2024-10-23 15:26 ` Waiman Long
2024-11-11 9:28 ` Maarten Lankhorst
2024-10-25 5:44 ` kernel test robot
2024-10-28 14:53 ` Friedrich Vock
2 siblings, 1 reply; 27+ messages in thread
From: Waiman Long @ 2024-10-23 15:26 UTC (permalink / raw)
To: Maarten Lankhorst, intel-xe, linux-kernel, dri-devel, Tejun Heo,
Zefan Li, Johannes Weiner, Andrew Morton
Cc: Friedrich Vock, cgroups, linux-mm, Maxime Ripard
On 10/23/24 3:52 AM, Maarten Lankhorst wrote:
> The initial version was based roughly on the rdma and misc cgroup
> controllers, with a lot of the accounting code borrowed from rdma.
>
> The current version is a complete rewrite with page counter; it uses
> the same min/low/max semantics as the memory cgroup as a result.
>
> There's a small mismatch as TTM uses u64, and page_counter long pages.
> In practice it's not a problem. 32-bits systems don't really come with
>> =4GB cards and as long as we're consistently wrong with units, it's
> fine. The device page size may not be in the same units as kernel page
> size, and each region might also have a different page size (VRAM vs GART
> for example).
>
> The interface is simple:
> - populate dev_cgroup_try_charge->regions[..] name and size for each active
> region, set num_regions accordingly.
> - Call (dev,drmm)_cgroup_register_device()
> - Use dev_cgroup_try_charge to check if you can allocate a chunk of memory,
> use dev_cgroup__uncharge when freeing it. This may return an error code,
> or -EAGAIN when the cgroup limit is reached. In that case a reference
> to the limiting pool is returned.
> - The limiting cs can be used as compare function for
> dev_cgroup_state_evict_valuable.
> - After having evicted enough, drop reference to limiting cs with
> dev_cgroup_pool_state_put.
>
> This API allows you to limit device resources with cgroups.
> You can see the supported cards in /sys/fs/cgroup/dev.region.capacity
> You need to echo +dev to cgroup.subtree_control, and then you can
> partition memory.
>
> Co-developed-by: Friedrich Vock <friedrich.vock@gmx.de>
> Signed-off-by: Friedrich Vock <friedrich.vock@gmx.de>
> Co-developed-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> Documentation/admin-guide/cgroup-v2.rst | 51 ++
> Documentation/core-api/cgroup.rst | 9 +
> Documentation/core-api/index.rst | 1 +
> Documentation/gpu/drm-compute.rst | 54 ++
> include/linux/cgroup_dev.h | 91 +++
> include/linux/cgroup_subsys.h | 4 +
> include/linux/page_counter.h | 2 +-
> init/Kconfig | 7 +
> kernel/cgroup/Makefile | 1 +
> kernel/cgroup/dev.c | 893 ++++++++++++++++++++++++
> mm/page_counter.c | 4 +-
> 11 files changed, 1114 insertions(+), 3 deletions(-)
> create mode 100644 Documentation/core-api/cgroup.rst
> create mode 100644 Documentation/gpu/drm-compute.rst
> create mode 100644 include/linux/cgroup_dev.h
> create mode 100644 kernel/cgroup/dev.c
Just a general comment.
Cgroup v1 has a legacy device controller in security/device_cgroup.c
which is no longer available in cgroup v2. So if you use the name device
controller, the documentation must be clear that it is completely
different and have no relationship from the device controller in cgroup v1.
Cheers,
Longman
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/7] kernel/cgroup: Add "dev" memory accounting cgroup
2024-10-23 15:26 ` Waiman Long
@ 2024-11-11 9:28 ` Maarten Lankhorst
0 siblings, 0 replies; 27+ messages in thread
From: Maarten Lankhorst @ 2024-11-11 9:28 UTC (permalink / raw)
To: Waiman Long, intel-xe, linux-kernel, dri-devel, Tejun Heo,
Zefan Li, Johannes Weiner, Andrew Morton
Cc: Friedrich Vock, cgroups, linux-mm, Maxime Ripard
Den 2024-10-23 kl. 17:26, skrev Waiman Long:
> On 10/23/24 3:52 AM, Maarten Lankhorst wrote:
>> The initial version was based roughly on the rdma and misc cgroup
>> controllers, with a lot of the accounting code borrowed from rdma.
>>
>> The current version is a complete rewrite with page counter; it uses
>> the same min/low/max semantics as the memory cgroup as a result.
>>
>> There's a small mismatch as TTM uses u64, and page_counter long pages.
>> In practice it's not a problem. 32-bits systems don't really come with
>>> =4GB cards and as long as we're consistently wrong with units, it's
>> fine. The device page size may not be in the same units as kernel page
>> size, and each region might also have a different page size (VRAM vs GART
>> for example).
>>
>> The interface is simple:
>> - populate dev_cgroup_try_charge->regions[..] name and size for each
>> active
>> region, set num_regions accordingly.
>> - Call (dev,drmm)_cgroup_register_device()
>> - Use dev_cgroup_try_charge to check if you can allocate a chunk of
>> memory,
>> use dev_cgroup__uncharge when freeing it. This may return an error
>> code,
>> or -EAGAIN when the cgroup limit is reached. In that case a reference
>> to the limiting pool is returned.
>> - The limiting cs can be used as compare function for
>> dev_cgroup_state_evict_valuable.
>> - After having evicted enough, drop reference to limiting cs with
>> dev_cgroup_pool_state_put.
>>
>> This API allows you to limit device resources with cgroups.
>> You can see the supported cards in /sys/fs/cgroup/dev.region.capacity
>> You need to echo +dev to cgroup.subtree_control, and then you can
>> partition memory.
>>
>> Co-developed-by: Friedrich Vock <friedrich.vock@gmx.de>
>> Signed-off-by: Friedrich Vock <friedrich.vock@gmx.de>
>> Co-developed-by: Maxime Ripard <mripard@kernel.org>
>> Signed-off-by: Maxime Ripard <mripard@kernel.org>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> Documentation/admin-guide/cgroup-v2.rst | 51 ++
>> Documentation/core-api/cgroup.rst | 9 +
>> Documentation/core-api/index.rst | 1 +
>> Documentation/gpu/drm-compute.rst | 54 ++
>> include/linux/cgroup_dev.h | 91 +++
>> include/linux/cgroup_subsys.h | 4 +
>> include/linux/page_counter.h | 2 +-
>> init/Kconfig | 7 +
>> kernel/cgroup/Makefile | 1 +
>> kernel/cgroup/dev.c | 893 ++++++++++++++++++++++++
>> mm/page_counter.c | 4 +-
>> 11 files changed, 1114 insertions(+), 3 deletions(-)
>> create mode 100644 Documentation/core-api/cgroup.rst
>> create mode 100644 Documentation/gpu/drm-compute.rst
>> create mode 100644 include/linux/cgroup_dev.h
>> create mode 100644 kernel/cgroup/dev.c
>
> Just a general comment.
>
> Cgroup v1 has a legacy device controller in security/device_cgroup.c
> which is no longer available in cgroup v2. So if you use the name device
> controller, the documentation must be clear that it is completely
> different and have no relationship from the device controller in cgroup v1.
Hey,
Thanks for noticing. I didn't know there was such a controller. Seems
weird to have one for managing access to opening devnodes instead of a
security module.
I'll update the documentation in the next version to make it more clear.
Cheers,
~Maarten
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/7] kernel/cgroup: Add "dev" memory accounting cgroup
2024-10-23 7:52 ` [PATCH 1/7] kernel/cgroup: " Maarten Lankhorst
2024-10-23 15:26 ` Waiman Long
@ 2024-10-25 5:44 ` kernel test robot
2024-10-28 14:53 ` Friedrich Vock
2 siblings, 0 replies; 27+ messages in thread
From: kernel test robot @ 2024-10-25 5:44 UTC (permalink / raw)
To: Maarten Lankhorst, intel-xe, linux-kernel, dri-devel, Tejun Heo,
Zefan Li, Johannes Weiner, Andrew Morton
Cc: oe-kbuild-all, Linux Memory Management List, Friedrich Vock,
cgroups, Maxime Ripard, Maarten Lankhorst
Hi Maarten,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm/drm-next]
[also build test WARNING on drm-exynos/exynos-drm-next linus/master v6.12-rc4 next-20241024]
[cannot apply to tj-cgroup/for-next drm-xe/drm-xe-next akpm-mm/mm-everything drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-misc/drm-misc-next drm-tip/drm-tip]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Maarten-Lankhorst/kernel-cgroup-Add-dev-memory-accounting-cgroup/20241023-155504
base: git://anongit.freedesktop.org/drm/drm drm-next
patch link: https://lore.kernel.org/r/20241023075302.27194-2-maarten.lankhorst%40linux.intel.com
patch subject: [PATCH 1/7] kernel/cgroup: Add "dev" memory accounting cgroup
config: i386-randconfig-061-20241025 (https://download.01.org/0day-ci/archive/20241025/202410251311.OLEnaBoD-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241025/202410251311.OLEnaBoD-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410251311.OLEnaBoD-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> kernel/cgroup/dev.c:423:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
kernel/cgroup/dev.c:423:9: sparse: struct list_head [noderef] __rcu *
kernel/cgroup/dev.c:423:9: sparse: struct list_head *
>> kernel/cgroup/dev.c:423:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
kernel/cgroup/dev.c:423:9: sparse: struct list_head [noderef] __rcu *
kernel/cgroup/dev.c:423:9: sparse: struct list_head *
kernel/cgroup/dev.c: note: in included file (through include/linux/smp.h, include/linux/lockdep.h, include/linux/spinlock.h, ...):
include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true
include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true
include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true
vim +423 kernel/cgroup/dev.c
400
401 /**
402 * dev_cgroup_unregister_device() - Unregister a previously registered device.
403 * @cgdev: The device to unregister.
404 *
405 * This function undoes dev_cgroup_register_device.
406 */
407 void dev_cgroup_unregister_device(struct dev_cgroup_device *cgdev)
408 {
409 struct devcg_device *dev;
410 struct list_head *entry;
411
412 if (!cgdev || !cgdev->priv)
413 return;
414
415 dev = cgdev->priv;
416 cgdev->priv = NULL;
417
418 spin_lock(&devcg_lock);
419
420 /* Remove from global device list */
421 list_del_rcu(&dev->dev_node);
422
> 423 list_for_each_rcu(entry, &dev->pools) {
424 struct dev_cgroup_pool_state *pool =
425 container_of(entry, typeof(*pool), dev_node);
426
427 list_del_rcu(&pool->css_node);
428 }
429
430 /*
431 * Ensure any RCU based lookups fail. Additionally,
432 * no new pools should be added to the dead device
433 * by get_cg_pool_unlocked.
434 */
435 dev->unregistered = true;
436 spin_unlock(&devcg_lock);
437
438 kref_put(&dev->ref, devcg_free_device);
439 }
440 EXPORT_SYMBOL_GPL(dev_cgroup_unregister_device);
441
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/7] kernel/cgroup: Add "dev" memory accounting cgroup
2024-10-23 7:52 ` [PATCH 1/7] kernel/cgroup: " Maarten Lankhorst
2024-10-23 15:26 ` Waiman Long
2024-10-25 5:44 ` kernel test robot
@ 2024-10-28 14:53 ` Friedrich Vock
2024-11-11 22:53 ` Maarten Lankhorst
2 siblings, 1 reply; 27+ messages in thread
From: Friedrich Vock @ 2024-10-28 14:53 UTC (permalink / raw)
To: Maarten Lankhorst, intel-xe, linux-kernel, dri-devel, Tejun Heo,
Zefan Li, Johannes Weiner, Andrew Morton
Cc: cgroups, linux-mm, Maxime Ripard
On 23.10.24 09:52, Maarten Lankhorst wrote:
> The initial version was based roughly on the rdma and misc cgroup
> controllers, with a lot of the accounting code borrowed from rdma.
>
> The current version is a complete rewrite with page counter; it uses
> the same min/low/max semantics as the memory cgroup as a result.
>
> There's a small mismatch as TTM uses u64, and page_counter long pages.
> In practice it's not a problem. 32-bits systems don't really come with
>> =4GB cards and as long as we're consistently wrong with units, it's
> fine. The device page size may not be in the same units as kernel page
> size, and each region might also have a different page size (VRAM vs GART
> for example).
>
> The interface is simple:
> - populate dev_cgroup_try_charge->regions[..] name and size for each active
> region, set num_regions accordingly.
> - Call (dev,drmm)_cgroup_register_device()
> - Use dev_cgroup_try_charge to check if you can allocate a chunk of memory,
> use dev_cgroup__uncharge when freeing it. This may return an error code,
> or -EAGAIN when the cgroup limit is reached. In that case a reference
> to the limiting pool is returned.
> - The limiting cs can be used as compare function for
> dev_cgroup_state_evict_valuable.
> - After having evicted enough, drop reference to limiting cs with
> dev_cgroup_pool_state_put.
>
> This API allows you to limit device resources with cgroups.
> You can see the supported cards in /sys/fs/cgroup/dev.region.capacity
> You need to echo +dev to cgroup.subtree_control, and then you can
> partition memory.
>
> Co-developed-by: Friedrich Vock <friedrich.vock@gmx.de>
> Signed-off-by: Friedrich Vock <friedrich.vock@gmx.de>
> Co-developed-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> Documentation/admin-guide/cgroup-v2.rst | 51 ++
> Documentation/core-api/cgroup.rst | 9 +
> Documentation/core-api/index.rst | 1 +
> Documentation/gpu/drm-compute.rst | 54 ++
> include/linux/cgroup_dev.h | 91 +++
> include/linux/cgroup_subsys.h | 4 +
> include/linux/page_counter.h | 2 +-
> init/Kconfig | 7 +
> kernel/cgroup/Makefile | 1 +
> kernel/cgroup/dev.c | 893 ++++++++++++++++++++++++
> mm/page_counter.c | 4 +-
> 11 files changed, 1114 insertions(+), 3 deletions(-)
> create mode 100644 Documentation/core-api/cgroup.rst
> create mode 100644 Documentation/gpu/drm-compute.rst
> create mode 100644 include/linux/cgroup_dev.h
> create mode 100644 kernel/cgroup/dev.c
>
> diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> index 69af2173555fb..e8fe79244af9c 100644
> --- a/Documentation/admin-guide/cgroup-v2.rst
> +++ b/Documentation/admin-guide/cgroup-v2.rst
> @@ -2612,6 +2612,57 @@ RDMA Interface Files
> mlx4_0 hca_handle=1 hca_object=20
> ocrdma1 hca_handle=1 hca_object=23
>
> +DEV
> +----
> +
> +The "dev" controller regulates the distribution and accounting of
> +device resources, currently only memory regions. Because each memory
> +region may have its own page size, which does not have to be equal
> +to the system page size. the units are in bytes.
> +
> +DEV Interface Files
> +~~~~~~~~~~~~~~~~~~~~
> +
> + dev.region.max, dev.region.min, dev.region.low
> + A readwrite nested-keyed file that exists for all the cgroups
> + except root that describes current configured resource limit
> + for a device.
> +
> + Lines are keyed by device name and are not ordered.
> + Each line contains space separated resource name and its configured
> + limit that can be distributed.
> +
> + The following nested keys are defined.
> +
> + ========== =======================================================
> + * Maximum amount of bytes that allocatable in this region
> + ========== =======================================================
> +
> + An example for xe follows::
> +
> + drm/0000:03:00.0 vram0=1073741824 stolen=max
> +
> + The semantics are the same as for the memory cgroup controller, and are
> + calculated in the same way.
> +
> + dev.region.capacity
> + A read-only file that describes maximum region capacity.
> + It only exists on the root cgroup. Not all memory can be
> + allocated by cgroups, as the kernel reserves some for
> + internal use.
> +
> + An example for xe follows::
> +
> + drm/0000:03:00.0 vram0=8514437120 stolen=67108864
> +
> + dev.region.current
> + A read-only file that describes current resource usage.
> + It exists for all the cgroup except root.
> +
> + An example for xe follows::
> +
> + drm/0000:03:00.0 vram0=12550144 stolen=8650752
> +
> HugeTLB
> -------
>
> diff --git a/Documentation/core-api/cgroup.rst b/Documentation/core-api/cgroup.rst
> new file mode 100644
> index 0000000000000..475b32255bd68
> --- /dev/null
> +++ b/Documentation/core-api/cgroup.rst
> @@ -0,0 +1,9 @@
> +==================
> +Cgroup Kernel APIs
> +==================
> +
> +Device Cgroup API (devcg)
> +=========================
> +.. kernel-doc:: kernel/cgroup/dev.c
> + :export:
> +
> diff --git a/Documentation/core-api/index.rst b/Documentation/core-api/index.rst
> index 6a875743dd4b7..dbd6c4f9a6313 100644
> --- a/Documentation/core-api/index.rst
> +++ b/Documentation/core-api/index.rst
> @@ -108,6 +108,7 @@ more memory-management documentation in Documentation/mm/index.rst.
> dma-isa-lpc
> swiotlb
> mm-api
> + cgroup
> genalloc
> pin_user_pages
> boot-time-mm
> diff --git a/Documentation/gpu/drm-compute.rst b/Documentation/gpu/drm-compute.rst
> new file mode 100644
> index 0000000000000..116270976ef7a
> --- /dev/null
> +++ b/Documentation/gpu/drm-compute.rst
> @@ -0,0 +1,54 @@
> +==================================
> +Long running workloads and compute
> +==================================
> +
> +Long running workloads (compute) are workloads that will not complete in 10
> +seconds. (The time let the user wait before he reaches for the power button).
> +This means that other techniques need to be used to manage those workloads,
> +that cannot use fences.
> +
> +Some hardware may schedule compute jobs, and have no way to pre-empt them, or
> +have their memory swapped out from them. Or they simply want their workload
> +not to be preempted or swapped out at all.
> +
> +This means that it differs from what is described in driver-api/dma-buf.rst.
> +
> +As with normal compute jobs, dma-fence may not be used at all. In this case,
> +not even to force preemption. The driver with is simply forced to unmap a BO
> +from the long compute job's address space on unbind immediately, not even
> +waiting for the workload to complete. Effectively this terminates the workload
> +when there is no hardware support to recover.
> +
> +Since this is undesirable, there need to be mitigations to prevent a workload
> +from being terminated. There are several possible approach, all with their
> +advantages and drawbacks.
> +
> +The first approach you will likely try is to pin all buffers used by compute.
> +This guarantees that the job will run uninterrupted, but also allows a very
> +denial of service attack by pinning as much memory as possible, hogging the
> +all GPU memory, and possibly a huge chunk of CPU memory.
> +
> +A second approach that will work slightly better on its own is adding an option
> +not to evict when creating a new job (any kind). If all of userspace opts in
> +to this flag, it would prevent cooperating userspace from forced terminating
> +older compute jobs to start a new one.
> +
> +If job preemption and recoverable pagefaults are not available, those are the
> +only approaches possible. So even with those, you want a separate way of
> +controlling resources. The standard kernel way of doing so is cgroups.
> +
> +This creates a third option, using cgroups to prevent eviction. Both GPU and
> +driver-allocated CPU memory would be accounted to the correct cgroup, and
> +eviction would be made cgroup aware. This allows the GPU to be partitioned
> +into cgroups, that will allow jobs to run next to each other without
> +interference.
> +
> +The interface to the cgroup would be similar to the current CPU memory
> +interface, with similar semantics for min/low/high/max, if eviction can
> +be made cgroup aware. For now only max is implemented.
> +
> +What should be noted is that each memory region (tiled memory for example)
> +should have its own accounting, using $card key0 = value0 key1 = value1.
> +
> +The key is set to the regionid set by the driver, for example "tile0".
> +For the value of $card, we use drmGetUnique().
> diff --git a/include/linux/cgroup_dev.h b/include/linux/cgroup_dev.h
> new file mode 100644
> index 0000000000000..c6311d1d3ce48
> --- /dev/null
> +++ b/include/linux/cgroup_dev.h
> @@ -0,0 +1,91 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef _CGROUP_DEV_H
> +#define _CGROUP_DEV_H
> +
> +#include <linux/types.h>
> +#include <linux/llist.h>
> +
> +struct dev_cgroup_pool_state;
> +
> +/*
> + * Use 8 as max, because of N^2 lookup when setting things, can be bumped if needed
> + * Identical to TTM_NUM_MEM_TYPES to allow simplifying that code.
> + */
> +#define DEVICE_CGROUP_MAX_REGIONS 8
> +
> +/* Public definition of cgroup device, should not be modified after _register() */
> +struct dev_cgroup_device {
> + struct {
> + u64 size;
> + const char *name;
> + } regions[DEVICE_CGROUP_MAX_REGIONS];
> +
> + int num_regions;
> +
> + /* used by cgroups, do not use */
> + void *priv;
> +};
> +
> +#if IS_ENABLED(CONFIG_CGROUP_DEV)
> +int dev_cgroup_register_device(struct dev_cgroup_device *cgdev,
> + const char *name);
> +void dev_cgroup_unregister_device(struct dev_cgroup_device *cgdev);
> +int dev_cgroup_try_charge(struct dev_cgroup_device *cgdev,
> + u32 index, u64 size,
> + struct dev_cgroup_pool_state **ret_pool,
> + struct dev_cgroup_pool_state **ret_limit_pool);
> +void dev_cgroup_uncharge(struct dev_cgroup_pool_state *pool,
> + u32 index, u64 size);
> +bool dev_cgroup_state_evict_valuable(struct dev_cgroup_device *dev, int index,
> + struct dev_cgroup_pool_state *limit_pool,
> + struct dev_cgroup_pool_state *test_pool,
> + bool ignore_low, bool *ret_hit_low);
> +
> +void dev_cgroup_pool_state_put(struct dev_cgroup_pool_state *pool);
> +#else
> +static inline int
> +dev_cgroup_register_device(struct dev_cgroup_device *cgdev,
> + const char *name)
> +{
> + return 0;
> +}
> +
> +static inline void dev_cgroup_unregister_device(struct dev_cgroup_device *cgdev)
> +{
> +}
> +
> +static int int dev_cgroup_try_charge(struct dev_cgroup_device *cgdev,
> + u32 index, u64 size,
> + struct dev_cgroup_pool_state **ret_pool,
> + struct dev_cgroup_pool_state **ret_limit_pool);
> +{
> + *ret_pool = NULL;
> +
> + if (ret_limit_pool)
> + *ret_limit_pool = NULL;
> +
> + return 0;
> +}
> +
> +static inline void dev_cgroup_uncharge(struct dev_cgroup_pool_state *pool,
> + u32 index, u64 size)
> +{ }
> +
> +static inline
> +bool dev_cgroup_state_evict_valuable(struct dev_cgroup_device *dev, int index,
> + struct dev_cgroup_pool_state *limit_pool,
> + struct dev_cgroup_pool_state *test_pool,
> + bool ignore_low, bool *ret_hit_low)
> +{
> + return true;
> +}
> +
> +static inline void dev_cgroup_pool_state_put(struct dev_cgroup_pool_state *pool)
> +{ }
> +
> +#endif
> +#endif /* _CGROUP_DEV_H */
> diff --git a/include/linux/cgroup_subsys.h b/include/linux/cgroup_subsys.h
> index 4452354872307..898340cfe5843 100644
> --- a/include/linux/cgroup_subsys.h
> +++ b/include/linux/cgroup_subsys.h
> @@ -65,6 +65,10 @@ SUBSYS(rdma)
> SUBSYS(misc)
> #endif
>
> +#if IS_ENABLED(CONFIG_CGROUP_DEV)
> +SUBSYS(dev)
> +#endif
> +
> /*
> * The following subsystems are not supported on the default hierarchy.
> */
> diff --git a/include/linux/page_counter.h b/include/linux/page_counter.h
> index 79dbd8bc35a72..d75376a1694ee 100644
> --- a/include/linux/page_counter.h
> +++ b/include/linux/page_counter.h
> @@ -96,7 +96,7 @@ static inline void page_counter_reset_watermark(struct page_counter *counter)
> counter->watermark = usage;
> }
>
> -#ifdef CONFIG_MEMCG
> +#if IS_ENABLED(CONFIG_MEMCG) || IS_ENABLED(CONFIG_CGROUP_DEVICE)
> void page_counter_calculate_protection(struct page_counter *root,
> struct page_counter *counter,
> bool recursive_protection);
> diff --git a/init/Kconfig b/init/Kconfig
> index 530a382ee0feb..2da595facd97f 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1123,6 +1123,13 @@ config CGROUP_RDMA
> Attaching processes with active RDMA resources to the cgroup
> hierarchy is allowed even if can cross the hierarchy's limit.
>
> +config CGROUP_DEV
> + bool "Device controller"
> + help
> + Provides the device subsystem controller.
> +
> + ...
> +
> config CGROUP_FREEZER
> bool "Freezer controller"
> help
> diff --git a/kernel/cgroup/Makefile b/kernel/cgroup/Makefile
> index a5c9359d516f8..441d346fdc51f 100644
> --- a/kernel/cgroup/Makefile
> +++ b/kernel/cgroup/Makefile
> @@ -7,4 +7,5 @@ obj-$(CONFIG_CGROUP_RDMA) += rdma.o
> obj-$(CONFIG_CPUSETS) += cpuset.o
> obj-$(CONFIG_CPUSETS_V1) += cpuset-v1.o
> obj-$(CONFIG_CGROUP_MISC) += misc.o
> +obj-$(CONFIG_CGROUP_DEV) += dev.o
> obj-$(CONFIG_CGROUP_DEBUG) += debug.o
> diff --git a/kernel/cgroup/dev.c b/kernel/cgroup/dev.c
> new file mode 100644
> index 0000000000000..e422ccbfbc444
> --- /dev/null
> +++ b/kernel/cgroup/dev.c
> @@ -0,0 +1,893 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2023-2024 Intel Corporation (Maarten Lankhorst <dev@lankhorst.se>)
> + * Copyright 2024 Red Hat (Maxime Ripard <mripard@kernel.org>)
> + * Partially based on the rdma and misc controllers, which bear the following copyrights:
> + *
> + * Copyright 2020 Google LLC
> + * Copyright (C) 2016 Parav Pandit <pandit.parav@gmail.com>
> + */
> +
> +#include <linux/cgroup.h>
> +#include <linux/cgroup_dev.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +#include <linux/page_counter.h>
> +#include <linux/parser.h>
> +#include <linux/slab.h>
> +
> +struct devcg_device {
> + /**
> + * @ref: References keeping the device alive.
> + * Keeps the device reference alive after a succesful RCU lookup.
> + */
> + struct kref ref;
> +
> + /** @rcu: RCU head for freeing */
> + struct rcu_head rcu;
> +
> + /**
> + * @dev_node: Linked into &devcg_devices list.
> + * Protected by RCU and global spinlock.
> + */
> + struct list_head dev_node;
> +
> + /**
> + * @pools: List of pools linked to this device.
> + * Protected by global spinlock only
> + */
> + struct list_head pools;
> +
> + /**
> + * @base: Copy of the struct passed on register.
> + * A copy is made to prevent lifetime issues. devcg_device may
> + * be kept alive when changing cgroups values concurrently through
> + * rcu lookups.
> + */
> + struct dev_cgroup_device base;
> +
> + /** @name: Name describing the node, set by dev_cgroup_register_device */
> + const char *name;
> +
> + /**
> + * @unregistered: Whether the device is unregistered by its caller.
> + * No new pools should be added to the device afterwards.
> + */
> + bool unregistered;
> +};
> +
> +struct devcg_state {
> + struct cgroup_subsys_state css;
> +
> + struct list_head pools;
> +};
> +
> +struct dev_cgroup_pool_state {
> + struct devcg_device *device;
> + struct devcg_state *cs;
> +
> + /* css node, RCU protected against device teardown */
> + struct list_head css_node;
> +
> + /* dev node, no RCU protection required */
> + struct list_head dev_node;
> +
> + int num_res, inited;
> + struct rcu_head rcu;
> +
> + struct devcg_pool_res {
> + struct page_counter cnt;
> + } resources[];
> +};
> +
> +/*
> + * 3 operations require locking protection:
> + * - Registering and unregistering device to/from list, requires global lock.
> + * - Adding a dev_cgroup_pool_state to a CSS, removing when CSS is freed.
> + * - Adding a dev_cgroup_pool_state to a device list.
> + *
> + * Since for the most common operations RCU provides enough protection, I
> + * do not think more granular locking makes sense. Most protection is offered
> + * by RCU and the lockless operating page_counter.
> + */
> +static DEFINE_SPINLOCK(devcg_lock);
> +static LIST_HEAD(devcg_devices);
> +
> +static inline struct devcg_state *
> +css_to_devcs(struct cgroup_subsys_state *css)
> +{
> + return container_of(css, struct devcg_state, css);
> +}
> +
> +static inline struct devcg_state *get_current_devcs(void)
> +{
> + return css_to_devcs(task_get_css(current, dev_cgrp_id));
> +}
> +
> +static struct devcg_state *parent_devcs(struct devcg_state *cg)
> +{
> + return cg->css.parent ? css_to_devcs(cg->css.parent) : NULL;
> +}
> +
> +static void free_cg_pool(struct dev_cgroup_pool_state *pool)
> +{
> + list_del(&pool->dev_node);
> + kfree(pool);
> +}
> +
> +static void
> +set_resource_min(struct dev_cgroup_pool_state *pool, int i, u64 val)
> +{
> + page_counter_set_min(&pool->resources[i].cnt, val);
> +}
> +
> +static void
> +set_resource_low(struct dev_cgroup_pool_state *pool, int i, u64 val)
> +{
> + page_counter_set_low(&pool->resources[i].cnt, val);
> +}
> +
> +static void
> +set_resource_max(struct dev_cgroup_pool_state *pool, int i, u64 val)
> +{
> + page_counter_set_max(&pool->resources[i].cnt, val);
> +}
> +
> +static u64 get_resource_low(struct dev_cgroup_pool_state *pool, int idx)
> +{
> + return pool ? READ_ONCE(pool->resources[idx].cnt.low) : 0;
> +}
> +
> +static u64 get_resource_min(struct dev_cgroup_pool_state *pool, int idx)
> +{
> + return pool ? READ_ONCE(pool->resources[idx].cnt.min) : 0;
> +}
> +
> +static u64 get_resource_max(struct dev_cgroup_pool_state *pool, int idx)
> +{
> + return pool ? READ_ONCE(pool->resources[idx].cnt.max) : PAGE_COUNTER_MAX;
> +}
> +
> +static u64 get_resource_current(struct dev_cgroup_pool_state *pool, int idx)
> +{
> + return pool ? page_counter_read(&pool->resources[idx].cnt) : 0;
> +}
> +
> +static void reset_all_resource_limits(struct dev_cgroup_pool_state *rpool)
> +{
> + int i;
> +
> + for (i = 0; i < rpool->num_res; i++) {
> + set_resource_min(rpool, i, 0);
> + set_resource_low(rpool, i, 0);
> + set_resource_max(rpool, i, PAGE_COUNTER_MAX);
> + }
> +}
> +
> +static void devcs_offline(struct cgroup_subsys_state *css)
> +{
> + struct devcg_state *devcs = css_to_devcs(css);
> + struct dev_cgroup_pool_state *pool;
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(pool, &devcs->pools, css_node)
> + reset_all_resource_limits(pool);
> + rcu_read_unlock();
> +}
> +
> +static void devcs_free(struct cgroup_subsys_state *css)
> +{
> + struct devcg_state *devcs = css_to_devcs(css);
> + struct dev_cgroup_pool_state *pool, *next;
> +
> + spin_lock(&devcg_lock);
> + list_for_each_entry_safe(pool, next, &devcs->pools, css_node) {
> + /*
> + *The pool is dead and all references are 0,
> + * no need for RCU protection with list_del_rcu or freeing.
> + */
> + list_del(&pool->css_node);
> + free_cg_pool(pool);
> + }
> + spin_unlock(&devcg_lock);
> +
> + kfree(devcs);
> +}
> +
> +static struct cgroup_subsys_state *
> +devcs_alloc(struct cgroup_subsys_state *parent_css)
> +{
> + struct devcg_state *devcs = kzalloc(sizeof(*devcs), GFP_KERNEL);
> + if (!devcs)
> + return ERR_PTR(-ENOMEM);
> +
> + INIT_LIST_HEAD(&devcs->pools);
> + return &devcs->css;
> +}
> +
> +static struct dev_cgroup_pool_state *
> +find_cg_pool_locked(struct devcg_state *devcs, struct devcg_device *dev)
> +{
> + struct dev_cgroup_pool_state *pool;
> +
> + list_for_each_entry_rcu(pool, &devcs->pools, css_node, spin_is_locked(&devcg_lock))
> + if (pool->device == dev)
> + return pool;
> +
> + return NULL;
> +}
> +
> +static struct dev_cgroup_pool_state *pool_parent(struct dev_cgroup_pool_state *pool)
> +{
> + if (!pool->resources[0].cnt.parent)
> + return NULL;
> +
> + return container_of(pool->resources[0].cnt.parent, typeof(*pool), resources[0].cnt);
> +}
> +
> +/**
> + * dev_cgroup_state_evict_valuable() - Check if we should evict from test_pool
> + * @dev: &dev_cgroup_device
> + * @index: The index number of the region being tested.
> + * @limit_pool: The pool for which we hit limits
> + * @test_pool: The pool for which to test
> + * @ignore_low: Whether we have to respect low watermarks.
> + * @ret_hit_low: Pointer to whether it makes sense to consider low watermark.
> + *
> + * This function returns true if we can evict from @test_pool, false if not.
> + * When returning false and @ignore_low is false, @ret_hit_low may
> + * be set to true to indicate this function can be retried with @ignore_low
> + * set to true.
> + *
> + * Return: bool
> + */
> +bool dev_cgroup_state_evict_valuable(struct dev_cgroup_device *dev, int index,
> + struct dev_cgroup_pool_state *limit_pool,
> + struct dev_cgroup_pool_state *test_pool,
> + bool ignore_low, bool *ret_hit_low)
> +{
> + struct dev_cgroup_pool_state *pool = test_pool;
> + struct page_counter *climit, *ctest;
> + u64 used, min, low;
> +
> + /* Can always evict from current pool, despite limits */
> + if (limit_pool == test_pool)
> + return true;
> +
> + if (limit_pool) {
> + if (!parent_devcs(limit_pool->cs))
> + return true;
> +
> + for (pool = test_pool; pool && limit_pool != pool; pool = pool_parent(pool))
> + {}
> +
> + if (!pool)
> + return false;
> + } else {
> + /*
> + * If there is no cgroup limiting memory usage, use the root
> + * cgroup instead for limit calculations.
> + */
> + for (limit_pool = test_pool; pool_parent(limit_pool); limit_pool = pool_parent(limit_pool))
> + {}
> + }
> +
> + climit = &limit_pool->resources[index].cnt;
> + ctest = &test_pool->resources[index].cnt;
> +
> + page_counter_calculate_protection(climit, ctest, true);
I realized we can't do this. As the documentation for
page_counter_calculate_protection states:
> WARNING: This function is not stateless! It can only be used as part
> of a top-down tree iteration, not for isolated queries.
I authored a fix with [1], though I'm not super happy with having to
iterate through the entire (sub-)hierarchy like this every time we
consider eviction. If anyone has a better idea, feel free to propose it.
This branch also contains another idea [2][3] I've been playing around
with. Essentially, what I'm trying to solve is TTM preferring to use
system memory over evicting VRAM, even if the new VRAM allocation would
be protected from eviction by low/min memory protection. In my testing,
it leads to a better experience to try evicting unprotected allocations
immediately in that case. I'm fine with this being follow-up work, but
given that the patchset is still in a rather early stage I thought I'd
pitch this now.
Thanks,
Friedrich
[1] https://gitlab.freedesktop.org/pixelcluster/linux/-/commit/1adc35adc2a7d05260275d69f139450c8af5dc3b
[2] https://gitlab.freedesktop.org/pixelcluster/linux/-/commit/3d2cdd25d62c539d80055d615bd0912e5907f639
[3] https://gitlab.freedesktop.org/pixelcluster/linux/-/commit/54446cc696fb8b2cab27b3dcc992a6738f5392ad
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/7] kernel/cgroup: Add "dev" memory accounting cgroup
2024-10-28 14:53 ` Friedrich Vock
@ 2024-11-11 22:53 ` Maarten Lankhorst
2024-11-14 8:45 ` Friedrich Vock
0 siblings, 1 reply; 27+ messages in thread
From: Maarten Lankhorst @ 2024-11-11 22:53 UTC (permalink / raw)
To: Friedrich Vock, intel-xe, linux-kernel, dri-devel, Tejun Heo,
Zefan Li, Johannes Weiner, Andrew Morton
Cc: cgroups, linux-mm, Maxime Ripard
Den 2024-10-28 kl. 15:53, skrev Friedrich Vock:
> On 23.10.24 09:52, Maarten Lankhorst wrote:
>> The initial version was based roughly on the rdma and misc cgroup
>> controllers, with a lot of the accounting code borrowed from rdma.
>>
>> The current version is a complete rewrite with page counter; it uses
>> the same min/low/max semantics as the memory cgroup as a result.
>>
>> There's a small mismatch as TTM uses u64, and page_counter long pages.
>> In practice it's not a problem. 32-bits systems don't really come with
>>> =4GB cards and as long as we're consistently wrong with units, it's
>> fine. The device page size may not be in the same units as kernel page
>> size, and each region might also have a different page size (VRAM vs GART
>> for example).
>>
>> The interface is simple:
>> - populate dev_cgroup_try_charge->regions[..] name and size for each
>> active
>> region, set num_regions accordingly.
>> - Call (dev,drmm)_cgroup_register_device()
>> - Use dev_cgroup_try_charge to check if you can allocate a chunk of
>> memory,
>> use dev_cgroup__uncharge when freeing it. This may return an error
>> code,
>> or -EAGAIN when the cgroup limit is reached. In that case a reference
>> to the limiting pool is returned.
>> - The limiting cs can be used as compare function for
>> dev_cgroup_state_evict_valuable.
>> - After having evicted enough, drop reference to limiting cs with
>> dev_cgroup_pool_state_put.
>>
>> This API allows you to limit device resources with cgroups.
>> You can see the supported cards in /sys/fs/cgroup/dev.region.capacity
>> You need to echo +dev to cgroup.subtree_control, and then you can
>> partition memory.
>>
>> Co-developed-by: Friedrich Vock <friedrich.vock@gmx.de>
>> Signed-off-by: Friedrich Vock <friedrich.vock@gmx.de>
>> Co-developed-by: Maxime Ripard <mripard@kernel.org>
>> Signed-off-by: Maxime Ripard <mripard@kernel.org>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> Documentation/admin-guide/cgroup-v2.rst | 51 ++
>> Documentation/core-api/cgroup.rst | 9 +
>> Documentation/core-api/index.rst | 1 +
>> Documentation/gpu/drm-compute.rst | 54 ++
>> include/linux/cgroup_dev.h | 91 +++
>> include/linux/cgroup_subsys.h | 4 +
>> include/linux/page_counter.h | 2 +-
>> init/Kconfig | 7 +
>> kernel/cgroup/Makefile | 1 +
>> kernel/cgroup/dev.c | 893 ++++++++++++++++++++++++
>> mm/page_counter.c | 4 +-
>> 11 files changed, 1114 insertions(+), 3 deletions(-)
>> create mode 100644 Documentation/core-api/cgroup.rst
>> create mode 100644 Documentation/gpu/drm-compute.rst
>> create mode 100644 include/linux/cgroup_dev.h
>> create mode 100644 kernel/cgroup/dev.c
>>
>> diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/
>> admin-guide/cgroup-v2.rst
>> index 69af2173555fb..e8fe79244af9c 100644
>> --- a/Documentation/admin-guide/cgroup-v2.rst
>> +++ b/Documentation/admin-guide/cgroup-v2.rst
>> @@ -2612,6 +2612,57 @@ RDMA Interface Files
>> mlx4_0 hca_handle=1 hca_object=20
>> ocrdma1 hca_handle=1 hca_object=23
>>
>> +DEV
>> +----
>> +
>> +The "dev" controller regulates the distribution and accounting of
>> +device resources, currently only memory regions. Because each memory
>> +region may have its own page size, which does not have to be equal
>> +to the system page size. the units are in bytes.
>> +
>> +DEV Interface Files
>> +~~~~~~~~~~~~~~~~~~~~
>> +
>> + dev.region.max, dev.region.min, dev.region.low
>> + A readwrite nested-keyed file that exists for all the cgroups
>> + except root that describes current configured resource limit
>> + for a device.
>> +
>> + Lines are keyed by device name and are not ordered.
>> + Each line contains space separated resource name and its configured
>> + limit that can be distributed.
>> +
>> + The following nested keys are defined.
>> +
>> + ==========
>> =======================================================
>> + * Maximum amount of bytes that allocatable in this region
>> + ==========
>> =======================================================
>> +
>> + An example for xe follows::
>> +
>> + drm/0000:03:00.0 vram0=1073741824 stolen=max
>> +
>> + The semantics are the same as for the memory cgroup controller,
>> and are
>> + calculated in the same way.
>> +
>> + dev.region.capacity
>> + A read-only file that describes maximum region capacity.
>> + It only exists on the root cgroup. Not all memory can be
>> + allocated by cgroups, as the kernel reserves some for
>> + internal use.
>> +
>> + An example for xe follows::
>> +
>> + drm/0000:03:00.0 vram0=8514437120 stolen=67108864
>> +
>> + dev.region.current
>> + A read-only file that describes current resource usage.
>> + It exists for all the cgroup except root.
>> +
>> + An example for xe follows::
>> +
>> + drm/0000:03:00.0 vram0=12550144 stolen=8650752
>> +
>> HugeTLB
>> -------
>>
>> diff --git a/Documentation/core-api/cgroup.rst b/Documentation/core-
>> api/cgroup.rst
>> new file mode 100644
>> index 0000000000000..475b32255bd68
>> --- /dev/null
>> +++ b/Documentation/core-api/cgroup.rst
>> @@ -0,0 +1,9 @@
>> +==================
>> +Cgroup Kernel APIs
>> +==================
>> +
>> +Device Cgroup API (devcg)
>> +=========================
>> +.. kernel-doc:: kernel/cgroup/dev.c
>> + :export:
>> +
>> diff --git a/Documentation/core-api/index.rst b/Documentation/core-
>> api/index.rst
>> index 6a875743dd4b7..dbd6c4f9a6313 100644
>> --- a/Documentation/core-api/index.rst
>> +++ b/Documentation/core-api/index.rst
>> @@ -108,6 +108,7 @@ more memory-management documentation in
>> Documentation/mm/index.rst.
>> dma-isa-lpc
>> swiotlb
>> mm-api
>> + cgroup
>> genalloc
>> pin_user_pages
>> boot-time-mm
>> diff --git a/Documentation/gpu/drm-compute.rst b/Documentation/gpu/
>> drm-compute.rst
>> new file mode 100644
>> index 0000000000000..116270976ef7a
>> --- /dev/null
>> +++ b/Documentation/gpu/drm-compute.rst
>> @@ -0,0 +1,54 @@
>> +==================================
>> +Long running workloads and compute
>> +==================================
>> +
>> +Long running workloads (compute) are workloads that will not complete
>> in 10
>> +seconds. (The time let the user wait before he reaches for the power
>> button).
>> +This means that other techniques need to be used to manage those
>> workloads,
>> +that cannot use fences.
>> +
>> +Some hardware may schedule compute jobs, and have no way to pre-empt
>> them, or
>> +have their memory swapped out from them. Or they simply want their
>> workload
>> +not to be preempted or swapped out at all.
>> +
>> +This means that it differs from what is described in driver-api/dma-
>> buf.rst.
>> +
>> +As with normal compute jobs, dma-fence may not be used at all. In
>> this case,
>> +not even to force preemption. The driver with is simply forced to
>> unmap a BO
>> +from the long compute job's address space on unbind immediately, not
>> even
>> +waiting for the workload to complete. Effectively this terminates the
>> workload
>> +when there is no hardware support to recover.
>> +
>> +Since this is undesirable, there need to be mitigations to prevent a
>> workload
>> +from being terminated. There are several possible approach, all with
>> their
>> +advantages and drawbacks.
>> +
>> +The first approach you will likely try is to pin all buffers used by
>> compute.
>> +This guarantees that the job will run uninterrupted, but also allows
>> a very
>> +denial of service attack by pinning as much memory as possible,
>> hogging the
>> +all GPU memory, and possibly a huge chunk of CPU memory.
>> +
>> +A second approach that will work slightly better on its own is adding
>> an option
>> +not to evict when creating a new job (any kind). If all of userspace
>> opts in
>> +to this flag, it would prevent cooperating userspace from forced
>> terminating
>> +older compute jobs to start a new one.
>> +
>> +If job preemption and recoverable pagefaults are not available, those
>> are the
>> +only approaches possible. So even with those, you want a separate way of
>> +controlling resources. The standard kernel way of doing so is cgroups.
>> +
>> +This creates a third option, using cgroups to prevent eviction. Both
>> GPU and
>> +driver-allocated CPU memory would be accounted to the correct cgroup,
>> and
>> +eviction would be made cgroup aware. This allows the GPU to be
>> partitioned
>> +into cgroups, that will allow jobs to run next to each other without
>> +interference.
>> +
>> +The interface to the cgroup would be similar to the current CPU memory
>> +interface, with similar semantics for min/low/high/max, if eviction can
>> +be made cgroup aware. For now only max is implemented.
>> +
>> +What should be noted is that each memory region (tiled memory for
>> example)
>> +should have its own accounting, using $card key0 = value0 key1 = value1.
>> +
>> +The key is set to the regionid set by the driver, for example "tile0".
>> +For the value of $card, we use drmGetUnique().
>> diff --git a/include/linux/cgroup_dev.h b/include/linux/cgroup_dev.h
>> new file mode 100644
>> index 0000000000000..c6311d1d3ce48
>> --- /dev/null
>> +++ b/include/linux/cgroup_dev.h
>> @@ -0,0 +1,91 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> + */
>> +
>> +#ifndef _CGROUP_DEV_H
>> +#define _CGROUP_DEV_H
>> +
>> +#include <linux/types.h>
>> +#include <linux/llist.h>
>> +
>> +struct dev_cgroup_pool_state;
>> +
>> +/*
>> + * Use 8 as max, because of N^2 lookup when setting things, can be
>> bumped if needed
>> + * Identical to TTM_NUM_MEM_TYPES to allow simplifying that code.
>> + */
>> +#define DEVICE_CGROUP_MAX_REGIONS 8
>> +
>> +/* Public definition of cgroup device, should not be modified after
>> _register() */
>> +struct dev_cgroup_device {
>> + struct {
>> + u64 size;
>> + const char *name;
>> + } regions[DEVICE_CGROUP_MAX_REGIONS];
>> +
>> + int num_regions;
>> +
>> + /* used by cgroups, do not use */
>> + void *priv;
>> +};
>> +
>> +#if IS_ENABLED(CONFIG_CGROUP_DEV)
>> +int dev_cgroup_register_device(struct dev_cgroup_device *cgdev,
>> + const char *name);
>> +void dev_cgroup_unregister_device(struct dev_cgroup_device *cgdev);
>> +int dev_cgroup_try_charge(struct dev_cgroup_device *cgdev,
>> + u32 index, u64 size,
>> + struct dev_cgroup_pool_state **ret_pool,
>> + struct dev_cgroup_pool_state **ret_limit_pool);
>> +void dev_cgroup_uncharge(struct dev_cgroup_pool_state *pool,
>> + u32 index, u64 size);
>> +bool dev_cgroup_state_evict_valuable(struct dev_cgroup_device *dev,
>> int index,
>> + struct dev_cgroup_pool_state *limit_pool,
>> + struct dev_cgroup_pool_state *test_pool,
>> + bool ignore_low, bool *ret_hit_low);
>> +
>> +void dev_cgroup_pool_state_put(struct dev_cgroup_pool_state *pool);
>> +#else
>> +static inline int
>> +dev_cgroup_register_device(struct dev_cgroup_device *cgdev,
>> + const char *name)
>> +{
>> + return 0;
>> +}
>> +
>> +static inline void dev_cgroup_unregister_device(struct
>> dev_cgroup_device *cgdev)
>> +{
>> +}
>> +
>> +static int int dev_cgroup_try_charge(struct dev_cgroup_device *cgdev,
>> + u32 index, u64 size,
>> + struct dev_cgroup_pool_state **ret_pool,
>> + struct dev_cgroup_pool_state **ret_limit_pool);
>> +{
>> + *ret_pool = NULL;
>> +
>> + if (ret_limit_pool)
>> + *ret_limit_pool = NULL;
>> +
>> + return 0;
>> +}
>> +
>> +static inline void dev_cgroup_uncharge(struct dev_cgroup_pool_state
>> *pool,
>> + u32 index, u64 size)
>> +{ }
>> +
>> +static inline
>> +bool dev_cgroup_state_evict_valuable(struct dev_cgroup_device *dev,
>> int index,
>> + struct dev_cgroup_pool_state *limit_pool,
>> + struct dev_cgroup_pool_state *test_pool,
>> + bool ignore_low, bool *ret_hit_low)
>> +{
>> + return true;
>> +}
>> +
>> +static inline void dev_cgroup_pool_state_put(struct
>> dev_cgroup_pool_state *pool)
>> +{ }
>> +
>> +#endif
>> +#endif /* _CGROUP_DEV_H */
>> diff --git a/include/linux/cgroup_subsys.h b/include/linux/
>> cgroup_subsys.h
>> index 4452354872307..898340cfe5843 100644
>> --- a/include/linux/cgroup_subsys.h
>> +++ b/include/linux/cgroup_subsys.h
>> @@ -65,6 +65,10 @@ SUBSYS(rdma)
>> SUBSYS(misc)
>> #endif
>>
>> +#if IS_ENABLED(CONFIG_CGROUP_DEV)
>> +SUBSYS(dev)
>> +#endif
>> +
>> /*
>> * The following subsystems are not supported on the default hierarchy.
>> */
>> diff --git a/include/linux/page_counter.h b/include/linux/page_counter.h
>> index 79dbd8bc35a72..d75376a1694ee 100644
>> --- a/include/linux/page_counter.h
>> +++ b/include/linux/page_counter.h
>> @@ -96,7 +96,7 @@ static inline void
>> page_counter_reset_watermark(struct page_counter *counter)
>> counter->watermark = usage;
>> }
>>
>> -#ifdef CONFIG_MEMCG
>> +#if IS_ENABLED(CONFIG_MEMCG) || IS_ENABLED(CONFIG_CGROUP_DEVICE)
>> void page_counter_calculate_protection(struct page_counter *root,
>> struct page_counter *counter,
>> bool recursive_protection);
>> diff --git a/init/Kconfig b/init/Kconfig
>> index 530a382ee0feb..2da595facd97f 100644
>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -1123,6 +1123,13 @@ config CGROUP_RDMA
>> Attaching processes with active RDMA resources to the cgroup
>> hierarchy is allowed even if can cross the hierarchy's limit.
>>
>> +config CGROUP_DEV
>> + bool "Device controller"
>> + help
>> + Provides the device subsystem controller.
>> +
>> + ...
>> +
>> config CGROUP_FREEZER
>> bool "Freezer controller"
>> help
>> diff --git a/kernel/cgroup/Makefile b/kernel/cgroup/Makefile
>> index a5c9359d516f8..441d346fdc51f 100644
>> --- a/kernel/cgroup/Makefile
>> +++ b/kernel/cgroup/Makefile
>> @@ -7,4 +7,5 @@ obj-$(CONFIG_CGROUP_RDMA) += rdma.o
>> obj-$(CONFIG_CPUSETS) += cpuset.o
>> obj-$(CONFIG_CPUSETS_V1) += cpuset-v1.o
>> obj-$(CONFIG_CGROUP_MISC) += misc.o
>> +obj-$(CONFIG_CGROUP_DEV) += dev.o
>> obj-$(CONFIG_CGROUP_DEBUG) += debug.o
>> diff --git a/kernel/cgroup/dev.c b/kernel/cgroup/dev.c
>> new file mode 100644
>> index 0000000000000..e422ccbfbc444
>> --- /dev/null
>> +++ b/kernel/cgroup/dev.c
>> @@ -0,0 +1,893 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright 2023-2024 Intel Corporation (Maarten Lankhorst
>> <dev@lankhorst.se>)
>> + * Copyright 2024 Red Hat (Maxime Ripard <mripard@kernel.org>)
>> + * Partially based on the rdma and misc controllers, which bear the
>> following copyrights:
>> + *
>> + * Copyright 2020 Google LLC
>> + * Copyright (C) 2016 Parav Pandit <pandit.parav@gmail.com>
>> + */
>> +
>> +#include <linux/cgroup.h>
>> +#include <linux/cgroup_dev.h>
>> +#include <linux/list.h>
>> +#include <linux/mutex.h>
>> +#include <linux/page_counter.h>
>> +#include <linux/parser.h>
>> +#include <linux/slab.h>
>> +
>> +struct devcg_device {
>> + /**
>> + * @ref: References keeping the device alive.
>> + * Keeps the device reference alive after a succesful RCU lookup.
>> + */
>> + struct kref ref;
>> +
>> + /** @rcu: RCU head for freeing */
>> + struct rcu_head rcu;
>> +
>> + /**
>> + * @dev_node: Linked into &devcg_devices list.
>> + * Protected by RCU and global spinlock.
>> + */
>> + struct list_head dev_node;
>> +
>> + /**
>> + * @pools: List of pools linked to this device.
>> + * Protected by global spinlock only
>> + */
>> + struct list_head pools;
>> +
>> + /**
>> + * @base: Copy of the struct passed on register.
>> + * A copy is made to prevent lifetime issues. devcg_device may
>> + * be kept alive when changing cgroups values concurrently through
>> + * rcu lookups.
>> + */
>> + struct dev_cgroup_device base;
>> +
>> + /** @name: Name describing the node, set by
>> dev_cgroup_register_device */
>> + const char *name;
>> +
>> + /**
>> + * @unregistered: Whether the device is unregistered by its caller.
>> + * No new pools should be added to the device afterwards.
>> + */
>> + bool unregistered;
>> +};
>> +
>> +struct devcg_state {
>> + struct cgroup_subsys_state css;
>> +
>> + struct list_head pools;
>> +};
>> +
>> +struct dev_cgroup_pool_state {
>> + struct devcg_device *device;
>> + struct devcg_state *cs;
>> +
>> + /* css node, RCU protected against device teardown */
>> + struct list_head css_node;
>> +
>> + /* dev node, no RCU protection required */
>> + struct list_head dev_node;
>> +
>> + int num_res, inited;
>> + struct rcu_head rcu;
>> +
>> + struct devcg_pool_res {
>> + struct page_counter cnt;
>> + } resources[];
>> +};
>> +
>> +/*
>> + * 3 operations require locking protection:
>> + * - Registering and unregistering device to/from list, requires
>> global lock.
>> + * - Adding a dev_cgroup_pool_state to a CSS, removing when CSS is
>> freed.
>> + * - Adding a dev_cgroup_pool_state to a device list.
>> + *
>> + * Since for the most common operations RCU provides enough
>> protection, I
>> + * do not think more granular locking makes sense. Most protection is
>> offered
>> + * by RCU and the lockless operating page_counter.
>> + */
>> +static DEFINE_SPINLOCK(devcg_lock);
>> +static LIST_HEAD(devcg_devices);
>> +
>> +static inline struct devcg_state *
>> +css_to_devcs(struct cgroup_subsys_state *css)
>> +{
>> + return container_of(css, struct devcg_state, css);
>> +}
>> +
>> +static inline struct devcg_state *get_current_devcs(void)
>> +{
>> + return css_to_devcs(task_get_css(current, dev_cgrp_id));
>> +}
>> +
>> +static struct devcg_state *parent_devcs(struct devcg_state *cg)
>> +{
>> + return cg->css.parent ? css_to_devcs(cg->css.parent) : NULL;
>> +}
>> +
>> +static void free_cg_pool(struct dev_cgroup_pool_state *pool)
>> +{
>> + list_del(&pool->dev_node);
>> + kfree(pool);
>> +}
>> +
>> +static void
>> +set_resource_min(struct dev_cgroup_pool_state *pool, int i, u64 val)
>> +{
>> + page_counter_set_min(&pool->resources[i].cnt, val);
>> +}
>> +
>> +static void
>> +set_resource_low(struct dev_cgroup_pool_state *pool, int i, u64 val)
>> +{
>> + page_counter_set_low(&pool->resources[i].cnt, val);
>> +}
>> +
>> +static void
>> +set_resource_max(struct dev_cgroup_pool_state *pool, int i, u64 val)
>> +{
>> + page_counter_set_max(&pool->resources[i].cnt, val);
>> +}
>> +
>> +static u64 get_resource_low(struct dev_cgroup_pool_state *pool, int idx)
>> +{
>> + return pool ? READ_ONCE(pool->resources[idx].cnt.low) : 0;
>> +}
>> +
>> +static u64 get_resource_min(struct dev_cgroup_pool_state *pool, int idx)
>> +{
>> + return pool ? READ_ONCE(pool->resources[idx].cnt.min) : 0;
>> +}
>> +
>> +static u64 get_resource_max(struct dev_cgroup_pool_state *pool, int idx)
>> +{
>> + return pool ? READ_ONCE(pool->resources[idx].cnt.max) :
>> PAGE_COUNTER_MAX;
>> +}
>> +
>> +static u64 get_resource_current(struct dev_cgroup_pool_state *pool,
>> int idx)
>> +{
>> + return pool ? page_counter_read(&pool->resources[idx].cnt) : 0;
>> +}
>> +
>> +static void reset_all_resource_limits(struct dev_cgroup_pool_state
>> *rpool)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < rpool->num_res; i++) {
>> + set_resource_min(rpool, i, 0);
>> + set_resource_low(rpool, i, 0);
>> + set_resource_max(rpool, i, PAGE_COUNTER_MAX);
>> + }
>> +}
>> +
>> +static void devcs_offline(struct cgroup_subsys_state *css)
>> +{
>> + struct devcg_state *devcs = css_to_devcs(css);
>> + struct dev_cgroup_pool_state *pool;
>> +
>> + rcu_read_lock();
>> + list_for_each_entry_rcu(pool, &devcs->pools, css_node)
>> + reset_all_resource_limits(pool);
>> + rcu_read_unlock();
>> +}
>> +
>> +static void devcs_free(struct cgroup_subsys_state *css)
>> +{
>> + struct devcg_state *devcs = css_to_devcs(css);
>> + struct dev_cgroup_pool_state *pool, *next;
>> +
>> + spin_lock(&devcg_lock);
>> + list_for_each_entry_safe(pool, next, &devcs->pools, css_node) {
>> + /*
>> + *The pool is dead and all references are 0,
>> + * no need for RCU protection with list_del_rcu or freeing.
>> + */
>> + list_del(&pool->css_node);
>> + free_cg_pool(pool);
>> + }
>> + spin_unlock(&devcg_lock);
>> +
>> + kfree(devcs);
>> +}
>> +
>> +static struct cgroup_subsys_state *
>> +devcs_alloc(struct cgroup_subsys_state *parent_css)
>> +{
>> + struct devcg_state *devcs = kzalloc(sizeof(*devcs), GFP_KERNEL);
>> + if (!devcs)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + INIT_LIST_HEAD(&devcs->pools);
>> + return &devcs->css;
>> +}
>> +
>> +static struct dev_cgroup_pool_state *
>> +find_cg_pool_locked(struct devcg_state *devcs, struct devcg_device *dev)
>> +{
>> + struct dev_cgroup_pool_state *pool;
>> +
>> + list_for_each_entry_rcu(pool, &devcs->pools, css_node,
>> spin_is_locked(&devcg_lock))
>> + if (pool->device == dev)
>> + return pool;
>> +
>> + return NULL;
>> +}
>> +
>> +static struct dev_cgroup_pool_state *pool_parent(struct
>> dev_cgroup_pool_state *pool)
>> +{
>> + if (!pool->resources[0].cnt.parent)
>> + return NULL;
>> +
>> + return container_of(pool->resources[0].cnt.parent, typeof(*pool),
>> resources[0].cnt);
>> +}
>> +
>> +/**
>> + * dev_cgroup_state_evict_valuable() - Check if we should evict from
>> test_pool
>> + * @dev: &dev_cgroup_device
>> + * @index: The index number of the region being tested.
>> + * @limit_pool: The pool for which we hit limits
>> + * @test_pool: The pool for which to test
>> + * @ignore_low: Whether we have to respect low watermarks.
>> + * @ret_hit_low: Pointer to whether it makes sense to consider low
>> watermark.
>> + *
>> + * This function returns true if we can evict from @test_pool, false
>> if not.
>> + * When returning false and @ignore_low is false, @ret_hit_low may
>> + * be set to true to indicate this function can be retried with
>> @ignore_low
>> + * set to true.
>> + *
>> + * Return: bool
>> + */
>> +bool dev_cgroup_state_evict_valuable(struct dev_cgroup_device *dev,
>> int index,
>> + struct dev_cgroup_pool_state *limit_pool,
>> + struct dev_cgroup_pool_state *test_pool,
>> + bool ignore_low, bool *ret_hit_low)
>> +{
>> + struct dev_cgroup_pool_state *pool = test_pool;
>> + struct page_counter *climit, *ctest;
>> + u64 used, min, low;
>> +
>> + /* Can always evict from current pool, despite limits */
>> + if (limit_pool == test_pool)
>> + return true;
>> +
>> + if (limit_pool) {
>> + if (!parent_devcs(limit_pool->cs))
>> + return true;
>> +
>> + for (pool = test_pool; pool && limit_pool != pool; pool =
>> pool_parent(pool))
>> + {}
>> +
>> + if (!pool)
>> + return false;
>> + } else {
>> + /*
>> + * If there is no cgroup limiting memory usage, use the root
>> + * cgroup instead for limit calculations.
>> + */
>> + for (limit_pool = test_pool; pool_parent(limit_pool);
>> limit_pool = pool_parent(limit_pool))
>> + {}
>> + }
>> +
>> + climit = &limit_pool->resources[index].cnt;
>> + ctest = &test_pool->resources[index].cnt;
>> +
>> + page_counter_calculate_protection(climit, ctest, true);
>
> I realized we can't do this. As the documentation for
> page_counter_calculate_protection states:
>
>> WARNING: This function is not stateless! It can only be used as part
>> of a top-down tree iteration, not for isolated queries.
>
> I authored a fix with [1], though I'm not super happy with having to
> iterate through the entire (sub-)hierarchy like this every time we
> consider eviction. If anyone has a better idea, feel free to propose it.
>
> This branch also contains another idea [2][3] I've been playing around
> with. Essentially, what I'm trying to solve is TTM preferring to use
> system memory over evicting VRAM, even if the new VRAM allocation would
> be protected from eviction by low/min memory protection. In my testing,
> it leads to a better experience to try evicting unprotected allocations
> immediately in that case. I'm fine with this being follow-up work, but
> given that the patchset is still in a rather early stage I thought I'd
> pitch this now.
Hey,
I don't know if an alternative implementation is needed here. I think
the current code is correct. The caller ensures that limit_pool and
test_pool are alive.
I believe it's roughly parallel to try_charge, but in 2 parts. As long
as the caller serializes call to evict_valuable, current code should be
fine?
Kind regards,
Maarten Lankhorst
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/7] kernel/cgroup: Add "dev" memory accounting cgroup
2024-11-11 22:53 ` Maarten Lankhorst
@ 2024-11-14 8:45 ` Friedrich Vock
0 siblings, 0 replies; 27+ messages in thread
From: Friedrich Vock @ 2024-11-14 8:45 UTC (permalink / raw)
To: Maarten Lankhorst, intel-xe, linux-kernel, dri-devel, Tejun Heo,
Zefan Li, Johannes Weiner, Andrew Morton
Cc: cgroups, linux-mm, Maxime Ripard
On 11.11.24 23:53, Maarten Lankhorst wrote:
>
>
> Den 2024-10-28 kl. 15:53, skrev Friedrich Vock:
>> On 23.10.24 09:52, Maarten Lankhorst wrote:
>>> The initial version was based roughly on the rdma and misc cgroup
>>> controllers, with a lot of the accounting code borrowed from rdma.
>>>
>>> The current version is a complete rewrite with page counter; it uses
>>> the same min/low/max semantics as the memory cgroup as a result.
>>>
>>> There's a small mismatch as TTM uses u64, and page_counter long pages.
>>> In practice it's not a problem. 32-bits systems don't really come with
>>>> =4GB cards and as long as we're consistently wrong with units, it's
>>> fine. The device page size may not be in the same units as kernel page
>>> size, and each region might also have a different page size (VRAM vs
>>> GART
>>> for example).
>>>
>>> The interface is simple:
>>> - populate dev_cgroup_try_charge->regions[..] name and size for each
>>> active
>>> region, set num_regions accordingly.
>>> - Call (dev,drmm)_cgroup_register_device()
>>> - Use dev_cgroup_try_charge to check if you can allocate a chunk of
>>> memory,
>>> use dev_cgroup__uncharge when freeing it. This may return an error
>>> code,
>>> or -EAGAIN when the cgroup limit is reached. In that case a reference
>>> to the limiting pool is returned.
>>> - The limiting cs can be used as compare function for
>>> dev_cgroup_state_evict_valuable.
>>> - After having evicted enough, drop reference to limiting cs with
>>> dev_cgroup_pool_state_put.
>>>
>>> This API allows you to limit device resources with cgroups.
>>> You can see the supported cards in /sys/fs/cgroup/dev.region.capacity
>>> You need to echo +dev to cgroup.subtree_control, and then you can
>>> partition memory.
>>>
>>> Co-developed-by: Friedrich Vock <friedrich.vock@gmx.de>
>>> Signed-off-by: Friedrich Vock <friedrich.vock@gmx.de>
>>> Co-developed-by: Maxime Ripard <mripard@kernel.org>
>>> Signed-off-by: Maxime Ripard <mripard@kernel.org>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> ---
>>> Documentation/admin-guide/cgroup-v2.rst | 51 ++
>>> Documentation/core-api/cgroup.rst | 9 +
>>> Documentation/core-api/index.rst | 1 +
>>> Documentation/gpu/drm-compute.rst | 54 ++
>>> include/linux/cgroup_dev.h | 91 +++
>>> include/linux/cgroup_subsys.h | 4 +
>>> include/linux/page_counter.h | 2 +-
>>> init/Kconfig | 7 +
>>> kernel/cgroup/Makefile | 1 +
>>> kernel/cgroup/dev.c | 893 ++++++++++++++++++++++++
>>> mm/page_counter.c | 4 +-
>>> 11 files changed, 1114 insertions(+), 3 deletions(-)
>>> create mode 100644 Documentation/core-api/cgroup.rst
>>> create mode 100644 Documentation/gpu/drm-compute.rst
>>> create mode 100644 include/linux/cgroup_dev.h
>>> create mode 100644 kernel/cgroup/dev.c
>>>
>>> diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/
>>> admin-guide/cgroup-v2.rst
>>> index 69af2173555fb..e8fe79244af9c 100644
>>> --- a/Documentation/admin-guide/cgroup-v2.rst
>>> +++ b/Documentation/admin-guide/cgroup-v2.rst
>>> @@ -2612,6 +2612,57 @@ RDMA Interface Files
>>> mlx4_0 hca_handle=1 hca_object=20
>>> ocrdma1 hca_handle=1 hca_object=23
>>>
>>> +DEV
>>> +----
>>> +
>>> +The "dev" controller regulates the distribution and accounting of
>>> +device resources, currently only memory regions. Because each memory
>>> +region may have its own page size, which does not have to be equal
>>> +to the system page size. the units are in bytes.
>>> +
>>> +DEV Interface Files
>>> +~~~~~~~~~~~~~~~~~~~~
>>> +
>>> + dev.region.max, dev.region.min, dev.region.low
>>> + A readwrite nested-keyed file that exists for all the cgroups
>>> + except root that describes current configured resource limit
>>> + for a device.
>>> +
>>> + Lines are keyed by device name and are not ordered.
>>> + Each line contains space separated resource name and its configured
>>> + limit that can be distributed.
>>> +
>>> + The following nested keys are defined.
>>> +
>>> + ==========
>>> =======================================================
>>> + * Maximum amount of bytes that allocatable in this region
>>> + ==========
>>> =======================================================
>>> +
>>> + An example for xe follows::
>>> +
>>> + drm/0000:03:00.0 vram0=1073741824 stolen=max
>>> +
>>> + The semantics are the same as for the memory cgroup controller,
>>> and are
>>> + calculated in the same way.
>>> +
>>> + dev.region.capacity
>>> + A read-only file that describes maximum region capacity.
>>> + It only exists on the root cgroup. Not all memory can be
>>> + allocated by cgroups, as the kernel reserves some for
>>> + internal use.
>>> +
>>> + An example for xe follows::
>>> +
>>> + drm/0000:03:00.0 vram0=8514437120 stolen=67108864
>>> +
>>> + dev.region.current
>>> + A read-only file that describes current resource usage.
>>> + It exists for all the cgroup except root.
>>> +
>>> + An example for xe follows::
>>> +
>>> + drm/0000:03:00.0 vram0=12550144 stolen=8650752
>>> +
>>> HugeTLB
>>> -------
>>>
>>> diff --git a/Documentation/core-api/cgroup.rst b/Documentation/core-
>>> api/cgroup.rst
>>> new file mode 100644
>>> index 0000000000000..475b32255bd68
>>> --- /dev/null
>>> +++ b/Documentation/core-api/cgroup.rst
>>> @@ -0,0 +1,9 @@
>>> +==================
>>> +Cgroup Kernel APIs
>>> +==================
>>> +
>>> +Device Cgroup API (devcg)
>>> +=========================
>>> +.. kernel-doc:: kernel/cgroup/dev.c
>>> + :export:
>>> +
>>> diff --git a/Documentation/core-api/index.rst b/Documentation/core-
>>> api/index.rst
>>> index 6a875743dd4b7..dbd6c4f9a6313 100644
>>> --- a/Documentation/core-api/index.rst
>>> +++ b/Documentation/core-api/index.rst
>>> @@ -108,6 +108,7 @@ more memory-management documentation in
>>> Documentation/mm/index.rst.
>>> dma-isa-lpc
>>> swiotlb
>>> mm-api
>>> + cgroup
>>> genalloc
>>> pin_user_pages
>>> boot-time-mm
>>> diff --git a/Documentation/gpu/drm-compute.rst b/Documentation/gpu/
>>> drm-compute.rst
>>> new file mode 100644
>>> index 0000000000000..116270976ef7a
>>> --- /dev/null
>>> +++ b/Documentation/gpu/drm-compute.rst
>>> @@ -0,0 +1,54 @@
>>> +==================================
>>> +Long running workloads and compute
>>> +==================================
>>> +
>>> +Long running workloads (compute) are workloads that will not
>>> complete in 10
>>> +seconds. (The time let the user wait before he reaches for the power
>>> button).
>>> +This means that other techniques need to be used to manage those
>>> workloads,
>>> +that cannot use fences.
>>> +
>>> +Some hardware may schedule compute jobs, and have no way to pre-empt
>>> them, or
>>> +have their memory swapped out from them. Or they simply want their
>>> workload
>>> +not to be preempted or swapped out at all.
>>> +
>>> +This means that it differs from what is described in driver-api/dma-
>>> buf.rst.
>>> +
>>> +As with normal compute jobs, dma-fence may not be used at all. In
>>> this case,
>>> +not even to force preemption. The driver with is simply forced to
>>> unmap a BO
>>> +from the long compute job's address space on unbind immediately, not
>>> even
>>> +waiting for the workload to complete. Effectively this terminates
>>> the workload
>>> +when there is no hardware support to recover.
>>> +
>>> +Since this is undesirable, there need to be mitigations to prevent a
>>> workload
>>> +from being terminated. There are several possible approach, all with
>>> their
>>> +advantages and drawbacks.
>>> +
>>> +The first approach you will likely try is to pin all buffers used by
>>> compute.
>>> +This guarantees that the job will run uninterrupted, but also allows
>>> a very
>>> +denial of service attack by pinning as much memory as possible,
>>> hogging the
>>> +all GPU memory, and possibly a huge chunk of CPU memory.
>>> +
>>> +A second approach that will work slightly better on its own is
>>> adding an option
>>> +not to evict when creating a new job (any kind). If all of userspace
>>> opts in
>>> +to this flag, it would prevent cooperating userspace from forced
>>> terminating
>>> +older compute jobs to start a new one.
>>> +
>>> +If job preemption and recoverable pagefaults are not available,
>>> those are the
>>> +only approaches possible. So even with those, you want a separate
>>> way of
>>> +controlling resources. The standard kernel way of doing so is cgroups.
>>> +
>>> +This creates a third option, using cgroups to prevent eviction. Both
>>> GPU and
>>> +driver-allocated CPU memory would be accounted to the correct
>>> cgroup, and
>>> +eviction would be made cgroup aware. This allows the GPU to be
>>> partitioned
>>> +into cgroups, that will allow jobs to run next to each other without
>>> +interference.
>>> +
>>> +The interface to the cgroup would be similar to the current CPU memory
>>> +interface, with similar semantics for min/low/high/max, if eviction can
>>> +be made cgroup aware. For now only max is implemented.
>>> +
>>> +What should be noted is that each memory region (tiled memory for
>>> example)
>>> +should have its own accounting, using $card key0 = value0 key1 =
>>> value1.
>>> +
>>> +The key is set to the regionid set by the driver, for example "tile0".
>>> +For the value of $card, we use drmGetUnique().
>>> diff --git a/include/linux/cgroup_dev.h b/include/linux/cgroup_dev.h
>>> new file mode 100644
>>> index 0000000000000..c6311d1d3ce48
>>> --- /dev/null
>>> +++ b/include/linux/cgroup_dev.h
>>> @@ -0,0 +1,91 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright © 2023 Intel Corporation
>>> + */
>>> +
>>> +#ifndef _CGROUP_DEV_H
>>> +#define _CGROUP_DEV_H
>>> +
>>> +#include <linux/types.h>
>>> +#include <linux/llist.h>
>>> +
>>> +struct dev_cgroup_pool_state;
>>> +
>>> +/*
>>> + * Use 8 as max, because of N^2 lookup when setting things, can be
>>> bumped if needed
>>> + * Identical to TTM_NUM_MEM_TYPES to allow simplifying that code.
>>> + */
>>> +#define DEVICE_CGROUP_MAX_REGIONS 8
>>> +
>>> +/* Public definition of cgroup device, should not be modified after
>>> _register() */
>>> +struct dev_cgroup_device {
>>> + struct {
>>> + u64 size;
>>> + const char *name;
>>> + } regions[DEVICE_CGROUP_MAX_REGIONS];
>>> +
>>> + int num_regions;
>>> +
>>> + /* used by cgroups, do not use */
>>> + void *priv;
>>> +};
>>> +
>>> +#if IS_ENABLED(CONFIG_CGROUP_DEV)
>>> +int dev_cgroup_register_device(struct dev_cgroup_device *cgdev,
>>> + const char *name);
>>> +void dev_cgroup_unregister_device(struct dev_cgroup_device *cgdev);
>>> +int dev_cgroup_try_charge(struct dev_cgroup_device *cgdev,
>>> + u32 index, u64 size,
>>> + struct dev_cgroup_pool_state **ret_pool,
>>> + struct dev_cgroup_pool_state **ret_limit_pool);
>>> +void dev_cgroup_uncharge(struct dev_cgroup_pool_state *pool,
>>> + u32 index, u64 size);
>>> +bool dev_cgroup_state_evict_valuable(struct dev_cgroup_device *dev,
>>> int index,
>>> + struct dev_cgroup_pool_state *limit_pool,
>>> + struct dev_cgroup_pool_state *test_pool,
>>> + bool ignore_low, bool *ret_hit_low);
>>> +
>>> +void dev_cgroup_pool_state_put(struct dev_cgroup_pool_state *pool);
>>> +#else
>>> +static inline int
>>> +dev_cgroup_register_device(struct dev_cgroup_device *cgdev,
>>> + const char *name)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +static inline void dev_cgroup_unregister_device(struct
>>> dev_cgroup_device *cgdev)
>>> +{
>>> +}
>>> +
>>> +static int int dev_cgroup_try_charge(struct dev_cgroup_device *cgdev,
>>> + u32 index, u64 size,
>>> + struct dev_cgroup_pool_state **ret_pool,
>>> + struct dev_cgroup_pool_state **ret_limit_pool);
>>> +{
>>> + *ret_pool = NULL;
>>> +
>>> + if (ret_limit_pool)
>>> + *ret_limit_pool = NULL;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static inline void dev_cgroup_uncharge(struct dev_cgroup_pool_state
>>> *pool,
>>> + u32 index, u64 size)
>>> +{ }
>>> +
>>> +static inline
>>> +bool dev_cgroup_state_evict_valuable(struct dev_cgroup_device *dev,
>>> int index,
>>> + struct dev_cgroup_pool_state *limit_pool,
>>> + struct dev_cgroup_pool_state *test_pool,
>>> + bool ignore_low, bool *ret_hit_low)
>>> +{
>>> + return true;
>>> +}
>>> +
>>> +static inline void dev_cgroup_pool_state_put(struct
>>> dev_cgroup_pool_state *pool)
>>> +{ }
>>> +
>>> +#endif
>>> +#endif /* _CGROUP_DEV_H */
>>> diff --git a/include/linux/cgroup_subsys.h b/include/linux/
>>> cgroup_subsys.h
>>> index 4452354872307..898340cfe5843 100644
>>> --- a/include/linux/cgroup_subsys.h
>>> +++ b/include/linux/cgroup_subsys.h
>>> @@ -65,6 +65,10 @@ SUBSYS(rdma)
>>> SUBSYS(misc)
>>> #endif
>>>
>>> +#if IS_ENABLED(CONFIG_CGROUP_DEV)
>>> +SUBSYS(dev)
>>> +#endif
>>> +
>>> /*
>>> * The following subsystems are not supported on the default
>>> hierarchy.
>>> */
>>> diff --git a/include/linux/page_counter.h b/include/linux/page_counter.h
>>> index 79dbd8bc35a72..d75376a1694ee 100644
>>> --- a/include/linux/page_counter.h
>>> +++ b/include/linux/page_counter.h
>>> @@ -96,7 +96,7 @@ static inline void
>>> page_counter_reset_watermark(struct page_counter *counter)
>>> counter->watermark = usage;
>>> }
>>>
>>> -#ifdef CONFIG_MEMCG
>>> +#if IS_ENABLED(CONFIG_MEMCG) || IS_ENABLED(CONFIG_CGROUP_DEVICE)
>>> void page_counter_calculate_protection(struct page_counter *root,
>>> struct page_counter *counter,
>>> bool recursive_protection);
>>> diff --git a/init/Kconfig b/init/Kconfig
>>> index 530a382ee0feb..2da595facd97f 100644
>>> --- a/init/Kconfig
>>> +++ b/init/Kconfig
>>> @@ -1123,6 +1123,13 @@ config CGROUP_RDMA
>>> Attaching processes with active RDMA resources to the cgroup
>>> hierarchy is allowed even if can cross the hierarchy's limit.
>>>
>>> +config CGROUP_DEV
>>> + bool "Device controller"
>>> + help
>>> + Provides the device subsystem controller.
>>> +
>>> + ...
>>> +
>>> config CGROUP_FREEZER
>>> bool "Freezer controller"
>>> help
>>> diff --git a/kernel/cgroup/Makefile b/kernel/cgroup/Makefile
>>> index a5c9359d516f8..441d346fdc51f 100644
>>> --- a/kernel/cgroup/Makefile
>>> +++ b/kernel/cgroup/Makefile
>>> @@ -7,4 +7,5 @@ obj-$(CONFIG_CGROUP_RDMA) += rdma.o
>>> obj-$(CONFIG_CPUSETS) += cpuset.o
>>> obj-$(CONFIG_CPUSETS_V1) += cpuset-v1.o
>>> obj-$(CONFIG_CGROUP_MISC) += misc.o
>>> +obj-$(CONFIG_CGROUP_DEV) += dev.o
>>> obj-$(CONFIG_CGROUP_DEBUG) += debug.o
>>> diff --git a/kernel/cgroup/dev.c b/kernel/cgroup/dev.c
>>> new file mode 100644
>>> index 0000000000000..e422ccbfbc444
>>> --- /dev/null
>>> +++ b/kernel/cgroup/dev.c
>>> @@ -0,0 +1,893 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright 2023-2024 Intel Corporation (Maarten Lankhorst
>>> <dev@lankhorst.se>)
>>> + * Copyright 2024 Red Hat (Maxime Ripard <mripard@kernel.org>)
>>> + * Partially based on the rdma and misc controllers, which bear the
>>> following copyrights:
>>> + *
>>> + * Copyright 2020 Google LLC
>>> + * Copyright (C) 2016 Parav Pandit <pandit.parav@gmail.com>
>>> + */
>>> +
>>> +#include <linux/cgroup.h>
>>> +#include <linux/cgroup_dev.h>
>>> +#include <linux/list.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/page_counter.h>
>>> +#include <linux/parser.h>
>>> +#include <linux/slab.h>
>>> +
>>> +struct devcg_device {
>>> + /**
>>> + * @ref: References keeping the device alive.
>>> + * Keeps the device reference alive after a succesful RCU lookup.
>>> + */
>>> + struct kref ref;
>>> +
>>> + /** @rcu: RCU head for freeing */
>>> + struct rcu_head rcu;
>>> +
>>> + /**
>>> + * @dev_node: Linked into &devcg_devices list.
>>> + * Protected by RCU and global spinlock.
>>> + */
>>> + struct list_head dev_node;
>>> +
>>> + /**
>>> + * @pools: List of pools linked to this device.
>>> + * Protected by global spinlock only
>>> + */
>>> + struct list_head pools;
>>> +
>>> + /**
>>> + * @base: Copy of the struct passed on register.
>>> + * A copy is made to prevent lifetime issues. devcg_device may
>>> + * be kept alive when changing cgroups values concurrently through
>>> + * rcu lookups.
>>> + */
>>> + struct dev_cgroup_device base;
>>> +
>>> + /** @name: Name describing the node, set by
>>> dev_cgroup_register_device */
>>> + const char *name;
>>> +
>>> + /**
>>> + * @unregistered: Whether the device is unregistered by its caller.
>>> + * No new pools should be added to the device afterwards.
>>> + */
>>> + bool unregistered;
>>> +};
>>> +
>>> +struct devcg_state {
>>> + struct cgroup_subsys_state css;
>>> +
>>> + struct list_head pools;
>>> +};
>>> +
>>> +struct dev_cgroup_pool_state {
>>> + struct devcg_device *device;
>>> + struct devcg_state *cs;
>>> +
>>> + /* css node, RCU protected against device teardown */
>>> + struct list_head css_node;
>>> +
>>> + /* dev node, no RCU protection required */
>>> + struct list_head dev_node;
>>> +
>>> + int num_res, inited;
>>> + struct rcu_head rcu;
>>> +
>>> + struct devcg_pool_res {
>>> + struct page_counter cnt;
>>> + } resources[];
>>> +};
>>> +
>>> +/*
>>> + * 3 operations require locking protection:
>>> + * - Registering and unregistering device to/from list, requires
>>> global lock.
>>> + * - Adding a dev_cgroup_pool_state to a CSS, removing when CSS is
>>> freed.
>>> + * - Adding a dev_cgroup_pool_state to a device list.
>>> + *
>>> + * Since for the most common operations RCU provides enough
>>> protection, I
>>> + * do not think more granular locking makes sense. Most protection
>>> is offered
>>> + * by RCU and the lockless operating page_counter.
>>> + */
>>> +static DEFINE_SPINLOCK(devcg_lock);
>>> +static LIST_HEAD(devcg_devices);
>>> +
>>> +static inline struct devcg_state *
>>> +css_to_devcs(struct cgroup_subsys_state *css)
>>> +{
>>> + return container_of(css, struct devcg_state, css);
>>> +}
>>> +
>>> +static inline struct devcg_state *get_current_devcs(void)
>>> +{
>>> + return css_to_devcs(task_get_css(current, dev_cgrp_id));
>>> +}
>>> +
>>> +static struct devcg_state *parent_devcs(struct devcg_state *cg)
>>> +{
>>> + return cg->css.parent ? css_to_devcs(cg->css.parent) : NULL;
>>> +}
>>> +
>>> +static void free_cg_pool(struct dev_cgroup_pool_state *pool)
>>> +{
>>> + list_del(&pool->dev_node);
>>> + kfree(pool);
>>> +}
>>> +
>>> +static void
>>> +set_resource_min(struct dev_cgroup_pool_state *pool, int i, u64 val)
>>> +{
>>> + page_counter_set_min(&pool->resources[i].cnt, val);
>>> +}
>>> +
>>> +static void
>>> +set_resource_low(struct dev_cgroup_pool_state *pool, int i, u64 val)
>>> +{
>>> + page_counter_set_low(&pool->resources[i].cnt, val);
>>> +}
>>> +
>>> +static void
>>> +set_resource_max(struct dev_cgroup_pool_state *pool, int i, u64 val)
>>> +{
>>> + page_counter_set_max(&pool->resources[i].cnt, val);
>>> +}
>>> +
>>> +static u64 get_resource_low(struct dev_cgroup_pool_state *pool, int
>>> idx)
>>> +{
>>> + return pool ? READ_ONCE(pool->resources[idx].cnt.low) : 0;
>>> +}
>>> +
>>> +static u64 get_resource_min(struct dev_cgroup_pool_state *pool, int
>>> idx)
>>> +{
>>> + return pool ? READ_ONCE(pool->resources[idx].cnt.min) : 0;
>>> +}
>>> +
>>> +static u64 get_resource_max(struct dev_cgroup_pool_state *pool, int
>>> idx)
>>> +{
>>> + return pool ? READ_ONCE(pool->resources[idx].cnt.max) :
>>> PAGE_COUNTER_MAX;
>>> +}
>>> +
>>> +static u64 get_resource_current(struct dev_cgroup_pool_state *pool,
>>> int idx)
>>> +{
>>> + return pool ? page_counter_read(&pool->resources[idx].cnt) : 0;
>>> +}
>>> +
>>> +static void reset_all_resource_limits(struct dev_cgroup_pool_state
>>> *rpool)
>>> +{
>>> + int i;
>>> +
>>> + for (i = 0; i < rpool->num_res; i++) {
>>> + set_resource_min(rpool, i, 0);
>>> + set_resource_low(rpool, i, 0);
>>> + set_resource_max(rpool, i, PAGE_COUNTER_MAX);
>>> + }
>>> +}
>>> +
>>> +static void devcs_offline(struct cgroup_subsys_state *css)
>>> +{
>>> + struct devcg_state *devcs = css_to_devcs(css);
>>> + struct dev_cgroup_pool_state *pool;
>>> +
>>> + rcu_read_lock();
>>> + list_for_each_entry_rcu(pool, &devcs->pools, css_node)
>>> + reset_all_resource_limits(pool);
>>> + rcu_read_unlock();
>>> +}
>>> +
>>> +static void devcs_free(struct cgroup_subsys_state *css)
>>> +{
>>> + struct devcg_state *devcs = css_to_devcs(css);
>>> + struct dev_cgroup_pool_state *pool, *next;
>>> +
>>> + spin_lock(&devcg_lock);
>>> + list_for_each_entry_safe(pool, next, &devcs->pools, css_node) {
>>> + /*
>>> + *The pool is dead and all references are 0,
>>> + * no need for RCU protection with list_del_rcu or freeing.
>>> + */
>>> + list_del(&pool->css_node);
>>> + free_cg_pool(pool);
>>> + }
>>> + spin_unlock(&devcg_lock);
>>> +
>>> + kfree(devcs);
>>> +}
>>> +
>>> +static struct cgroup_subsys_state *
>>> +devcs_alloc(struct cgroup_subsys_state *parent_css)
>>> +{
>>> + struct devcg_state *devcs = kzalloc(sizeof(*devcs), GFP_KERNEL);
>>> + if (!devcs)
>>> + return ERR_PTR(-ENOMEM);
>>> +
>>> + INIT_LIST_HEAD(&devcs->pools);
>>> + return &devcs->css;
>>> +}
>>> +
>>> +static struct dev_cgroup_pool_state *
>>> +find_cg_pool_locked(struct devcg_state *devcs, struct devcg_device
>>> *dev)
>>> +{
>>> + struct dev_cgroup_pool_state *pool;
>>> +
>>> + list_for_each_entry_rcu(pool, &devcs->pools, css_node,
>>> spin_is_locked(&devcg_lock))
>>> + if (pool->device == dev)
>>> + return pool;
>>> +
>>> + return NULL;
>>> +}
>>> +
>>> +static struct dev_cgroup_pool_state *pool_parent(struct
>>> dev_cgroup_pool_state *pool)
>>> +{
>>> + if (!pool->resources[0].cnt.parent)
>>> + return NULL;
>>> +
>>> + return container_of(pool->resources[0].cnt.parent,
>>> typeof(*pool), resources[0].cnt);
>>> +}
>>> +
>>> +/**
>>> + * dev_cgroup_state_evict_valuable() - Check if we should evict from
>>> test_pool
>>> + * @dev: &dev_cgroup_device
>>> + * @index: The index number of the region being tested.
>>> + * @limit_pool: The pool for which we hit limits
>>> + * @test_pool: The pool for which to test
>>> + * @ignore_low: Whether we have to respect low watermarks.
>>> + * @ret_hit_low: Pointer to whether it makes sense to consider low
>>> watermark.
>>> + *
>>> + * This function returns true if we can evict from @test_pool, false
>>> if not.
>>> + * When returning false and @ignore_low is false, @ret_hit_low may
>>> + * be set to true to indicate this function can be retried with
>>> @ignore_low
>>> + * set to true.
>>> + *
>>> + * Return: bool
>>> + */
>>> +bool dev_cgroup_state_evict_valuable(struct dev_cgroup_device *dev,
>>> int index,
>>> + struct dev_cgroup_pool_state *limit_pool,
>>> + struct dev_cgroup_pool_state *test_pool,
>>> + bool ignore_low, bool *ret_hit_low)
>>> +{
>>> + struct dev_cgroup_pool_state *pool = test_pool;
>>> + struct page_counter *climit, *ctest;
>>> + u64 used, min, low;
>>> +
>>> + /* Can always evict from current pool, despite limits */
>>> + if (limit_pool == test_pool)
>>> + return true;
>>> +
>>> + if (limit_pool) {
>>> + if (!parent_devcs(limit_pool->cs))
>>> + return true;
>>> +
>>> + for (pool = test_pool; pool && limit_pool != pool; pool =
>>> pool_parent(pool))
>>> + {}
>>> +
>>> + if (!pool)
>>> + return false;
>>> + } else {
>>> + /*
>>> + * If there is no cgroup limiting memory usage, use the root
>>> + * cgroup instead for limit calculations.
>>> + */
>>> + for (limit_pool = test_pool; pool_parent(limit_pool);
>>> limit_pool = pool_parent(limit_pool))
>>> + {}
>>> + }
>>> +
>>> + climit = &limit_pool->resources[index].cnt;
>>> + ctest = &test_pool->resources[index].cnt;
>>> +
>>> + page_counter_calculate_protection(climit, ctest, true);
>>
>> I realized we can't do this. As the documentation for
>> page_counter_calculate_protection states:
>>
>>> WARNING: This function is not stateless! It can only be used as part
>>> of a top-down tree iteration, not for isolated queries.
>>
>> I authored a fix with [1], though I'm not super happy with having to
>> iterate through the entire (sub-)hierarchy like this every time we
>> consider eviction. If anyone has a better idea, feel free to propose it.
>>
>> This branch also contains another idea [2][3] I've been playing around
>> with. Essentially, what I'm trying to solve is TTM preferring to use
>> system memory over evicting VRAM, even if the new VRAM allocation would
>> be protected from eviction by low/min memory protection. In my testing,
>> it leads to a better experience to try evicting unprotected allocations
>> immediately in that case. I'm fine with this being follow-up work, but
>> given that the patchset is still in a rather early stage I thought I'd
>> pitch this now.
> Hey,
>
> I don't know if an alternative implementation is needed here. I think
> the current code is correct. The caller ensures that limit_pool and
> test_pool are alive.
>
The issue is not about lifetimes. If you look into the implementation of
page_counter_calculate_protection, it uses the parent's elow as part of
calculating the current node's elow. However, unless we do a top-down
iteration, the parent's elow value may be outdated or might never have
been calculated at all yet. This is why the comment about requiring a
top-down iteration exists, and I don't see why it shouldn't apply to us.
Regards,
Friedrich
> I believe it's roughly parallel to try_charge, but in 2 parts. As long
> as the caller serializes call to evict_valuable, current code should be
> fine?
>
> Kind regards,
> Maarten Lankhorst
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 2/7] drm/drv: Add drmm cgroup registration for dev cgroups.
2024-10-23 7:52 [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup Maarten Lankhorst
2024-10-23 7:52 ` [PATCH 1/7] kernel/cgroup: " Maarten Lankhorst
@ 2024-10-23 7:52 ` Maarten Lankhorst
2024-10-23 8:45 ` Jani Nikula
` (2 more replies)
2024-10-23 7:52 ` [PATCH 3/7] drm/ttm: Handle cgroup based eviction in TTM Maarten Lankhorst
` (5 subsequent siblings)
7 siblings, 3 replies; 27+ messages in thread
From: Maarten Lankhorst @ 2024-10-23 7:52 UTC (permalink / raw)
To: intel-xe, linux-kernel, dri-devel, Tejun Heo, Zefan Li,
Johannes Weiner, Andrew Morton
Cc: Friedrich Vock, cgroups, linux-mm, Maxime Ripard, Maarten Lankhorst
From: Maxime Ripard <mripard@kernel.org>
Drivers will need to register a cgroup device at probe time, so let's
give them a drm-managed helper.
Signed-off-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/drm_drv.c | 21 +++++++++++++++++++++
include/drm/drm_drv.h | 4 ++++
2 files changed, 25 insertions(+)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index c2c172eb25df7..251d1d69b09c5 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -26,6 +26,7 @@
* DEALINGS IN THE SOFTWARE.
*/
+#include <linux/cgroup_dev.h>
#include <linux/debugfs.h>
#include <linux/fs.h>
#include <linux/module.h>
@@ -820,6 +821,26 @@ void drm_dev_put(struct drm_device *dev)
}
EXPORT_SYMBOL(drm_dev_put);
+static inline void drmm_cg_unregister_device(struct drm_device *dev, void *arg)
+{
+ dev_cgroup_unregister_device(arg);
+}
+
+int drmm_cgroup_register_device(struct drm_device *dev,
+ struct dev_cgroup_device *cgdev)
+{
+ int ret;
+ char dev_name[32];
+
+ snprintf(dev_name, sizeof(dev_name), "drm/%s", dev->unique);
+ ret = dev_cgroup_register_device(cgdev, dev_name);
+ if (ret)
+ return ret;
+
+ return drmm_add_action_or_reset(dev, drmm_cg_unregister_device, cgdev);
+}
+EXPORT_SYMBOL_GPL(drmm_cgroup_register_device);
+
static int create_compat_control_link(struct drm_device *dev)
{
struct drm_minor *minor;
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 1bbbcb8e2d231..3e83c7ce7f2d0 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -34,6 +34,7 @@
#include <drm/drm_device.h>
+struct dev_cgroup_device;
struct drm_fb_helper;
struct drm_fb_helper_surface_size;
struct drm_file;
@@ -438,6 +439,9 @@ void *__devm_drm_dev_alloc(struct device *parent,
const struct drm_driver *driver,
size_t size, size_t offset);
+int drmm_cgroup_register_device(struct drm_device *dev,
+ struct dev_cgroup_device *cgdev);
+
/**
* devm_drm_dev_alloc - Resource managed allocation of a &drm_device instance
* @parent: Parent device object
--
2.45.2
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 2/7] drm/drv: Add drmm cgroup registration for dev cgroups.
2024-10-23 7:52 ` [PATCH 2/7] drm/drv: Add drmm cgroup registration for dev cgroups Maarten Lankhorst
@ 2024-10-23 8:45 ` Jani Nikula
2024-10-24 10:35 ` kernel test robot
2024-10-24 15:42 ` kernel test robot
2 siblings, 0 replies; 27+ messages in thread
From: Jani Nikula @ 2024-10-23 8:45 UTC (permalink / raw)
To: Maarten Lankhorst, intel-xe, linux-kernel, dri-devel, Tejun Heo,
Zefan Li, Johannes Weiner, Andrew Morton
Cc: Friedrich Vock, cgroups, linux-mm, Maxime Ripard, Maarten Lankhorst
On Wed, 23 Oct 2024, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
> From: Maxime Ripard <mripard@kernel.org>
>
> Drivers will need to register a cgroup device at probe time, so let's
> give them a drm-managed helper.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/drm_drv.c | 21 +++++++++++++++++++++
> include/drm/drm_drv.h | 4 ++++
> 2 files changed, 25 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index c2c172eb25df7..251d1d69b09c5 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -26,6 +26,7 @@
> * DEALINGS IN THE SOFTWARE.
> */
>
> +#include <linux/cgroup_dev.h>
> #include <linux/debugfs.h>
> #include <linux/fs.h>
> #include <linux/module.h>
> @@ -820,6 +821,26 @@ void drm_dev_put(struct drm_device *dev)
> }
> EXPORT_SYMBOL(drm_dev_put);
>
> +static inline void drmm_cg_unregister_device(struct drm_device *dev, void *arg)
Nitpick, inline in .c files is mostly useless, just use static and let
the compiler do its job.
BR,
Jani.
> +{
> + dev_cgroup_unregister_device(arg);
> +}
> +
> +int drmm_cgroup_register_device(struct drm_device *dev,
> + struct dev_cgroup_device *cgdev)
> +{
> + int ret;
> + char dev_name[32];
> +
> + snprintf(dev_name, sizeof(dev_name), "drm/%s", dev->unique);
> + ret = dev_cgroup_register_device(cgdev, dev_name);
> + if (ret)
> + return ret;
> +
> + return drmm_add_action_or_reset(dev, drmm_cg_unregister_device, cgdev);
> +}
> +EXPORT_SYMBOL_GPL(drmm_cgroup_register_device);
> +
> static int create_compat_control_link(struct drm_device *dev)
> {
> struct drm_minor *minor;
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 1bbbcb8e2d231..3e83c7ce7f2d0 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -34,6 +34,7 @@
>
> #include <drm/drm_device.h>
>
> +struct dev_cgroup_device;
> struct drm_fb_helper;
> struct drm_fb_helper_surface_size;
> struct drm_file;
> @@ -438,6 +439,9 @@ void *__devm_drm_dev_alloc(struct device *parent,
> const struct drm_driver *driver,
> size_t size, size_t offset);
>
> +int drmm_cgroup_register_device(struct drm_device *dev,
> + struct dev_cgroup_device *cgdev);
> +
> /**
> * devm_drm_dev_alloc - Resource managed allocation of a &drm_device instance
> * @parent: Parent device object
--
Jani Nikula, Intel
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 2/7] drm/drv: Add drmm cgroup registration for dev cgroups.
2024-10-23 7:52 ` [PATCH 2/7] drm/drv: Add drmm cgroup registration for dev cgroups Maarten Lankhorst
2024-10-23 8:45 ` Jani Nikula
@ 2024-10-24 10:35 ` kernel test robot
2024-10-24 15:42 ` kernel test robot
2 siblings, 0 replies; 27+ messages in thread
From: kernel test robot @ 2024-10-24 10:35 UTC (permalink / raw)
To: Maarten Lankhorst, intel-xe, linux-kernel, dri-devel, Tejun Heo,
Zefan Li, Johannes Weiner, Andrew Morton
Cc: oe-kbuild-all, Linux Memory Management List, Friedrich Vock,
cgroups, Maxime Ripard, Maarten Lankhorst
Hi Maarten,
kernel test robot noticed the following build errors:
[auto build test ERROR on drm/drm-next]
[also build test ERROR on drm-exynos/exynos-drm-next next-20241024]
[cannot apply to tj-cgroup/for-next drm-xe/drm-xe-next akpm-mm/mm-everything drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-misc/drm-misc-next drm-tip/drm-tip linus/master v6.12-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Maarten-Lankhorst/kernel-cgroup-Add-dev-memory-accounting-cgroup/20241023-155504
base: git://anongit.freedesktop.org/drm/drm drm-next
patch link: https://lore.kernel.org/r/20241023075302.27194-3-maarten.lankhorst%40linux.intel.com
patch subject: [PATCH 2/7] drm/drv: Add drmm cgroup registration for dev cgroups.
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20241024/202410241806.p6u3FcGS-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241024/202410241806.p6u3FcGS-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410241806.p6u3FcGS-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
In file included from drivers/gpu/drm/drm_drv.c:29:
>> include/linux/cgroup_dev.h:61:12: error: two or more data types in declaration specifiers
61 | static int int dev_cgroup_try_charge(struct dev_cgroup_device *cgdev,
| ^~~
>> include/linux/cgroup_dev.h:65:1: error: expected identifier or '(' before '{' token
65 | {
| ^
>> include/linux/cgroup_dev.h:61:16: warning: 'dev_cgroup_try_charge' declared 'static' but never defined [-Wunused-function]
61 | static int int dev_cgroup_try_charge(struct dev_cgroup_device *cgdev,
| ^~~~~~~~~~~~~~~~~~~~~
vim +61 include/linux/cgroup_dev.h
487073b1855ef4 Maarten Lankhorst 2024-10-23 60
487073b1855ef4 Maarten Lankhorst 2024-10-23 @61 static int int dev_cgroup_try_charge(struct dev_cgroup_device *cgdev,
487073b1855ef4 Maarten Lankhorst 2024-10-23 62 u32 index, u64 size,
487073b1855ef4 Maarten Lankhorst 2024-10-23 63 struct dev_cgroup_pool_state **ret_pool,
487073b1855ef4 Maarten Lankhorst 2024-10-23 64 struct dev_cgroup_pool_state **ret_limit_pool);
487073b1855ef4 Maarten Lankhorst 2024-10-23 @65 {
487073b1855ef4 Maarten Lankhorst 2024-10-23 66 *ret_pool = NULL;
487073b1855ef4 Maarten Lankhorst 2024-10-23 67
487073b1855ef4 Maarten Lankhorst 2024-10-23 68 if (ret_limit_pool)
487073b1855ef4 Maarten Lankhorst 2024-10-23 69 *ret_limit_pool = NULL;
487073b1855ef4 Maarten Lankhorst 2024-10-23 70
487073b1855ef4 Maarten Lankhorst 2024-10-23 71 return 0;
487073b1855ef4 Maarten Lankhorst 2024-10-23 72 }
487073b1855ef4 Maarten Lankhorst 2024-10-23 73
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 2/7] drm/drv: Add drmm cgroup registration for dev cgroups.
2024-10-23 7:52 ` [PATCH 2/7] drm/drv: Add drmm cgroup registration for dev cgroups Maarten Lankhorst
2024-10-23 8:45 ` Jani Nikula
2024-10-24 10:35 ` kernel test robot
@ 2024-10-24 15:42 ` kernel test robot
2 siblings, 0 replies; 27+ messages in thread
From: kernel test robot @ 2024-10-24 15:42 UTC (permalink / raw)
To: Maarten Lankhorst, intel-xe, linux-kernel, dri-devel, Tejun Heo,
Zefan Li, Johannes Weiner, Andrew Morton
Cc: oe-kbuild-all, Linux Memory Management List, Friedrich Vock,
cgroups, Maxime Ripard, Maarten Lankhorst
Hi Maarten,
kernel test robot noticed the following build errors:
[auto build test ERROR on drm/drm-next]
[also build test ERROR on drm-exynos/exynos-drm-next next-20241024]
[cannot apply to tj-cgroup/for-next drm-xe/drm-xe-next akpm-mm/mm-everything drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-misc/drm-misc-next drm-tip/drm-tip linus/master v6.12-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Maarten-Lankhorst/kernel-cgroup-Add-dev-memory-accounting-cgroup/20241023-155504
base: git://anongit.freedesktop.org/drm/drm drm-next
patch link: https://lore.kernel.org/r/20241023075302.27194-3-maarten.lankhorst%40linux.intel.com
patch subject: [PATCH 2/7] drm/drv: Add drmm cgroup registration for dev cgroups.
config: arm64-randconfig-002-20241024 (https://download.01.org/0day-ci/archive/20241024/202410242324.adtg6g0w-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241024/202410242324.adtg6g0w-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410242324.adtg6g0w-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from drivers/gpu/drm/drm_drv.c:29:
include/linux/cgroup_dev.h:61:12: error: two or more data types in declaration specifiers
61 | static int int dev_cgroup_try_charge(struct dev_cgroup_device *cgdev,
| ^~~
include/linux/cgroup_dev.h:65:1: error: expected identifier or '(' before '{' token
65 | {
| ^
>> include/linux/cgroup_dev.h:61:16: error: 'dev_cgroup_try_charge' declared 'static' but never defined [-Werror=unused-function]
61 | static int int dev_cgroup_try_charge(struct dev_cgroup_device *cgdev,
| ^~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
vim +61 include/linux/cgroup_dev.h
487073b1855ef4 Maarten Lankhorst 2024-10-23 60
487073b1855ef4 Maarten Lankhorst 2024-10-23 @61 static int int dev_cgroup_try_charge(struct dev_cgroup_device *cgdev,
487073b1855ef4 Maarten Lankhorst 2024-10-23 62 u32 index, u64 size,
487073b1855ef4 Maarten Lankhorst 2024-10-23 63 struct dev_cgroup_pool_state **ret_pool,
487073b1855ef4 Maarten Lankhorst 2024-10-23 64 struct dev_cgroup_pool_state **ret_limit_pool);
487073b1855ef4 Maarten Lankhorst 2024-10-23 65 {
487073b1855ef4 Maarten Lankhorst 2024-10-23 66 *ret_pool = NULL;
487073b1855ef4 Maarten Lankhorst 2024-10-23 67
487073b1855ef4 Maarten Lankhorst 2024-10-23 68 if (ret_limit_pool)
487073b1855ef4 Maarten Lankhorst 2024-10-23 69 *ret_limit_pool = NULL;
487073b1855ef4 Maarten Lankhorst 2024-10-23 70
487073b1855ef4 Maarten Lankhorst 2024-10-23 71 return 0;
487073b1855ef4 Maarten Lankhorst 2024-10-23 72 }
487073b1855ef4 Maarten Lankhorst 2024-10-23 73
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 3/7] drm/ttm: Handle cgroup based eviction in TTM
2024-10-23 7:52 [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup Maarten Lankhorst
2024-10-23 7:52 ` [PATCH 1/7] kernel/cgroup: " Maarten Lankhorst
2024-10-23 7:52 ` [PATCH 2/7] drm/drv: Add drmm cgroup registration for dev cgroups Maarten Lankhorst
@ 2024-10-23 7:52 ` Maarten Lankhorst
2024-10-23 7:52 ` [PATCH 4/7] drm/xe: Implement cgroup for vram Maarten Lankhorst
` (4 subsequent siblings)
7 siblings, 0 replies; 27+ messages in thread
From: Maarten Lankhorst @ 2024-10-23 7:52 UTC (permalink / raw)
To: intel-xe, linux-kernel, dri-devel, Tejun Heo, Zefan Li,
Johannes Weiner, Andrew Morton
Cc: Friedrich Vock, cgroups, linux-mm, Maxime Ripard, Maarten Lankhorst
cgroup resource allocation has to be handled in TTM, so -EAGAIN from
cgroups can be converted into -ENOSPC, and the limitcg can be properly
evicted in ttm code.
When hitting a resource limit through -EAGAIN, the cgroup for which the
limit is hit is also returned. This allows eviction to delete only from
cgroups which are a subgroup of the current cgroup.
The returned CSS is used to determine if eviction is valuable for a
given resource, and allows TTM to only target specific resources to
lower memory usage.
Co-developed-by: Friedrich Vock <friedrich.vock@gmx.de>
Signed-off-by: Friedrich Vock <friedrich.vock@gmx.de>
Co-developed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/ttm/tests/ttm_bo_test.c | 18 +++---
.../gpu/drm/ttm/tests/ttm_bo_validate_test.c | 4 +-
drivers/gpu/drm/ttm/tests/ttm_resource_test.c | 2 +-
drivers/gpu/drm/ttm/ttm_bo.c | 57 ++++++++++++++++---
drivers/gpu/drm/ttm/ttm_resource.c | 24 +++++++-
include/drm/ttm/ttm_resource.h | 16 +++++-
6 files changed, 98 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/ttm/tests/ttm_bo_test.c b/drivers/gpu/drm/ttm/tests/ttm_bo_test.c
index 3139fd9128d84..f8f20d2f61740 100644
--- a/drivers/gpu/drm/ttm/tests/ttm_bo_test.c
+++ b/drivers/gpu/drm/ttm/tests/ttm_bo_test.c
@@ -258,13 +258,13 @@ static void ttm_bo_unreserve_basic(struct kunit *test)
bo = ttm_bo_kunit_init(test, test->priv, BO_SIZE, NULL);
bo->priority = bo_prio;
- err = ttm_resource_alloc(bo, place, &res1);
+ err = ttm_resource_alloc(bo, place, &res1, NULL);
KUNIT_ASSERT_EQ(test, err, 0);
bo->resource = res1;
/* Add a dummy resource to populate LRU */
- ttm_resource_alloc(bo, place, &res2);
+ ttm_resource_alloc(bo, place, &res2, NULL);
dma_resv_lock(bo->base.resv, NULL);
ttm_bo_unreserve(bo);
@@ -300,12 +300,12 @@ static void ttm_bo_unreserve_pinned(struct kunit *test)
dma_resv_lock(bo->base.resv, NULL);
ttm_bo_pin(bo);
- err = ttm_resource_alloc(bo, place, &res1);
+ err = ttm_resource_alloc(bo, place, &res1, NULL);
KUNIT_ASSERT_EQ(test, err, 0);
bo->resource = res1;
/* Add a dummy resource to the pinned list */
- err = ttm_resource_alloc(bo, place, &res2);
+ err = ttm_resource_alloc(bo, place, &res2, NULL);
KUNIT_ASSERT_EQ(test, err, 0);
KUNIT_ASSERT_EQ(test,
list_is_last(&res2->lru.link, &priv->ttm_dev->unevictable), 1);
@@ -355,7 +355,7 @@ static void ttm_bo_unreserve_bulk(struct kunit *test)
ttm_bo_set_bulk_move(bo1, &lru_bulk_move);
dma_resv_unlock(bo1->base.resv);
- err = ttm_resource_alloc(bo1, place, &res1);
+ err = ttm_resource_alloc(bo1, place, &res1, NULL);
KUNIT_ASSERT_EQ(test, err, 0);
bo1->resource = res1;
@@ -363,7 +363,7 @@ static void ttm_bo_unreserve_bulk(struct kunit *test)
ttm_bo_set_bulk_move(bo2, &lru_bulk_move);
dma_resv_unlock(bo2->base.resv);
- err = ttm_resource_alloc(bo2, place, &res2);
+ err = ttm_resource_alloc(bo2, place, &res2, NULL);
KUNIT_ASSERT_EQ(test, err, 0);
bo2->resource = res2;
@@ -401,7 +401,7 @@ static void ttm_bo_put_basic(struct kunit *test)
bo = ttm_bo_kunit_init(test, test->priv, BO_SIZE, NULL);
bo->type = ttm_bo_type_device;
- err = ttm_resource_alloc(bo, place, &res);
+ err = ttm_resource_alloc(bo, place, &res, NULL);
KUNIT_ASSERT_EQ(test, err, 0);
bo->resource = res;
@@ -518,7 +518,7 @@ static void ttm_bo_pin_unpin_resource(struct kunit *test)
bo = ttm_bo_kunit_init(test, test->priv, BO_SIZE, NULL);
- err = ttm_resource_alloc(bo, place, &res);
+ err = ttm_resource_alloc(bo, place, &res, NULL);
KUNIT_ASSERT_EQ(test, err, 0);
bo->resource = res;
@@ -569,7 +569,7 @@ static void ttm_bo_multiple_pin_one_unpin(struct kunit *test)
bo = ttm_bo_kunit_init(test, test->priv, BO_SIZE, NULL);
- err = ttm_resource_alloc(bo, place, &res);
+ err = ttm_resource_alloc(bo, place, &res, NULL);
KUNIT_ASSERT_EQ(test, err, 0);
bo->resource = res;
diff --git a/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c b/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
index 1adf18481ea05..3148f5d3dbd66 100644
--- a/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
+++ b/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
@@ -542,7 +542,7 @@ static void ttm_bo_validate_no_placement_signaled(struct kunit *test)
bo->ttm = old_tt;
}
- err = ttm_resource_alloc(bo, place, &bo->resource);
+ err = ttm_resource_alloc(bo, place, &bo->resource, NULL);
KUNIT_EXPECT_EQ(test, err, 0);
KUNIT_ASSERT_EQ(test, man->usage, size);
@@ -603,7 +603,7 @@ static void ttm_bo_validate_no_placement_not_signaled(struct kunit *test)
bo = ttm_bo_kunit_init(test, test->priv, size, NULL);
bo->type = params->bo_type;
- err = ttm_resource_alloc(bo, place, &bo->resource);
+ err = ttm_resource_alloc(bo, place, &bo->resource, NULL);
KUNIT_EXPECT_EQ(test, err, 0);
placement = kunit_kzalloc(test, sizeof(*placement), GFP_KERNEL);
diff --git a/drivers/gpu/drm/ttm/tests/ttm_resource_test.c b/drivers/gpu/drm/ttm/tests/ttm_resource_test.c
index a9f4b81921c3c..e6ea2bd01f07a 100644
--- a/drivers/gpu/drm/ttm/tests/ttm_resource_test.c
+++ b/drivers/gpu/drm/ttm/tests/ttm_resource_test.c
@@ -302,7 +302,7 @@ static void ttm_sys_man_free_basic(struct kunit *test)
res = kunit_kzalloc(test, sizeof(*res), GFP_KERNEL);
KUNIT_ASSERT_NOT_NULL(test, res);
- ttm_resource_alloc(bo, place, &res);
+ ttm_resource_alloc(bo, place, &res, NULL);
man = ttm_manager_type(priv->devs->ttm_dev, mem_type);
man->func->free(man, res);
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 48c5365efca1c..5bdebfdc95e92 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -42,6 +42,7 @@
#include <linux/file.h>
#include <linux/module.h>
#include <linux/atomic.h>
+#include <linux/cgroup_dev.h>
#include <linux/dma-resv.h>
#include "ttm_module.h"
@@ -447,7 +448,7 @@ int ttm_bo_evict_first(struct ttm_device *bdev, struct ttm_resource_manager *man
struct ttm_buffer_object *bo;
struct ttm_resource *res;
unsigned int mem_type;
- int ret = 0;
+ int ret;
spin_lock(&bdev->lru_lock);
res = ttm_resource_manager_first(man, &cursor);
@@ -499,14 +500,28 @@ struct ttm_bo_evict_walk {
struct ttm_resource **res;
/** @evicted: Number of successful evictions. */
unsigned long evicted;
+
+ /** @limit_pool: Which pool limit we should test against */
+ struct dev_cgroup_pool_state *limit_pool;
+ /** @try_low: Whether we should attempt to evict BO's with low watermark threshold */
+ bool try_low;
+ /** @hit_low: If we cannot evict a bo when @try_low is false (first pass) */
+ bool hit_low;
};
static s64 ttm_bo_evict_cb(struct ttm_lru_walk *walk, struct ttm_buffer_object *bo)
{
struct ttm_bo_evict_walk *evict_walk =
container_of(walk, typeof(*evict_walk), walk);
+ struct ttm_resource_manager *man =
+ ttm_manager_type(bo->bdev, bo->resource->mem_type);
s64 lret;
+ if (!dev_cgroup_state_evict_valuable(man->cgdev, man->cgidx,
+ evict_walk->limit_pool, bo->resource->css,
+ evict_walk->try_low, &evict_walk->hit_low))
+ return 0;
+
if (bo->pin_count || !bo->bdev->funcs->eviction_valuable(bo, evict_walk->place))
return 0;
@@ -524,7 +539,7 @@ static s64 ttm_bo_evict_cb(struct ttm_lru_walk *walk, struct ttm_buffer_object *
evict_walk->evicted++;
if (evict_walk->res)
lret = ttm_resource_alloc(evict_walk->evictor, evict_walk->place,
- evict_walk->res);
+ evict_walk->res, NULL);
if (lret == 0)
return 1;
out:
@@ -545,7 +560,8 @@ static int ttm_bo_evict_alloc(struct ttm_device *bdev,
struct ttm_buffer_object *evictor,
struct ttm_operation_ctx *ctx,
struct ww_acquire_ctx *ticket,
- struct ttm_resource **res)
+ struct ttm_resource **res,
+ struct dev_cgroup_pool_state *limit_pool)
{
struct ttm_bo_evict_walk evict_walk = {
.walk = {
@@ -556,22 +572,39 @@ static int ttm_bo_evict_alloc(struct ttm_device *bdev,
.place = place,
.evictor = evictor,
.res = res,
+ .limit_pool = limit_pool,
};
s64 lret;
evict_walk.walk.trylock_only = true;
lret = ttm_lru_walk_for_evict(&evict_walk.walk, bdev, man, 1);
+
+ /* One more attempt if we hit low limit? */
+ if (!lret && evict_walk.hit_low) {
+ evict_walk.try_low = true;
+ lret = ttm_lru_walk_for_evict(&evict_walk.walk, bdev, man, 1);
+ }
if (lret || !ticket)
goto out;
+ /* Reset low limit */
+ evict_walk.try_low = evict_walk.hit_low = false;
/* If ticket-locking, repeat while making progress. */
evict_walk.walk.trylock_only = false;
+
+retry:
do {
/* The walk may clear the evict_walk.walk.ticket field */
evict_walk.walk.ticket = ticket;
evict_walk.evicted = 0;
lret = ttm_lru_walk_for_evict(&evict_walk.walk, bdev, man, 1);
} while (!lret && evict_walk.evicted);
+
+ /* We hit the low limit? Try once more */
+ if (!lret && evict_walk.hit_low && !evict_walk.try_low) {
+ evict_walk.try_low = true;
+ goto retry;
+ }
out:
if (lret < 0)
return lret;
@@ -689,6 +722,7 @@ static int ttm_bo_alloc_resource(struct ttm_buffer_object *bo,
for (i = 0; i < placement->num_placement; ++i) {
const struct ttm_place *place = &placement->placement[i];
+ struct dev_cgroup_pool_state *limit_pool = NULL;
struct ttm_resource_manager *man;
bool may_evict;
@@ -701,15 +735,20 @@ static int ttm_bo_alloc_resource(struct ttm_buffer_object *bo,
continue;
may_evict = (force_space && place->mem_type != TTM_PL_SYSTEM);
- ret = ttm_resource_alloc(bo, place, res);
+ ret = ttm_resource_alloc(bo, place, res, force_space ? &limit_pool : NULL);
if (ret) {
- if (ret != -ENOSPC)
+ if (ret != -ENOSPC && ret != -EAGAIN) {
+ dev_cgroup_pool_state_put(limit_pool);
return ret;
- if (!may_evict)
+ }
+ if (!may_evict) {
+ dev_cgroup_pool_state_put(limit_pool);
continue;
+ }
ret = ttm_bo_evict_alloc(bdev, man, place, bo, ctx,
- ticket, res);
+ ticket, res, limit_pool);
+ dev_cgroup_pool_state_put(limit_pool);
if (ret == -EBUSY)
continue;
if (ret)
@@ -1056,6 +1095,8 @@ struct ttm_bo_swapout_walk {
struct ttm_lru_walk walk;
/** @gfp_flags: The gfp flags to use for ttm_tt_swapout() */
gfp_t gfp_flags;
+
+ bool hit_low, evict_low;
};
static s64
@@ -1106,7 +1147,7 @@ ttm_bo_swapout_cb(struct ttm_lru_walk *walk, struct ttm_buffer_object *bo)
memset(&hop, 0, sizeof(hop));
place.mem_type = TTM_PL_SYSTEM;
- ret = ttm_resource_alloc(bo, &place, &evict_mem);
+ ret = ttm_resource_alloc(bo, &place, &evict_mem, NULL);
if (ret)
goto out;
diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
index a87665eb28a62..93190f7330b5c 100644
--- a/drivers/gpu/drm/ttm/ttm_resource.c
+++ b/drivers/gpu/drm/ttm/ttm_resource.c
@@ -26,6 +26,7 @@
#include <linux/io-mapping.h>
#include <linux/iosys-map.h>
#include <linux/scatterlist.h>
+#include <linux/cgroup_dev.h>
#include <drm/ttm/ttm_bo.h>
#include <drm/ttm/ttm_placement.h>
@@ -350,15 +351,29 @@ EXPORT_SYMBOL(ttm_resource_fini);
int ttm_resource_alloc(struct ttm_buffer_object *bo,
const struct ttm_place *place,
- struct ttm_resource **res_ptr)
+ struct ttm_resource **res_ptr,
+ struct dev_cgroup_pool_state **ret_limit_pool)
{
struct ttm_resource_manager *man =
ttm_manager_type(bo->bdev, place->mem_type);
+ struct dev_cgroup_pool_state *pool = NULL;
int ret;
+ if (man->cgdev) {
+ ret = dev_cgroup_try_charge(man->cgdev, man->cgidx,
+ bo->base.size, &pool, ret_limit_pool);
+ if (ret)
+ return ret;
+ }
+
ret = man->func->alloc(man, bo, place, res_ptr);
- if (ret)
+ if (ret) {
+ if (pool)
+ dev_cgroup_uncharge(pool, man->cgidx, bo->base.size);
return ret;
+ }
+
+ (*res_ptr)->css = pool;
spin_lock(&bo->bdev->lru_lock);
ttm_resource_add_bulk_move(*res_ptr, bo);
@@ -370,6 +385,7 @@ EXPORT_SYMBOL_FOR_TESTS_ONLY(ttm_resource_alloc);
void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource **res)
{
struct ttm_resource_manager *man;
+ struct dev_cgroup_pool_state *pool;
if (!*res)
return;
@@ -377,9 +393,13 @@ void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource **res)
spin_lock(&bo->bdev->lru_lock);
ttm_resource_del_bulk_move(*res, bo);
spin_unlock(&bo->bdev->lru_lock);
+
+ pool = (*res)->css;
man = ttm_manager_type(bo->bdev, (*res)->mem_type);
man->func->free(man, *res);
*res = NULL;
+ if (man->cgdev)
+ dev_cgroup_uncharge(pool, man->cgidx, bo->base.size);
}
EXPORT_SYMBOL(ttm_resource_free);
diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h
index be034be56ba1b..12ff4b66c6f7d 100644
--- a/include/drm/ttm/ttm_resource.h
+++ b/include/drm/ttm/ttm_resource.h
@@ -38,6 +38,7 @@
#define TTM_MAX_BO_PRIORITY 4U
#define TTM_NUM_MEM_TYPES 8
+struct dev_cgroup_device;
struct ttm_device;
struct ttm_resource_manager;
struct ttm_resource;
@@ -211,6 +212,15 @@ struct ttm_resource_manager {
* bdev->lru_lock.
*/
uint64_t usage;
+
+ /**
+ * @cgdev: dev_cgroup_device used for memory accounting, if not NULL.
+ */
+ struct dev_cgroup_device *cgdev;
+ /**
+ * @cgidx: Resource index used by this resource manager for cgroup accounting
+ */
+ u32 cgidx;
};
/**
@@ -239,6 +249,7 @@ struct ttm_bus_placement {
* @placement: Placement flags.
* @bus: Placement on io bus accessible to the CPU
* @bo: weak reference to the BO, protected by ttm_device::lru_lock
+ * @css: cgroup state this resource is charged to
*
* Structure indicating the placement and space resources used by a
* buffer object.
@@ -251,6 +262,8 @@ struct ttm_resource {
struct ttm_bus_placement bus;
struct ttm_buffer_object *bo;
+ struct dev_cgroup_pool_state *css;
+
/**
* @lru: Least recently used list, see &ttm_resource_manager.lru
*/
@@ -432,7 +445,8 @@ void ttm_resource_fini(struct ttm_resource_manager *man,
int ttm_resource_alloc(struct ttm_buffer_object *bo,
const struct ttm_place *place,
- struct ttm_resource **res);
+ struct ttm_resource **res,
+ struct dev_cgroup_pool_state **ret_limit_pool);
void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource **res);
bool ttm_resource_intersects(struct ttm_device *bdev,
struct ttm_resource *res,
--
2.45.2
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH 4/7] drm/xe: Implement cgroup for vram
2024-10-23 7:52 [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup Maarten Lankhorst
` (2 preceding siblings ...)
2024-10-23 7:52 ` [PATCH 3/7] drm/ttm: Handle cgroup based eviction in TTM Maarten Lankhorst
@ 2024-10-23 7:52 ` Maarten Lankhorst
2024-10-23 7:52 ` [PATCH 5/7] drm/amdgpu: Add cgroups implementation Maarten Lankhorst
` (3 subsequent siblings)
7 siblings, 0 replies; 27+ messages in thread
From: Maarten Lankhorst @ 2024-10-23 7:52 UTC (permalink / raw)
To: intel-xe, linux-kernel, dri-devel, Tejun Heo, Zefan Li,
Johannes Weiner, Andrew Morton
Cc: Friedrich Vock, cgroups, linux-mm, Maxime Ripard, Maarten Lankhorst
Add vram based cgroup eviction to Xe.
Most hardware with VRAM uses TTM for its management, and can be
similarly trivially enabled.
Co-developed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/xe/xe_device.c | 4 ++++
drivers/gpu/drm/xe/xe_device_types.h | 4 ++++
drivers/gpu/drm/xe/xe_ttm_vram_mgr.c | 10 ++++++++++
3 files changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index 51bb9d875268f..cb4b2013eb808 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -726,6 +726,10 @@ int xe_device_probe(struct xe_device *xe)
/* Allocate and map stolen after potential VRAM resize */
xe_ttm_stolen_mgr_init(xe);
+ err = drmm_cgroup_register_device(&xe->drm, &xe->cg);
+ if (err)
+ goto err;
+
/*
* Now that GT is initialized (TTM in particular),
* we can try to init display, and inherit the initial fb.
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index 85bede4dd6461..0401b4fe6f645 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -7,6 +7,7 @@
#define _XE_DEVICE_TYPES_H_
#include <linux/pci.h>
+#include <linux/cgroup_dev.h>
#include <drm/drm_device.h>
#include <drm/drm_file.h>
@@ -257,6 +258,9 @@ struct xe_device {
/** @devcoredump: device coredump */
struct xe_devcoredump devcoredump;
+ /** @cg: device cgroup bookkeeping */
+ struct dev_cgroup_device cg;
+
/** @info: device info */
struct intel_device_info {
/** @info.platform_name: platform name */
diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
index 423b261ea7430..cb463be53f4bc 100644
--- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
+++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
@@ -339,6 +339,16 @@ int __xe_ttm_vram_mgr_init(struct xe_device *xe, struct xe_ttm_vram_mgr *mgr,
struct ttm_resource_manager *man = &mgr->manager;
int err;
+ if (mem_type != XE_PL_STOLEN) {
+ int cgregion = xe->cg.num_regions++;
+
+ xe->cg.regions[cgregion].size = size;
+ xe->cg.regions[cgregion].name =
+ mem_type == XE_PL_VRAM0 ? "vram0" : "vram1";
+ man->cgdev = &xe->cg;
+ man->cgidx = cgregion;
+ }
+
man->func = &xe_ttm_vram_mgr_func;
mgr->mem_type = mem_type;
mutex_init(&mgr->lock);
--
2.45.2
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH 5/7] drm/amdgpu: Add cgroups implementation
2024-10-23 7:52 [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup Maarten Lankhorst
` (3 preceding siblings ...)
2024-10-23 7:52 ` [PATCH 4/7] drm/xe: Implement cgroup for vram Maarten Lankhorst
@ 2024-10-23 7:52 ` Maarten Lankhorst
2024-10-23 7:52 ` [PATCH 6/7] [HACK] drm/xe: Hack to test with mapped pages instead of vram Maarten Lankhorst
` (2 subsequent siblings)
7 siblings, 0 replies; 27+ messages in thread
From: Maarten Lankhorst @ 2024-10-23 7:52 UTC (permalink / raw)
To: intel-xe, linux-kernel, dri-devel, Tejun Heo, Zefan Li,
Johannes Weiner, Andrew Morton
Cc: Friedrich Vock, cgroups, linux-mm, Maxime Ripard, Maarten Lankhorst
Similar to xe, enable some simple management of VRAM only.
Co-developed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 6 ++++++
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 6 ++++++
3 files changed, 14 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 9b1e0ede05a45..27c11e43f8e9e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -43,6 +43,7 @@
#include "amdgpu_ctx.h"
#include <linux/atomic.h>
+#include <linux/cgroup_dev.h>
#include <linux/wait.h>
#include <linux/list.h>
#include <linux/kref.h>
@@ -835,6 +836,7 @@ struct amdgpu_device {
struct device *dev;
struct pci_dev *pdev;
struct drm_device ddev;
+ struct dev_cgroup_device cg;
#ifdef CONFIG_DRM_AMD_ACP
struct amdgpu_acp acp;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 74adb983ab03e..3f6554c7aac2f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1874,6 +1874,12 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
return r;
}
+ r = drmm_cgroup_register_device(adev_to_drm(adev), &adev->cg);
+ if (r) {
+ DRM_ERROR("Failed initializing cgroup allocator.\n");
+ return r;
+ }
+
/* Change the size here instead of the init above so only lpfn is affected */
amdgpu_ttm_set_buffer_funcs_status(adev, false);
#ifdef CONFIG_64BIT
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index 7d26a962f811c..44d560bef5b7d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -927,6 +927,12 @@ int amdgpu_vram_mgr_init(struct amdgpu_device *adev)
DRM_INFO("Setup dummy vram mgr\n");
}
+ adev->cg.regions[0].size = adev->gmc.real_vram_size;
+ adev->cg.regions[0].name = "vram";
+ adev->cg.num_regions++;
+ man->cgdev = &adev->cg;
+ man->cgidx = 0;
+
ttm_set_driver_manager(&adev->mman.bdev, TTM_PL_VRAM, &mgr->manager);
ttm_resource_manager_set_used(man, true);
return 0;
--
2.45.2
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH 6/7] [HACK] drm/xe: Hack to test with mapped pages instead of vram.
2024-10-23 7:52 [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup Maarten Lankhorst
` (4 preceding siblings ...)
2024-10-23 7:52 ` [PATCH 5/7] drm/amdgpu: Add cgroups implementation Maarten Lankhorst
@ 2024-10-23 7:52 ` Maarten Lankhorst
2024-10-23 8:52 ` Jani Nikula
2024-10-23 7:53 ` [PATCH 7/7] [DISCUSSION] drm/gem: Add cgroup memory accounting Maarten Lankhorst
2024-10-23 19:40 ` [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup Tejun Heo
7 siblings, 1 reply; 27+ messages in thread
From: Maarten Lankhorst @ 2024-10-23 7:52 UTC (permalink / raw)
To: intel-xe, linux-kernel, dri-devel, Tejun Heo, Zefan Li,
Johannes Weiner, Andrew Morton
Cc: Friedrich Vock, cgroups, linux-mm, Maxime Ripard, Maarten Lankhorst
We will probably want to make this a proper region in TTM for
everything, so that we can charge VRAM twice, once for mapped
in sysmem, once for mapped in vram. That way we don't need to
deal with evict failing from lack of available memory in mapped.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
drivers/gpu/drm/xe/xe_ttm_sys_mgr.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/gpu/drm/xe/xe_ttm_sys_mgr.c b/drivers/gpu/drm/xe/xe_ttm_sys_mgr.c
index 9844a8edbfe19..20fa8ec8925ef 100644
--- a/drivers/gpu/drm/xe/xe_ttm_sys_mgr.c
+++ b/drivers/gpu/drm/xe/xe_ttm_sys_mgr.c
@@ -101,6 +101,18 @@ static void ttm_sys_mgr_fini(struct drm_device *drm, void *arg)
ttm_set_driver_manager(&xe->ttm, XE_PL_TT, NULL);
}
+static inline void apply_cg(struct xe_device *xe,
+ struct ttm_resource_manager *man,
+ u64 gtt_size)
+{
+ int cgregion = xe->cg.num_regions++;
+
+ xe->cg.regions[cgregion].size = gtt_size;
+ xe->cg.regions[cgregion].name = "mapped";
+ man->cgdev = &xe->cg;
+ man->cgidx = cgregion;
+
+}
int xe_ttm_sys_mgr_init(struct xe_device *xe)
{
struct ttm_resource_manager *man = &xe->mem.sys_mgr;
@@ -116,6 +128,8 @@ int xe_ttm_sys_mgr_init(struct xe_device *xe)
man->func = &xe_ttm_sys_mgr_func;
ttm_resource_manager_init(man, &xe->ttm, gtt_size >> PAGE_SHIFT);
ttm_set_driver_manager(&xe->ttm, XE_PL_TT, man);
+ apply_cg(xe, man, gtt_size);
+
ttm_resource_manager_set_used(man, true);
return drmm_add_action_or_reset(&xe->drm, ttm_sys_mgr_fini, xe);
}
--
2.45.2
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 6/7] [HACK] drm/xe: Hack to test with mapped pages instead of vram.
2024-10-23 7:52 ` [PATCH 6/7] [HACK] drm/xe: Hack to test with mapped pages instead of vram Maarten Lankhorst
@ 2024-10-23 8:52 ` Jani Nikula
0 siblings, 0 replies; 27+ messages in thread
From: Jani Nikula @ 2024-10-23 8:52 UTC (permalink / raw)
To: Maarten Lankhorst, intel-xe, linux-kernel, dri-devel, Tejun Heo,
Zefan Li, Johannes Weiner, Andrew Morton
Cc: Friedrich Vock, cgroups, linux-mm, Maxime Ripard, Maarten Lankhorst
On Wed, 23 Oct 2024, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote:
> We will probably want to make this a proper region in TTM for
> everything, so that we can charge VRAM twice, once for mapped
> in sysmem, once for mapped in vram. That way we don't need to
> deal with evict failing from lack of available memory in mapped.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
> drivers/gpu/drm/xe/xe_ttm_sys_mgr.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/xe_ttm_sys_mgr.c b/drivers/gpu/drm/xe/xe_ttm_sys_mgr.c
> index 9844a8edbfe19..20fa8ec8925ef 100644
> --- a/drivers/gpu/drm/xe/xe_ttm_sys_mgr.c
> +++ b/drivers/gpu/drm/xe/xe_ttm_sys_mgr.c
> @@ -101,6 +101,18 @@ static void ttm_sys_mgr_fini(struct drm_device *drm, void *arg)
> ttm_set_driver_manager(&xe->ttm, XE_PL_TT, NULL);
> }
>
> +static inline void apply_cg(struct xe_device *xe,
> + struct ttm_resource_manager *man,
> + u64 gtt_size)
Ditto here about static inline in .c.
> +{
> + int cgregion = xe->cg.num_regions++;
> +
> + xe->cg.regions[cgregion].size = gtt_size;
> + xe->cg.regions[cgregion].name = "mapped";
> + man->cgdev = &xe->cg;
> + man->cgidx = cgregion;
> +
> +}
> int xe_ttm_sys_mgr_init(struct xe_device *xe)
> {
> struct ttm_resource_manager *man = &xe->mem.sys_mgr;
> @@ -116,6 +128,8 @@ int xe_ttm_sys_mgr_init(struct xe_device *xe)
> man->func = &xe_ttm_sys_mgr_func;
> ttm_resource_manager_init(man, &xe->ttm, gtt_size >> PAGE_SHIFT);
> ttm_set_driver_manager(&xe->ttm, XE_PL_TT, man);
> + apply_cg(xe, man, gtt_size);
> +
> ttm_resource_manager_set_used(man, true);
> return drmm_add_action_or_reset(&xe->drm, ttm_sys_mgr_fini, xe);
> }
--
Jani Nikula, Intel
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 7/7] [DISCUSSION] drm/gem: Add cgroup memory accounting
2024-10-23 7:52 [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup Maarten Lankhorst
` (5 preceding siblings ...)
2024-10-23 7:52 ` [PATCH 6/7] [HACK] drm/xe: Hack to test with mapped pages instead of vram Maarten Lankhorst
@ 2024-10-23 7:53 ` Maarten Lankhorst
2024-10-23 19:40 ` [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup Tejun Heo
7 siblings, 0 replies; 27+ messages in thread
From: Maarten Lankhorst @ 2024-10-23 7:53 UTC (permalink / raw)
To: intel-xe, linux-kernel, dri-devel, Tejun Heo, Zefan Li,
Johannes Weiner, Andrew Morton
Cc: Friedrich Vock, cgroups, linux-mm, Maxime Ripard, Maarten Lankhorst
From: Maxime Ripard <mripard@kernel.org>
In order to support any device using the GEM support, let's register a
dev cgroup device in the drm_dev_register path, and account for
allocated buffers in the buffer allocation path.
Marked discussion by Maarten Lankhorst:
This is only implemented for drm_gem_dma_helper.c, and breaks the other
drivers. It's still here for discussion, as we need to figure out how to
make something like this work for both TTM and GEM drivers.
Signed-off-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/drm_drv.c | 11 ++++++++++-
drivers/gpu/drm/drm_gem.c | 4 ++++
drivers/gpu/drm/drm_gem_dma_helper.c | 4 ++++
include/drm/drm_device.h | 4 ++++
include/drm/drm_gem.h | 2 ++
5 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 251d1d69b09c5..6b1cffd1f52c6 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -930,6 +930,12 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
if (drm_dev_needs_global_mutex(dev))
mutex_lock(&drm_global_mutex);
+ ret = drmm_cgroup_register_device(dev, &dev->cg);
+ if (ret) {
+ DRM_ERROR("Cannot create cgroup device.\n");
+ goto out_unlock;
+ }
+
if (drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL))
accel_debugfs_register(dev);
else
@@ -937,7 +943,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
ret = drm_minor_register(dev, DRM_MINOR_RENDER);
if (ret)
- goto err_minors;
+ goto out_unregister_cgroup;
ret = drm_minor_register(dev, DRM_MINOR_PRIMARY);
if (ret)
@@ -982,6 +988,8 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
drm_minor_unregister(dev, DRM_MINOR_ACCEL);
drm_minor_unregister(dev, DRM_MINOR_PRIMARY);
drm_minor_unregister(dev, DRM_MINOR_RENDER);
+out_unregister_cgroup:
+ dev_cgroup_unregister_device(&dev->cg);
out_unlock:
if (drm_dev_needs_global_mutex(dev))
mutex_unlock(&drm_global_mutex);
@@ -1024,6 +1032,7 @@ void drm_dev_unregister(struct drm_device *dev)
drm_minor_unregister(dev, DRM_MINOR_PRIMARY);
drm_minor_unregister(dev, DRM_MINOR_RENDER);
drm_debugfs_dev_fini(dev);
+ dev_cgroup_unregister_device(&dev->cg);
}
EXPORT_SYMBOL(drm_dev_unregister);
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index ee811764c3df4..95af268d9dd9b 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -110,6 +110,10 @@ drm_gem_init(struct drm_device *dev)
DRM_FILE_PAGE_OFFSET_START,
DRM_FILE_PAGE_OFFSET_SIZE);
+ dev->cg.regions[0].size = U64_MAX;
+ dev->cg.regions[0].name = "gem";
+ dev->cg.num_regions++;
+
return drmm_add_action(dev, drm_gem_init_release, NULL);
}
diff --git a/drivers/gpu/drm/drm_gem_dma_helper.c b/drivers/gpu/drm/drm_gem_dma_helper.c
index 870b90b78bc4e..32eafbf4aa814 100644
--- a/drivers/gpu/drm/drm_gem_dma_helper.c
+++ b/drivers/gpu/drm/drm_gem_dma_helper.c
@@ -106,6 +106,10 @@ __drm_gem_dma_create(struct drm_device *drm, size_t size, bool private)
goto error;
}
+ ret = dev_cgroup_try_charge(&drm->cg, 0, size, &dma_obj->base.cgroup_pool_state, NULL);
+ if (ret)
+ goto error;
+
return dma_obj;
error:
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index c91f87b5242d7..bdb7656e7db67 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -1,6 +1,7 @@
#ifndef _DRM_DEVICE_H_
#define _DRM_DEVICE_H_
+#include <linux/cgroup_dev.h>
#include <linux/list.h>
#include <linux/kref.h>
#include <linux/mutex.h>
@@ -317,6 +318,9 @@ struct drm_device {
* Root directory for debugfs files.
*/
struct dentry *debugfs_root;
+
+ /** @cg: device cgroup bookkeeping */
+ struct dev_cgroup_device cg;
};
#endif
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 5b8b1b059d32c..cc998abea7924 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -430,6 +430,8 @@ struct drm_gem_object {
* The current LRU list that the GEM object is on.
*/
struct drm_gem_lru *lru;
+
+ struct dev_cgroup_pool_state *cgroup_pool_state;
};
/**
--
2.45.2
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup.
2024-10-23 7:52 [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup Maarten Lankhorst
` (6 preceding siblings ...)
2024-10-23 7:53 ` [PATCH 7/7] [DISCUSSION] drm/gem: Add cgroup memory accounting Maarten Lankhorst
@ 2024-10-23 19:40 ` Tejun Heo
2024-10-24 7:20 ` Maxime Ripard
7 siblings, 1 reply; 27+ messages in thread
From: Tejun Heo @ 2024-10-23 19:40 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: intel-xe, linux-kernel, dri-devel, Zefan Li, Johannes Weiner,
Andrew Morton, Friedrich Vock, cgroups, linux-mm, Maxime Ripard
Hello,
On Wed, Oct 23, 2024 at 09:52:53AM +0200, Maarten Lankhorst wrote:
> New submission!
> I've added documentation for each call, and integrated the renaming from
> drm cgroup to dev cgroup, based on maxime ripard's work.
>
> Maxime has been testing this with dma-buf heaps and v4l2 too, and it seems to work.
> In the initial submission, I've decided to only add the smallest enablement possible,
> to have less chance of breaking things.
>
> The API has been changed slightly, from "$name region.$regionname=$limit" in a file called
> dev.min/low/max to "$subsystem/$name $regionname=$limit" in a file called dev.region.min/low/max.
>
> This hopefully allows us to perhaps extend the API later on with the possibility to
> set scheduler weights on the device, like in
>
> https://blogs.igalia.com/tursulin/drm-scheduling-cgroup-controller/
>
> Maarten Lankhorst (5):
> kernel/cgroup: Add "dev" memory accounting cgroup
Yeah, let's not use "dev" name for this. As Waiman pointed out, it conflicts
with the devices controller from cgroup1. While cgroup1 is mostly
deprecated, the same features are provided through BPF in systemd using the
same terminologies, so this is going to be really confusing.
What happened with Tvrtko's weighted implementation? I've seen many proposed
patchsets in this area but as far as I could see none could establish
consensus among GPU crowd and that's one of the reasons why nothing ever
landed. Is the aim of this patchset establishing such consensus?
If reaching consensus doesn't seem feasible in a predictable timeframe, my
suggesstion is just extending the misc controller. If the only way forward
here is fragmented vendor(s)-specific implementations, let's throw them into
the misc controller.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup.
2024-10-23 19:40 ` [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup Tejun Heo
@ 2024-10-24 7:20 ` Maxime Ripard
2024-10-24 17:06 ` Tejun Heo
0 siblings, 1 reply; 27+ messages in thread
From: Maxime Ripard @ 2024-10-24 7:20 UTC (permalink / raw)
To: Tejun Heo
Cc: Maarten Lankhorst, intel-xe, linux-kernel, dri-devel, Zefan Li,
Johannes Weiner, Andrew Morton, Friedrich Vock, cgroups,
linux-mm
[-- Attachment #1: Type: text/plain, Size: 3187 bytes --]
Hi Tejun,
Thanks a lot for your review.
On Wed, Oct 23, 2024 at 09:40:28AM -1000, Tejun Heo wrote:
> On Wed, Oct 23, 2024 at 09:52:53AM +0200, Maarten Lankhorst wrote:
> > New submission!
> > I've added documentation for each call, and integrated the renaming from
> > drm cgroup to dev cgroup, based on maxime ripard's work.
> >
> > Maxime has been testing this with dma-buf heaps and v4l2 too, and it seems to work.
> > In the initial submission, I've decided to only add the smallest enablement possible,
> > to have less chance of breaking things.
> >
> > The API has been changed slightly, from "$name region.$regionname=$limit" in a file called
> > dev.min/low/max to "$subsystem/$name $regionname=$limit" in a file called dev.region.min/low/max.
> >
> > This hopefully allows us to perhaps extend the API later on with the possibility to
> > set scheduler weights on the device, like in
> >
> > https://blogs.igalia.com/tursulin/drm-scheduling-cgroup-controller/
> >
> > Maarten Lankhorst (5):
> > kernel/cgroup: Add "dev" memory accounting cgroup
>
> Yeah, let's not use "dev" name for this. As Waiman pointed out, it conflicts
> with the devices controller from cgroup1. While cgroup1 is mostly
> deprecated, the same features are provided through BPF in systemd using the
> same terminologies, so this is going to be really confusing.
Yeah, I agree. We switched to dev because we want to support more than
just DRM, but all DMA-able memory. We have patches to support for v4l2
and dma-buf heaps, so using the name DRM didn't feel great either.
Do you have a better name in mind? "device memory"? "dma memory"?
> What happened with Tvrtko's weighted implementation? I've seen many proposed
> patchsets in this area but as far as I could see none could establish
> consensus among GPU crowd and that's one of the reasons why nothing ever
> landed. Is the aim of this patchset establishing such consensus?
Yeah, we have a consensus by now I think. Valve, Intel, Google, and Red
Hat have been involved in that series and we all agree on the implementation.
Tvrtko aims at a different feature set though: this one is about memory
allocation limits, Tvrtko's about scheduling.
Scheduling doesn't make much sense for things outside of DRM (and even
for a fraction of all DRM devices), and it's pretty much orthogonal. So
i guess you can expect another series from Tvrtko, but I don't think
they should be considered equivalent or dependent on each other.
> If reaching consensus doesn't seem feasible in a predictable timeframe, my
> suggesstion is just extending the misc controller. If the only way forward
> here is fragmented vendor(s)-specific implementations, let's throw them into
> the misc controller.
I don't think we have a fragmented implementation here, at all. The last
patch especially implements it for all devices implementing the GEM
interface in DRM, which would be around 100 drivers from various vendors.
It's marked as a discussion because we don't quite know how to plumb it
in for all drivers in the current DRM framework, but it's very much what
we want to achieve.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup.
2024-10-24 7:20 ` Maxime Ripard
@ 2024-10-24 17:06 ` Tejun Heo
2024-10-28 10:05 ` Maxime Ripard
0 siblings, 1 reply; 27+ messages in thread
From: Tejun Heo @ 2024-10-24 17:06 UTC (permalink / raw)
To: Maxime Ripard
Cc: Maarten Lankhorst, intel-xe, linux-kernel, dri-devel, Zefan Li,
Johannes Weiner, Andrew Morton, Friedrich Vock, cgroups,
linux-mm
Hello,
On Thu, Oct 24, 2024 at 09:20:43AM +0200, Maxime Ripard wrote:
...
> > Yeah, let's not use "dev" name for this. As Waiman pointed out, it conflicts
> > with the devices controller from cgroup1. While cgroup1 is mostly
> > deprecated, the same features are provided through BPF in systemd using the
> > same terminologies, so this is going to be really confusing.
>
> Yeah, I agree. We switched to dev because we want to support more than
> just DRM, but all DMA-able memory. We have patches to support for v4l2
> and dma-buf heaps, so using the name DRM didn't feel great either.
>
> Do you have a better name in mind? "device memory"? "dma memory"?
Maybe just dma (I think the term isn't used heavily anymore, so the word is
kinda open)? But, hopefully, others have better ideas.
> > What happened with Tvrtko's weighted implementation? I've seen many proposed
> > patchsets in this area but as far as I could see none could establish
> > consensus among GPU crowd and that's one of the reasons why nothing ever
> > landed. Is the aim of this patchset establishing such consensus?
>
> Yeah, we have a consensus by now I think. Valve, Intel, Google, and Red
> Hat have been involved in that series and we all agree on the implementation.
That's great to hear.
> Tvrtko aims at a different feature set though: this one is about memory
> allocation limits, Tvrtko's about scheduling.
>
> Scheduling doesn't make much sense for things outside of DRM (and even
> for a fraction of all DRM devices), and it's pretty much orthogonal. So
> i guess you can expect another series from Tvrtko, but I don't think
> they should be considered equivalent or dependent on each other.
Yeah, I get that this is about memory and that is about processing capacity,
so the plan is going for separate controllers for each? Or would it be
better to present both under the same controller interface? Even if they're
going to be separate controllers, we at least want to be aligned on how
devices and their configurations are presented in the two controllers.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup.
2024-10-24 17:06 ` Tejun Heo
@ 2024-10-28 10:05 ` Maxime Ripard
2024-10-29 20:38 ` Johannes Weiner
0 siblings, 1 reply; 27+ messages in thread
From: Maxime Ripard @ 2024-10-28 10:05 UTC (permalink / raw)
To: Tejun Heo
Cc: Maarten Lankhorst, intel-xe, linux-kernel, dri-devel, Zefan Li,
Johannes Weiner, Andrew Morton, Friedrich Vock, cgroups,
linux-mm
[-- Attachment #1: Type: text/plain, Size: 2657 bytes --]
On Thu, Oct 24, 2024 at 07:06:36AM -1000, Tejun Heo wrote:
> Hello,
>
> On Thu, Oct 24, 2024 at 09:20:43AM +0200, Maxime Ripard wrote:
> ...
> > > Yeah, let's not use "dev" name for this. As Waiman pointed out, it conflicts
> > > with the devices controller from cgroup1. While cgroup1 is mostly
> > > deprecated, the same features are provided through BPF in systemd using the
> > > same terminologies, so this is going to be really confusing.
> >
> > Yeah, I agree. We switched to dev because we want to support more than
> > just DRM, but all DMA-able memory. We have patches to support for v4l2
> > and dma-buf heaps, so using the name DRM didn't feel great either.
> >
> > Do you have a better name in mind? "device memory"? "dma memory"?
>
> Maybe just dma (I think the term isn't used heavily anymore, so the word is
> kinda open)? But, hopefully, others have better ideas.
>
> > > What happened with Tvrtko's weighted implementation? I've seen many proposed
> > > patchsets in this area but as far as I could see none could establish
> > > consensus among GPU crowd and that's one of the reasons why nothing ever
> > > landed. Is the aim of this patchset establishing such consensus?
> >
> > Yeah, we have a consensus by now I think. Valve, Intel, Google, and Red
> > Hat have been involved in that series and we all agree on the implementation.
>
> That's great to hear.
>
> > Tvrtko aims at a different feature set though: this one is about memory
> > allocation limits, Tvrtko's about scheduling.
> >
> > Scheduling doesn't make much sense for things outside of DRM (and even
> > for a fraction of all DRM devices), and it's pretty much orthogonal. So
> > i guess you can expect another series from Tvrtko, but I don't think
> > they should be considered equivalent or dependent on each other.
>
> Yeah, I get that this is about memory and that is about processing capacity,
> so the plan is going for separate controllers for each? Or would it be
> better to present both under the same controller interface? Even if they're
> going to be separate controllers, we at least want to be aligned on how
> devices and their configurations are presented in the two controllers.
It's still up in the air, I think.
My personal opinion is that there's only DRM (and accel) devices that
really care about scheduling constraints anyway, so it wouldn't (have
to) be as generic as this one.
And if we would call it dma, then the naming becomes a bit weird since
DMA doesn't have much to do with scheduling.
But I guess it's just another instance of the "naming is hard" problem :)
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup.
2024-10-28 10:05 ` Maxime Ripard
@ 2024-10-29 20:38 ` Johannes Weiner
2024-11-06 10:31 ` Maxime Ripard
0 siblings, 1 reply; 27+ messages in thread
From: Johannes Weiner @ 2024-10-29 20:38 UTC (permalink / raw)
To: Maxime Ripard
Cc: Tejun Heo, Maarten Lankhorst, intel-xe, linux-kernel, dri-devel,
Zefan Li, Andrew Morton, Friedrich Vock, cgroups, linux-mm
On Mon, Oct 28, 2024 at 11:05:48AM +0100, Maxime Ripard wrote:
> On Thu, Oct 24, 2024 at 07:06:36AM -1000, Tejun Heo wrote:
> > Hello,
> >
> > On Thu, Oct 24, 2024 at 09:20:43AM +0200, Maxime Ripard wrote:
> > ...
> > > > Yeah, let's not use "dev" name for this. As Waiman pointed out, it conflicts
> > > > with the devices controller from cgroup1. While cgroup1 is mostly
> > > > deprecated, the same features are provided through BPF in systemd using the
> > > > same terminologies, so this is going to be really confusing.
> > >
> > > Yeah, I agree. We switched to dev because we want to support more than
> > > just DRM, but all DMA-able memory. We have patches to support for v4l2
> > > and dma-buf heaps, so using the name DRM didn't feel great either.
> > >
> > > Do you have a better name in mind? "device memory"? "dma memory"?
> >
> > Maybe just dma (I think the term isn't used heavily anymore, so the word is
> > kinda open)? But, hopefully, others have better ideas.
> >
> > > > What happened with Tvrtko's weighted implementation? I've seen many proposed
> > > > patchsets in this area but as far as I could see none could establish
> > > > consensus among GPU crowd and that's one of the reasons why nothing ever
> > > > landed. Is the aim of this patchset establishing such consensus?
> > >
> > > Yeah, we have a consensus by now I think. Valve, Intel, Google, and Red
> > > Hat have been involved in that series and we all agree on the implementation.
> >
> > That's great to hear.
> >
> > > Tvrtko aims at a different feature set though: this one is about memory
> > > allocation limits, Tvrtko's about scheduling.
> > >
> > > Scheduling doesn't make much sense for things outside of DRM (and even
> > > for a fraction of all DRM devices), and it's pretty much orthogonal. So
> > > i guess you can expect another series from Tvrtko, but I don't think
> > > they should be considered equivalent or dependent on each other.
> >
> > Yeah, I get that this is about memory and that is about processing capacity,
> > so the plan is going for separate controllers for each? Or would it be
> > better to present both under the same controller interface? Even if they're
> > going to be separate controllers, we at least want to be aligned on how
> > devices and their configurations are presented in the two controllers.
>
> It's still up in the air, I think.
>
> My personal opinion is that there's only DRM (and accel) devices that
> really care about scheduling constraints anyway, so it wouldn't (have
> to) be as generic as this one.
If they represent different resources that aren't always controlled in
conjunction, it makes sense to me to have separate controllers as well.
Especially if a merged version would have separate control files for
each resource anyway (dev.region.*, dev.weight etc.)
> And if we would call it dma, then the naming becomes a bit weird since
> DMA doesn't have much to do with scheduling.
>
> But I guess it's just another instance of the "naming is hard" problem :)
Yes it would be good to have something catchy, easy on the eyes, and
vaguely familiar. devcomp(ute), devproc, devcpu, devcycles all kind of
suck. drm and gpu seem too specific for a set that includes npus and
potentially other accelerators in the future.
I don't think we want to go full devspace & devtime, either, though.
How about dmem for this one, and dpu for the other one. For device
memory and device processing unit, respectively.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup.
2024-10-29 20:38 ` Johannes Weiner
@ 2024-11-06 10:31 ` Maxime Ripard
2024-11-06 18:20 ` Tejun Heo
0 siblings, 1 reply; 27+ messages in thread
From: Maxime Ripard @ 2024-11-06 10:31 UTC (permalink / raw)
To: Johannes Weiner
Cc: Tejun Heo, Maarten Lankhorst, intel-xe, linux-kernel, dri-devel,
Zefan Li, Andrew Morton, Friedrich Vock, cgroups, linux-mm
[-- Attachment #1: Type: text/plain, Size: 3806 bytes --]
On Tue, Oct 29, 2024 at 04:38:34PM -0400, Johannes Weiner wrote:
> On Mon, Oct 28, 2024 at 11:05:48AM +0100, Maxime Ripard wrote:
> > On Thu, Oct 24, 2024 at 07:06:36AM -1000, Tejun Heo wrote:
> > > Hello,
> > >
> > > On Thu, Oct 24, 2024 at 09:20:43AM +0200, Maxime Ripard wrote:
> > > ...
> > > > > Yeah, let's not use "dev" name for this. As Waiman pointed out, it conflicts
> > > > > with the devices controller from cgroup1. While cgroup1 is mostly
> > > > > deprecated, the same features are provided through BPF in systemd using the
> > > > > same terminologies, so this is going to be really confusing.
> > > >
> > > > Yeah, I agree. We switched to dev because we want to support more than
> > > > just DRM, but all DMA-able memory. We have patches to support for v4l2
> > > > and dma-buf heaps, so using the name DRM didn't feel great either.
> > > >
> > > > Do you have a better name in mind? "device memory"? "dma memory"?
> > >
> > > Maybe just dma (I think the term isn't used heavily anymore, so the word is
> > > kinda open)? But, hopefully, others have better ideas.
> > >
> > > > > What happened with Tvrtko's weighted implementation? I've seen many proposed
> > > > > patchsets in this area but as far as I could see none could establish
> > > > > consensus among GPU crowd and that's one of the reasons why nothing ever
> > > > > landed. Is the aim of this patchset establishing such consensus?
> > > >
> > > > Yeah, we have a consensus by now I think. Valve, Intel, Google, and Red
> > > > Hat have been involved in that series and we all agree on the implementation.
> > >
> > > That's great to hear.
> > >
> > > > Tvrtko aims at a different feature set though: this one is about memory
> > > > allocation limits, Tvrtko's about scheduling.
> > > >
> > > > Scheduling doesn't make much sense for things outside of DRM (and even
> > > > for a fraction of all DRM devices), and it's pretty much orthogonal. So
> > > > i guess you can expect another series from Tvrtko, but I don't think
> > > > they should be considered equivalent or dependent on each other.
> > >
> > > Yeah, I get that this is about memory and that is about processing capacity,
> > > so the plan is going for separate controllers for each? Or would it be
> > > better to present both under the same controller interface? Even if they're
> > > going to be separate controllers, we at least want to be aligned on how
> > > devices and their configurations are presented in the two controllers.
> >
> > It's still up in the air, I think.
> >
> > My personal opinion is that there's only DRM (and accel) devices that
> > really care about scheduling constraints anyway, so it wouldn't (have
> > to) be as generic as this one.
>
> If they represent different resources that aren't always controlled in
> conjunction, it makes sense to me to have separate controllers as well.
>
> Especially if a merged version would have separate control files for
> each resource anyway (dev.region.*, dev.weight etc.)
>
> > And if we would call it dma, then the naming becomes a bit weird since
> > DMA doesn't have much to do with scheduling.
> >
> > But I guess it's just another instance of the "naming is hard" problem :)
>
> Yes it would be good to have something catchy, easy on the eyes, and
> vaguely familiar. devcomp(ute), devproc, devcpu, devcycles all kind of
> suck. drm and gpu seem too specific for a set that includes npus and
> potentially other accelerators in the future.
>
> I don't think we want to go full devspace & devtime, either, though.
>
> How about dmem for this one, and dpu for the other one. For device
> memory and device processing unit, respectively.
dmem sounds great to me, does everyone agree?
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup.
2024-11-06 10:31 ` Maxime Ripard
@ 2024-11-06 18:20 ` Tejun Heo
2024-11-13 14:58 ` Maarten Lankhorst
0 siblings, 1 reply; 27+ messages in thread
From: Tejun Heo @ 2024-11-06 18:20 UTC (permalink / raw)
To: Maxime Ripard
Cc: Johannes Weiner, Maarten Lankhorst, intel-xe, linux-kernel,
dri-devel, Zefan Li, Andrew Morton, Friedrich Vock, cgroups,
linux-mm
On Wed, Nov 06, 2024 at 11:31:49AM +0100, Maxime Ripard wrote:
...
> > How about dmem for this one, and dpu for the other one. For device
> > memory and device processing unit, respectively.
>
> dmem sounds great to me, does everyone agree?
Sounds good to me.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup.
2024-11-06 18:20 ` Tejun Heo
@ 2024-11-13 14:58 ` Maarten Lankhorst
2024-11-13 18:29 ` Tejun Heo
0 siblings, 1 reply; 27+ messages in thread
From: Maarten Lankhorst @ 2024-11-13 14:58 UTC (permalink / raw)
To: Tejun Heo, Maxime Ripard
Cc: Johannes Weiner, intel-xe, linux-kernel, dri-devel, Zefan Li,
Andrew Morton, Friedrich Vock, cgroups, linux-mm
Hey,
Den 2024-11-06 kl. 19:20, skrev Tejun Heo:
> On Wed, Nov 06, 2024 at 11:31:49AM +0100, Maxime Ripard wrote:
> ...
>>> How about dmem for this one, and dpu for the other one. For device
>>> memory and device processing unit, respectively.
>>
>> dmem sounds great to me, does everyone agree?
>
> Sounds good to me.
>
> Thanks.
>
Thanks for all feedback and discussion. I checked mostly on patchwork so
I missed the discussion here. Fortunately it's only been about naming. :)
I'm thinking of adding a 'high' knob as well, that will work similarly
to high in normal mem controller. (so not proportionally calculated like
'max', but (usage + allocated) < max = ok.
Recursively of course.
Cheers,
~Maarten
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 0/7] kernel/cgroups: Add "dev" memory accounting cgroup.
2024-11-13 14:58 ` Maarten Lankhorst
@ 2024-11-13 18:29 ` Tejun Heo
0 siblings, 0 replies; 27+ messages in thread
From: Tejun Heo @ 2024-11-13 18:29 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: Maxime Ripard, Johannes Weiner, intel-xe, linux-kernel,
dri-devel, Zefan Li, Andrew Morton, Friedrich Vock, cgroups,
linux-mm
Hello,
On Wed, Nov 13, 2024 at 03:58:25PM +0100, Maarten Lankhorst wrote:
...
> Thanks for all feedback and discussion. I checked mostly on patchwork so I
> missed the discussion here. Fortunately it's only been about naming. :)
>
> I'm thinking of adding a 'high' knob as well, that will work similarly to
> high in normal mem controller. (so not proportionally calculated like 'max',
> but (usage + allocated) < max = ok.
>
> Recursively of course.
I'd be cautious about adding knobs. These being published API, it's easy to
paint oneself into a corner. I suggest starting with what's essential.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 27+ messages in thread