Add an owner to the mm_struct (v3)

This patch removes the mem_cgroup member from mm_struct and instead adds an owner. This approach was suggested by Paul Menage. The advantage of this approach is that, once the mm->owner is known, using the subsystem id, the cgroup can be determined. It also allows several control groups that are virtually grouped by mm_struct, to exist independent of the memory controller i.e., without adding mem_cgroup's for each controller, to mm_struct.

A new config option CONFIG_MM_OWNER is added and the memory resource controller selects this config option.

NOTE: This patch was developed on top of 2.6.25-rc5-mm1 and is applied on top of the memory-controller-move-to-own-slab patch (which is already present in the Andrew's patchset).

I am indebted to Paul Menage for the several reviews of this patchset and helping me make it lighter and simpler.

This patch was tested on a powerpc box, by running a task under the memory resource controller and moving it across groups at a constant interval.

Signed-off-by Balbir Singh <[email protected]>

---

changing mm->owner without notifying controllers makes it difficult to use. can you provide a notification mechanism?

Quoted 4 lines · YAMAMOTO Takashi

changing mm->owner without notifying controllers makes it difficult to use. can you provide a notification mechanism?

Yes, I think that call will need to be in the task_lock() critical section in which we update mm->owner.

Right now I think the only user that needs to be notified at that point is Balbir's virtual address limits controller.

Paul

Quoted 18 lines

This patch removes the mem_cgroup member from mm_struct and instead adds an owner. This approach was suggested by Paul Menage. The advantage of this approach is that, once the mm->owner is known, using the subsystem id, the cgroup can be determined. It also allows several control groups that are virtually grouped by mm_struct, to exist independent of the memory controller i.e., without adding mem_cgroup's for each controller, to mm_struct.

A new config option CONFIG_MM_OWNER is added and the memory resource controller selects this config option.

NOTE: This patch was developed on top of 2.6.25-rc5-mm1 and is applied on top of the memory-controller-move-to-own-slab patch (which is already present in the Andrew's patchset).

I am indebted to Paul Menage for the several reviews of this patchset and helping me make it lighter and simpler.

Quoted 5 lines

This patch was tested on a powerpc box, by running a task under the memory resource controller and moving it across groups at a constant interval.

Signed-off-by Balbir Singh <[email protected]>

---

changing mm->owner without notifying controllers makes it difficult to use. can you provide a notification mechanism?

-static struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p) +struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p)
{
return container_of(task_subsys_state(p, mem_cgroup_subsys_id),
struct mem_cgroup, css);
}

+static inline void mm_fork_init_owner(struct task_struct *p) +{
+}

+
+void mm_update_next_owner(struct mm_struct *mm)
+{
+ struct task_struct *c, *g, *p = current;
+
+ /*
+ * This routine should not be called for init_task
+ */
+ BUG_ON(p == p->parent);

+ list_for_each_entry(c, &p->children, sibling) {
+ if (c->mm && (c->mm == mm))

+assign_new_owner:

 +       BUG_ON(c == p);
 +       task_lock(c);
 +       if (c->mm != mm) {

+ task_unlock(c);
+ goto retry;
+ }
+ mm->owner = c;

Quoted 6 lines · Balbir Singh

-static struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p) +struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p)
{
return container_of(task_subsys_state(p, mem_cgroup_subsys_id),
struct mem_cgroup, css);
}

This should probably be inlined in the header file if it's needed outside this file.

Quoted 3 lines

+static inline void mm_fork_init_owner(struct task_struct *p) +{
+}

I think this is stale.

Quoted 9 lines

+
+void mm_update_next_owner(struct mm_struct *mm)
+{
+ struct task_struct *c, *g, *p = current;
+
+ /*
+ * This routine should not be called for init_task
+ */
+ BUG_ON(p == p->parent);

I think (as you mentioned earlier) that we need an RCU critical section in this function, in order for the tasklist traversal to be safe.

Maybe also BUG_ON(p != mm->owner) ?

Quoted 2 lines

+ list_for_each_entry(c, &p->children, sibling) {
+ if (c->mm && (c->mm == mm))

Since mm != NULL, no need to test for c->mm since if it's NULL then c->mm != mm

Quoted 8 lines

+assign_new_owner:

 +       BUG_ON(c == p);
 +       task_lock(c);
 +       if (c->mm != mm) {

+ task_unlock(c);
+ goto retry;
+ }
+ mm->owner = c;

Here we'll want to call vm_cgroup_update_mm_owner(), to adjust the accounting. (Or if in future we end up with more than a couple of subsystems that want notification at this time, we'll want to call cgroup_update_mm_owner() and have it call any interested subsystems.

Paul

Quoted 5 lines

Here we'll want to call vm_cgroup_update_mm_owner(), to adjust the accounting. (Or if in future we end up with more than a couple of subsystems that want notification at this time, we'll want to call cgroup_update_mm_owner() and have it call any interested subsystems.

I don't think we need to adjust accounting, since only mm->owner is changing and not the cgroup to which the task/mm belongs. Do we really need to notify? I don't want to do any notifications under task_lock().

Quoted 6 lines · Balbir Singh

Quoted 5 lines

Here we'll want to call vm_cgroup_update_mm_owner(), to adjust the accounting. (Or if in future we end up with more than a couple of subsystems that want notification at this time, we'll want to call cgroup_update_mm_owner() and have it call any interested subsystems.

Quoted 3 lines

I don't think we need to adjust accounting, since only mm->owner is changing and not the cgroup to which the task/mm belongs. Do we really need to notify? I don't want to do any notifications under task_lock().

It's possible but unlikely that the new owner is in a different cgroup.

Quoted 11 lines · Paul Menage

Quoted 9 lines · Balbir Singh

Quoted 5 lines

Here we'll want to call vm_cgroup_update_mm_owner(), to adjust the accounting. (Or if in future we end up with more than a couple of subsystems that want notification at this time, we'll want to call cgroup_update_mm_owner() and have it call any interested subsystems.

I don't think we need to adjust accounting, since only mm->owner is changing and not the cgroup to which the task/mm belongs. Do we really need to notify? I don't want to do any notifications under task_lock().

Quoted 1 line

It's possible but unlikely that the new owner is in a different cgroup.

Hmmm... that can never happen with thread groups, since mm->owner is p->group_leader and that never exits unless all threads are gone (it can explicitly change groups though). Without thread groups, the new owner can belong to a different cgroup, so we might need notification.