Add rlimit controller to cgroups (v3)

mm/rlimitcgroup.c line 93

Quoted 1 line · On Sun, 04 May 2008 03:07:36 +0530 Balbir Singh <[email protected]>…

mm/rlimitcgroup.c new file +1 -0

+	*tmp = ((*tmp + PAGE_SIZE) >> PAGE_SHIFT) << PAGE_SHIFT;

Whatever this is doing, it should not be doing it this way ;)

perhaps

	*tmp = ALIGN(*tmp, PAGE_SIZE);

or even

	*tmp = PAGE_ALIGN(*tmp);

?

<looks at PAGE_ALIGN>

Each architecture implements its own version and they of course do it differently. It's crying out for a consolidated implementation but we have no include/linux/page.h into which to consolidate it.

mm/rlimitcgroup.c line 41

Quoted 10 lines · Balbir Singh

mm/rlimitcgroup.c new file +10 -0

+struct cgroup_subsys rlimit_cgroup_subsys;
+
+struct rlimit_cgroup {
+	struct cgroup_subsys_state css;
+	struct res_counter as_res;	/* address space counter */
+};
+
+static struct rlimit_cgroup init_rlimit_cgroup;
+
+struct rlimit_cgroup *rlimit_cgroup_from_cgrp(struct cgroup *cgrp)

It can be static if I don't miss anything.

This patch adds an additional field to the mm_owner callbacks. This field is required to get to the mm that changed.

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

---

 include/linux/cgroup.h |    3 ++-
 kernel/cgroup.c        |    2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff -puN kernel/cgroup.c~cgroup-add-task-to-mm--owner-callbacks kernel/cgroup.c

kernel/cgroup.c renamed from kernel/cgroup.c~cgroup-add-task-to-mm--owner-callbacks +1 -1

@@ -2772,7 +2772,7 @@ void cgroup_mm_owner_callbacks(struct ta
 			if (oldcgrp == newcgrp)
 				continue;
 			if (ss->mm_owner_changed)
-				ss->mm_owner_changed(ss, oldcgrp, newcgrp);
+				ss->mm_owner_changed(ss, oldcgrp, newcgrp, new);
 		}
 	}
 }

diff -puN include/linux/cgroup.h~cgroup-add-task-to-mm--owner-callbacks include/linux/cgroup.h

Quoted 28 lines · On Sun, 04 May 2008 03:08:04 +0530 Balbir Singh <[email protected]>…

This patch adds an additional field to the mm_owner callbacks. This field is required to get to the mm that changed.

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

---

 include/linux/cgroup.h |    3 ++-
 kernel/cgroup.c        |    2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff -puN kernel/cgroup.c~cgroup-add-task-to-mm--owner-callbacks kernel/cgroup.c

kernel/cgroup.c renamed from kernel/cgroup.c~cgroup-add-task-to-mm--owner-callbacks +1 -1

@@ -2772,7 +2772,7 @@ void cgroup_mm_owner_callbacks(struct ta
 			if (oldcgrp == newcgrp)
 				continue;
 			if (ss->mm_owner_changed)
-				ss->mm_owner_changed(ss, oldcgrp, newcgrp);
+				ss->mm_owner_changed(ss, oldcgrp, newcgrp, new);
 		}
 	}
 }

diff -puN include/linux/cgroup.h~cgroup-add-task-to-mm--owner-callbacks include/linux/cgroup.h

include/linux/cgroup.h renamed from include/linux/cgroup.h~cgroup-add-task-to-mm--owner-callbacks +0 -0

@@ -310,7 +310,8 @@ struct cgroup_subsys {
 	 */

include/linux/cgroup.h struct cgroup_subsys { · line 314

Quoted 5 lines

include/linux/cgroup.h renamed from include/linux/cgroup.h~cgroup-add-task-to-mm--owner-callbacks +2 -1

 	void (*mm_owner_changed)(struct cgroup_subsys *ss,
 					struct cgroup *old,
-					struct cgroup *new);
+					struct cgroup *new,
+					struct task_struct *p);

If mm_owner_changed() had any documentation I'd suggest that it be updated. Sneaky.

The existing comment:

	/*
	 * This routine is called with the task_lock of mm->owner held
	 */
	void (*mm_owner_changed)(struct cgroup_subsys *ss,
					struct cgroup *old,
					struct cgroup *new);

Is rather mysterious. To what mm does it refer?

This patch adds an additional field to the mm_owner callbacks. This field is required to get to the mm that changed.

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

---

  include/linux/cgroup.h |    3 ++-
  kernel/cgroup.c        |    2 +-
  2 files changed, 3 insertions(+), 2 deletions(-)

diff -puN kernel/cgroup.c~cgroup-add-task-to-mm--owner-callbacks kernel/cgroup.c
--- linux-2.6.25/kernel/cgroup.c~cgroup-add-task-to-mm--owner-callbacks 2008-05-04 02:53:05.000000000 +0530 +++ linux-2.6.25-balbir/kernel/cgroup.c 2008-05-04 02:53:05.000000000 +0530
@@ -2772,7 +2772,7 @@ void cgroup_mm_owner_callbacks(struct ta

                        if (oldcgrp == newcgrp)
                                continue;
                        if (ss->mm_owner_changed)
 -                               ss->mm_owner_changed(ss, oldcgrp, newcgrp);
 +                               ss->mm_owner_changed(ss, oldcgrp, newcgrp, new);
                }
        }
  }
 diff -puN include/linux/cgroup.h~cgroup-add-task-to-mm--owner-callbacks include/linux/cgroup.h
 --- linux-2.6.25/include/linux/cgroup.h~cgroup-add-task-to-mm--owner-callbacks  2008-05-04 02:53:05.000000000 +0530
 +++ linux-2.6.25-balbir/include/linux/cgroup.h  2008-05-04 02:53:05.000000000 +0530
 @@ -310,7 +310,8 @@ struct cgroup_subsys {
         */
        void (*mm_owner_changed)(struct cgroup_subsys *ss,
                                        struct cgroup *old,
 -                                       struct cgroup *new);
 +                                       struct cgroup *new,
 +                                       struct task_struct *p);
        int subsys_id;
        int active;
        int disabled;

_

--
Warm Regards,
Balbir Singh
Linux Technology Center
IBM, ISTL

IOW, have we chosen the best, most maintainable representation for these things?

Review

Quoted 1 line · On Sun, 04 May 2008 03:08:14 +0530 Balbir Singh <[email protected]>…

(quoted diff) +1 -0

+	if (res_counter_charge(&rcg->as_res, (mm->total_vm << PAGE_SHIFT)))

I worry a bit about all the conversion between page-counts and byte-counts in this code.

For example, what happens if a process sits there increasing its rss with sbrk(4095) or sbrk(4097) or all sorts of other scenarios? Do we get in a situation in which the accounting is systematically wrong?

Worse, do we risk getting into that situation in the future, as unrelated changes are made to the surrounding code?

IOW, have we chosen the best, most maintainable representation for these things?

+
+int rlimit_cgroup_charge_as(struct mm_struct *mm, unsigned long nr_pages) +{
+ int ret;
+ struct rlimit_cgroup *rcg;
+

 +       rcu_read_lock();
 +       rcg = rlimit_cgroup_from_task(rcu_dereference(mm->owner));
 +       css_get(&rcg->css);
 +       rcu_read_unlock();
 +
 +       ret = res_counter_charge(&rcg->as_res, (nr_pages << PAGE_SHIFT));
 +       css_put(&rcg->css);
 +       return ret;

+}

+
+void rlimit_cgroup_uncharge_as(struct mm_struct *mm, unsigned long nr_pages) +{
+ struct rlimit_cgroup *rcg;
+

 +       rcu_read_lock();
 +       rcg = rlimit_cgroup_from_task(rcu_dereference(mm->owner));
 +       css_get(&rcg->css);
 +       rcu_read_unlock();

+
+ res_counter_uncharge(&rcg->as_res, (nr_pages << PAGE_SHIFT));
+ css_put(&rcg->css);
+}

+/*
+ * TODO: get the attach callbacks to fail and disallow task movement.
+ */

+static void rlimit_cgroup_move_task(struct cgroup_subsys *ss,

 +                                       struct cgroup *cgrp,
 +                                       struct cgroup *old_cgrp,
 +                                       struct task_struct *p)

+{
+ struct mm_struct *mm;
+ struct rlimit_cgroup *rcg, *old_rcg;
+
+ mm = get_task_mm(p);
+ if (mm == NULL)
+ return;
+
+ rcu_read_lock();
+ if (p != rcu_dereference(mm->owner))
+ goto out;
+
+ rcg = rlimit_cgroup_from_cgrp(cgrp);
+ old_rcg = rlimit_cgroup_from_cgrp(old_cgrp);
+
+ if (rcg == old_rcg)
+ goto out;
+

+ if (res_counter_charge(&rcg->as_res, (mm->total_vm << PAGE_SHIFT)))
+ goto out;
+ res_counter_uncharge(&old_rcg->as_res, (mm->total_vm << PAGE_SHIFT));
+out:
+ rcu_read_unlock();
+ mmput(mm);
+}
+

+static void rlimit_cgroup_mm_owner_changed(struct cgroup_subsys *ss,

 +                                               struct cgroup *cgrp,
 +                                               struct cgroup *old_cgrp,
 +                                               struct task_struct *p)
 +{
 +       struct rlimit_cgroup *rcg, *old_rcg;
 +       struct mm_struct *mm = get_task_mm(p);
 +
 +       BUG_ON(!mm);
 +       rcg = rlimit_cgroup_from_cgrp(cgrp);
 +       old_rcg = rlimit_cgroup_from_cgrp(old_cgrp);
 +       if (res_counter_charge(&rcg->as_res, (mm->total_vm << PAGE_SHIFT)))

+ goto out;
+ res_counter_uncharge(&old_rcg->as_res, (mm->total_vm << PAGE_SHIFT));
+out:
+ mmput(mm);
+}
+

Quoted 14 lines · Balbir Singh

+
+int rlimit_cgroup_charge_as(struct mm_struct *mm, unsigned long nr_pages) +{
+ int ret;
+ struct rlimit_cgroup *rcg;
+

 +       rcu_read_lock();
 +       rcg = rlimit_cgroup_from_task(rcu_dereference(mm->owner));
 +       css_get(&rcg->css);
 +       rcu_read_unlock();
 +
 +       ret = res_counter_charge(&rcg->as_res, (nr_pages << PAGE_SHIFT));
 +       css_put(&rcg->css);
 +       return ret;

Quoted 1 line

+}

You need to synchronize against mm->owner changing, or mm->owner->cgroups changing. How about:

int rlimit_cgroup_charge_as(struct mm_struct *mm, unsigned long nr_pages) {
int ret;
struct rlimit_cgroup *rcg;
struct task_struct *owner;

rcu_read_lock();
again:

  /* Find and lock the owner of the mm */
  owner = rcu_dereference(mm->owner);
  task_lock(owner);
  if (mm->owner != owner) {
    task_unlock(owner);
    goto again;
  }

/* Charge the owner's cgroup with the new memory */ rcg = rlimit_cgroup_from_task(owner);
ret = res_counter_charge(&rcg->as_res, (nr_pages << PAGE_SHIFT)); task_unlock(owner);
rcu_read_unlock();
return ret;
}

Quoted 5 lines

+
+void rlimit_cgroup_uncharge_as(struct mm_struct *mm, unsigned long nr_pages) +{
+ struct rlimit_cgroup *rcg;
+

Quoted 8 lines

 +       rcu_read_lock();
 +       rcg = rlimit_cgroup_from_task(rcu_dereference(mm->owner));
 +       css_get(&rcg->css);
 +       rcu_read_unlock();

+
+ res_counter_uncharge(&rcg->as_res, (nr_pages << PAGE_SHIFT));
+ css_put(&rcg->css);
+}

Can't this be implemented as just a call to charge() with a negative value? (Possibly fixing res_counter_charge() to handle negative values if necessary) Seems simpler.

Quoted 3 lines

+/*
+ * TODO: get the attach callbacks to fail and disallow task movement.
+ */

You mean disallow all movement within a hierarchy that has this cgroup mounted? Doesn't that make it rather hard to use?

Quoted 22 lines

+static void rlimit_cgroup_move_task(struct cgroup_subsys *ss,

 +                                       struct cgroup *cgrp,
 +                                       struct cgroup *old_cgrp,
 +                                       struct task_struct *p)

+{
+ struct mm_struct *mm;
+ struct rlimit_cgroup *rcg, *old_rcg;
+
+ mm = get_task_mm(p);
+ if (mm == NULL)
+ return;
+
+ rcu_read_lock();
+ if (p != rcu_dereference(mm->owner))
+ goto out;
+
+ rcg = rlimit_cgroup_from_cgrp(cgrp);
+ old_rcg = rlimit_cgroup_from_cgrp(old_cgrp);
+
+ if (rcg == old_rcg)
+ goto out;
+

Quoted 8 lines

+ if (res_counter_charge(&rcg->as_res, (mm->total_vm << PAGE_SHIFT)))
+ goto out;
+ res_counter_uncharge(&old_rcg->as_res, (mm->total_vm << PAGE_SHIFT));
+out:
+ rcu_read_unlock();
+ mmput(mm);
+}
+

Since you need to protect against concurrent charges, and against concurrent mm ownership changes, I think you should just do this under task_lock(p).

Quoted 12 lines

+static void rlimit_cgroup_mm_owner_changed(struct cgroup_subsys *ss,

 +                                               struct cgroup *cgrp,
 +                                               struct cgroup *old_cgrp,
 +                                               struct task_struct *p)
 +{
 +       struct rlimit_cgroup *rcg, *old_rcg;
 +       struct mm_struct *mm = get_task_mm(p);
 +
 +       BUG_ON(!mm);
 +       rcg = rlimit_cgroup_from_cgrp(cgrp);
 +       old_rcg = rlimit_cgroup_from_cgrp(old_cgrp);
 +       if (res_counter_charge(&rcg->as_res, (mm->total_vm << PAGE_SHIFT)))

Quoted 6 lines

+ goto out;
+ res_counter_uncharge(&old_rcg->as_res, (mm->total_vm << PAGE_SHIFT));
+out:
+ mmput(mm);
+}
+

Also needs to task_lock(p) to prevent concurrent charges or cgroup reassignments?

Paul

Quoted 14 lines · Balbir Singh

+
+int rlimit_cgroup_charge_as(struct mm_struct *mm, unsigned long nr_pages) +{
+ int ret;
+ struct rlimit_cgroup *rcg;
+

 +       rcu_read_lock();
 +       rcg = rlimit_cgroup_from_task(rcu_dereference(mm->owner));
 +       css_get(&rcg->css);
 +       rcu_read_unlock();
 +
 +       ret = res_counter_charge(&rcg->as_res, (nr_pages << PAGE_SHIFT));
 +       css_put(&rcg->css);
 +       return ret;

Quoted 1 line

+}

You need to synchronize against mm->owner changing, or mm->owner->cgroups changing. How about:

int rlimit_cgroup_charge_as(struct mm_struct *mm, unsigned long nr_pages) {
int ret;
struct rlimit_cgroup *rcg;
struct task_struct *owner;

rcu_read_lock();
again:

  /* Find and lock the owner of the mm */
  owner = rcu_dereference(mm->owner);
  task_lock(owner);
  if (mm->owner != owner) {
    task_unlock(owner);
    goto again;
  }

/* Charge the owner's cgroup with the new memory */ rcg = rlimit_cgroup_from_task(owner);
ret = res_counter_charge(&rcg->as_res, (nr_pages << PAGE_SHIFT)); task_unlock(owner);
rcu_read_unlock();
return ret;
}

Quoted 5 lines

+
+void rlimit_cgroup_uncharge_as(struct mm_struct *mm, unsigned long nr_pages) +{
+ struct rlimit_cgroup *rcg;
+

Quoted 8 lines

 +       rcu_read_lock();
 +       rcg = rlimit_cgroup_from_task(rcu_dereference(mm->owner));
 +       css_get(&rcg->css);
 +       rcu_read_unlock();

+
+ res_counter_uncharge(&rcg->as_res, (nr_pages << PAGE_SHIFT));
+ css_put(&rcg->css);
+}

Can't this be implemented as just a call to charge() with a negative value? (Possibly fixing res_counter_charge() to handle negative values if necessary) Seems simpler.

Quoted 3 lines

+/*
+ * TODO: get the attach callbacks to fail and disallow task movement.
+ */

You mean disallow all movement within a hierarchy that has this cgroup mounted? Doesn't that make it rather hard to use?

Quoted 22 lines

+static void rlimit_cgroup_move_task(struct cgroup_subsys *ss,

 +                                       struct cgroup *cgrp,
 +                                       struct cgroup *old_cgrp,
 +                                       struct task_struct *p)

+{
+ struct mm_struct *mm;
+ struct rlimit_cgroup *rcg, *old_rcg;
+
+ mm = get_task_mm(p);
+ if (mm == NULL)
+ return;
+
+ rcu_read_lock();
+ if (p != rcu_dereference(mm->owner))
+ goto out;
+
+ rcg = rlimit_cgroup_from_cgrp(cgrp);
+ old_rcg = rlimit_cgroup_from_cgrp(old_cgrp);
+
+ if (rcg == old_rcg)
+ goto out;
+

Quoted 8 lines

+ if (res_counter_charge(&rcg->as_res, (mm->total_vm << PAGE_SHIFT)))
+ goto out;
+ res_counter_uncharge(&old_rcg->as_res, (mm->total_vm << PAGE_SHIFT));
+out:
+ rcu_read_unlock();
+ mmput(mm);
+}
+

Since you need to protect against concurrent charges, and against concurrent mm ownership changes, I think you should just do this under task_lock(p).

Quoted 13 lines

+static void rlimit_cgroup_mm_owner_changed(struct cgroup_subsys *ss,

 +                                               struct cgroup *cgrp,
 +                                               struct cgroup *old_cgrp,
 +                                               struct task_struct *p)
 +{
 +       struct rlimit_cgroup *rcg, *old_rcg;
 +       struct mm_struct *mm = get_task_mm(p);
 +
 +       BUG_ON(!mm);
 +       rcg = rlimit_cgroup_from_cgrp(cgrp);
 +       old_rcg = rlimit_cgroup_from_cgrp(old_cgrp);
 +       if (res_counter_charge(&rcg->as_res, (mm->total_vm << PAGE_SHIFT)))

Quoted 6 lines

+ goto out;
+ res_counter_uncharge(&old_rcg->as_res, (mm->total_vm << PAGE_SHIFT));
+out:
+ mmput(mm);
+}
+

Also needs to task_lock(p) to prevent concurrent charges or cgroup reassignments?

Quoted 17 lines · Paul Menage

Quoted 15 lines · Balbir Singh

+
+int rlimit_cgroup_charge_as(struct mm_struct *mm, unsigned long nr_pages) +{
+ int ret;
+ struct rlimit_cgroup *rcg;
+

 +       rcu_read_lock();
 +       rcg = rlimit_cgroup_from_task(rcu_dereference(mm->owner));
 +       css_get(&rcg->css);
 +       rcu_read_unlock();
 +
 +       ret = res_counter_charge(&rcg->as_res, (nr_pages << PAGE_SHIFT));
 +       css_put(&rcg->css);
 +       return ret;

+}

Quoted 3 lines

You need to synchronize against mm->owner changing, or mm->owner->cgroups changing. How about:

My mind goes blank at times, so forgive me asking, what happens if we don't use task_lock(). mm->owner cannot be freed, even if it changes, we get the callback in mm_owner_changed(). The locations from where we call _charge and _uncharge, we know that the mm is not going to change either.

Quoted 2 lines

Consider the following scenario

We try to move task "t1" from cgroup "A" to cgroup "B".
Doing so, causes "B" to go over it's limit, what do we do? Ideally, we would like to be able to go back to cgroups and say, please fail attach, since that causes "B" to go over it's specified limit.

This patch adds support for accounting and control of virtual address space limits. The accounting is done via the rlimit_cgroup_(un)charge_as functions. The core of the accounting takes place during fork time in copy_process(), may_expand_vm(), remove_vma_list() and exit_mmap(). There are some special cases that are handled here as well (arch/ia64/kernel/perform.c, arch/x86/kernel/ptrace.c, insert_special_mapping())

Quoted 8 lines · Balbir Singh

This patch adds support for accounting and control of virtual address space limits. The accounting is done via the rlimit_cgroup_(un)charge_as functions. The core of the accounting takes place during fork time in copy_process(), may_expand_vm(), remove_vma_list() and exit_mmap(). There are some special cases that are handled here as well (arch/ia64/kernel/perform.c, arch/x86/kernel/ptrace.c, insert_special_mapping())

The basic idea of the patches looks fine (apart from some synchronization issues) but Is calling this the "rlimit" controller a great idea? That implies that it handles all (or at least many) of the things that setrlimit()/getrlimit() handle.

While some of the other rlimit things definitely do make sense as cgroup controllers, putting them all in the same controller doesn't really - paying for the address-space tracking overhead just to get, say, the equivalent of RLIMIT_NPROC (max tasks) isn't a great idea.

Can you instead give this a name that somehow refers to virtual address space limits, e.g. "va" or "as". That would still fit if you expanded it to deal with locked virtual address space limits too.

I think that an "rlimit" controller would probably be best for representing just those limits that don't really make sense when aggregated across different tasks, but apply separately to each task (e.g. RLIMIT_FSIZE, RLIMIT_CORE, RLIMIT_NICE, RLIMIT_NOFILE, RLIMIT_RTPRIO, RLIMIT_STACK, RLIMIT_SIGPENDING, and maybe RLIMIT_CPU), in order to provide an easy way to change these limits on a group of running tasks.

On a separate note for the address-space tracking, ideally the subsystem would track whether or not it was bound to a hierarchy, and skip charging/uncharging if not. That way there's no (noticeable) overhead for compiling in the subsystem but not using it. At the point when the subsystem was bound to a hierarchy, it could at that point run through all mms and charge each one's existing address space to the appropriate cgroup. (Currently that would only be the root cgroup in the hierarchy).

I currently intend to use this controller for controlling memory related rlimits, like address space and mlock'ed memory. How about we use something like "memrlimit"?

Good suggestion, but it will be hard if not impossible to account the data correctly as it changes, if we do the accounting/summation at bind time. We'll need a really big lock to do it, something I want to avoid. Did you have something else in mind?

Quoted 3 lines · Balbir Singh

I currently intend to use this controller for controlling memory related rlimits, like address space and mlock'ed memory. How about we use something like "memrlimit"?

Sounds reasonable.

Quoted 4 lines

Good suggestion, but it will be hard if not impossible to account the data correctly as it changes, if we do the accounting/summation at bind time. We'll need a really big lock to do it, something I want to avoid. Did you have something else in mind?

Yes, it'll be tricky but I think worthwhile. I believe it can be done without the charge/uncharge code needing to take a global lock, except for when we're actually binding/unbinding, with careful use of RCU.

This is the documentation patch. It describes the rlimit controller and how to build and use it.

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

---

 Documentation/controllers/rlimit.txt |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff -puN /dev/null Documentation/controllers/rlimit.txt

Documentation/controllers/rlimit.txt new file +8 -0

@@ -0,0 +1,29 @@
+This controller is enabled by the CONFIG_CGROUP_RLIMIT_CTLR option. Prior
+to reading this documentation please read Documentation/cgroups.txt and
+Documentation/controllers/memory.txt. Several of the principles of this
+controller are similar to the memory resource controller.
+
+This controller framework is designed to be extensible to control any
+resource limit (memory related) with little effort.
+

Quoted 22 lines · On Sun, 04 May 2008 03:08:25 +0530 Balbir Singh <[email protected]>…

This is the documentation patch. It describes the rlimit controller and how to build and use it.

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

---

 Documentation/controllers/rlimit.txt |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff -puN /dev/null Documentation/controllers/rlimit.txt

Documentation/controllers/rlimit.txt new file +8 -0

@@ -0,0 +1,29 @@
+This controller is enabled by the CONFIG_CGROUP_RLIMIT_CTLR option. Prior
+to reading this documentation please read Documentation/cgroups.txt and
+Documentation/controllers/memory.txt. Several of the principles of this
+controller are similar to the memory resource controller.
+
+This controller framework is designed to be extensible to control any
+resource limit (memory related) with little effort.
+

Documentation/controllers/rlimit.txt line 18

Quoted 10 lines

Documentation/controllers/rlimit.txt new file +10 -0

+This new controller, controls the address space expansion of the tasks
+belonging to a cgroup. Address space control is provided along the same lines as
+RLIMIT_AS control, which is available via getrlimit(2)/setrlimit(2).
+The interface for controlling address space is provided through
+"rlimit.limit_in_bytes". The file is similar to "limit_in_bytes" w.r.t. the user
+interface. Please see section 3 of the memory resource controller documentation
+for more details on how to use the user interface to get and set values.
+
+The "rlimit.usage_in_bytes" file provides information about the total address
+space usage of the tasks in the cgroup, in bytes.

Finally, with a bit of between-the-line reading, I begin to understand what this stuff is actually supposed to do.

It puts an upper limit upon the _total_ address-space size of all the mms which are contained within the resource group, yes?

(can am mm be shared by two threads whcih are in different resource groups, btw?)

Quoted 5 lines

Documentation/controllers/rlimit.txt new file +5 -0

+Advantages of providing this feature
+
+1. Control over virtual address space allows for a cgroup to fail gracefully
+   i.e., via a malloc or mmap failure as compared to OOM kill when no
+   pages can be reclaimed.

Documentation/controllers/rlimit.txt line 29

Quoted 5 lines

Documentation/controllers/rlimit.txt new file +5 -0

+2. It provides better control over how many pages can be swapped out when
+   the cgroup goes over its limit. A badly setup cgroup can cause excessive
+   swapping. Providing control over the address space allocations ensures
+   that the system administrator has control over the total swapping that
+   can take place.

Here's another missing piece: what is the kernel's behaviour when such a limit is increased? Seems that the sole option is a failure return from

mmap/brk/sbrk/etc, yes?

This should be spelled out in careful detail, please. This is a newly-proposed kernel<->userspace interface and we care about those very much.

Finally, I worry about overflows. afacit the sum-of-address-space-sizes-for-a-cgroup is accounted for in an unsigned long?

If so, a 32-bit machine could easily overflow it.

And a 64-bit machine could possibly do so with a bit of effort, perhaps? That's assuming that the code doesn't attempt to avoid duplicate accounting due to multiple-mms-mapping-the-same-pages, which afaict appears to be the case. (Then again, perhaps no machine will ever have the pagetable space to get that far).

Ho hum, I had to do rather a lot of guesswork here to try to understand your proposed overall design for this feature. I'd prefer to hear about your design via more direct means.

Quoted 3 lines

Ho hum, I had to do rather a lot of guesswork here to try to understand your proposed overall design for this feature. I'd prefer to hear about your design via more direct means.

Do you have any suggestions on how to do that better. Would you like documentation to be the first patch in the series? I had sent out two RFC's earlier and got comments and feedback from several people.

Quoted 3 lines · On Tue, 06 May 2008 11:09:52 +0530 Balbir Singh <[email protected]>…

Quoted 3 lines

Ho hum, I had to do rather a lot of guesswork here to try to understand your proposed overall design for this feature. I'd prefer to hear about your design via more direct means.

Quoted 4 lines

Do you have any suggestions on how to do that better. Would you like documentation to be the first patch in the series? I had sent out two RFC's earlier and got comments and feedback from several people.

I do like to see the overall what-i-am-setting-out-to-do description in there somewhere - sometimes a Docuemtation/ file is appropriate, other times do it via changelog.

But the first part of the review is reviewing whatever it is which you set out to achieve. Once that's understood and sounds like a good idea then we can start looking at how you did it.

This is the third version of the address space control patches. These patches are against 2.6.25-mm1 and have been tested using KVM in SMP mode, both with and without the config enabled.

The first patch adds the user interface. The second patch fixes the cgroup mm_owner_changed callback to pass the task struct, so that accounting can be adjusted on owner changes. The thrid patch adds accounting and control. The fourth patch updates documentation.

An earlier post of the patchset can be found at
http://lwn.net/Articles/275143/

This patch is built on top of the mm owner patches and utilizes that feature to virtually group tasks by mm_struct.

Reviews, Comments?

"+This controller framework is designed to be extensible to control any "+resource limit (memory related) with little effort." memory only ? Ok, all you want to do is related to memory, but someone may want to limit RLIMIT_CPU by group or RLIMIT_CORE by group or.... (I have no plan but they seems useful.;)

Quoted 5 lines

"+This controller framework is designed to be extensible to control any "+resource limit (memory related) with little effort." memory only ? Ok, all you want to do is related to memory, but someone may want to limit RLIMIT_CPU by group or RLIMIT_CORE by group or.... (I have no plan but they seems useful.;)

...RLIMIT_MEMLOCK is in my want-to-do-list ;)

Quoted 10 lines

This is the third version of the address space control patches. These patches are against 2.6.25-mm1 and have been tested using KVM in SMP mode, both with and without the config enabled.

The first patch adds the user interface. The second patch fixes the cgroup mm_owner_changed callback to pass the task struct, so that accounting can be adjusted on owner changes. The thrid patch adds accounting and control. The fourth patch updates documentation.

Quoted 8 lines

An earlier post of the patchset can be found at
http://lwn.net/Articles/275143/

This patch is built on top of the mm owner patches and utilizes that feature to virtually group tasks by mm_struct.

Reviews, Comments?

I can't read the whole patch deeply now but this new concept "rlimit-controlle r" seems make sense to me.

At quick glance, I have some thoughts.

1. kerner/rlimit_cgroup.c is better for future expansion.

2. why
"+This controller framework is designed to be extensible to control any "+resource limit (memory related) with little effort." memory only ? Ok, all you want to do is related to memory, but someone may want to limit RLIMIT_CPU by group or RLIMIT_CORE by group or.... (I have no plan but they seems useful.;)

So, could you add design hint of rlimit contoller to the documentation ?

3. Rleated to 2. Showing what kind of "rlimit" params are supported by cgroup will be good.

I don't think you have to implement all things at once. Staring from "only RLIMIT_AS is supported now" is good. Someone will expand it if he needs. But showing basic view of "gerenal purpose rlimit contoller" in _doc ument_ or _comments_ or _codes_ is a good thing to do.

If you don't want to provide RLIMIT feature other than address space, it's better to avoid using the name of RLIMIT. It's confusing.

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Quoted 3 lines

3. Rleated to 2. Showing what kind of "rlimit" params are supported by cgroup will be good.

Do you mean in init/Kconfig or documentation?. I should probably rename limit_in_bytes and usage_in_bytes to add an as_ prefix, so that the UI clearly shows what is supported as well.