Skip to contentSkip to conversation

memcg, cgroup: kill css id

PATCH Applied Li Zefan 4 participants 14 messages Sep 23, 2013 – Sep 24, 2013

Subsystems (3)
lore
Share & tools
lore

Li Zefan

author
#0 lore raw

v6 cover memcg, cgroup: kill css id

Hi Andrew,

The whole patchset has been acked and reviewed by Michal and Tejun. Could you merge it into mm tree?

===========

This patchset converts memcg to use cgroup->id, and then we remove cgroup css_id.

As we've removed memcg's own refcnt, converting memcg to use cgroup->id is very straight-forward.

v6:
- rebased against mmotm 2013-09-20-15-59
- moved cgroup id check from mem_cgroup_css_alloc() to mem_cgroup_css_online()

v5:
- rebased against mmotm 2013-08-07-16-55

v4:
- make cgroup_from_id() inline and check if cgroup_mutex is held.
- add a comment for idr_remove() in cgroup_offline)fn().

v2->v3:
- some minor cleanups suggested by Michal.
- fixed the call to idr_alloc() in cgroup_init() in the first patch.

Li Zefan (5):
memcg: convert to use cgroup_is_descendant()
memcg: convert to use cgroup id
memcg: fail to create cgroup if the cgroup id is too big
memcg: stop using css id
cgroup: kill css_id

 include/linux/cgroup.h |  37 --------
 kernel/cgroup.c        | 248 +------------------------------------------------
 mm/memcontrol.c        |  66 +++++++------
 3 files changed, 41 insertions(+), 310 deletions(-)
Signature · 7 lines
1.8.0.2

--
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>
author
#1 lore raw

PATCH v6 1/5 memcg: convert to use cgroup_is_descendant()

This is a preparation to kill css_id.

---
 mm/memcontrol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/memcontrol.c +1 -1
index 7dda769..9117249 100644
@@ -1405,7 +1405,7 @@ bool __mem_cgroup_same_or_subtree(const struct mem_cgroup *root_memcg,
 		return true;
 	if (!root_memcg->use_hierarchy || !memcg)
 		return false;
-	return css_is_ancestor(&memcg->css, &root_memcg->css);
+	return cgroup_is_descendant(memcg->css.cgroup, root_memcg->css.cgroup);
 }
 
 static bool mem_cgroup_same_or_subtree(const struct mem_cgroup *root_memcg,
Signature · 7 lines
1.8.0.2

--
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>
author
#2 lore raw

PATCH v6 2/5 memcg: convert to use cgroup id

In reply to Li Zefan

Use cgroup id instead of css id. This is a preparation to kill css id.

Note, as memcg treat 0 as an invalid id, while cgroup id starts with 0, we define memcg_id == cgroup_id + 1.

---
 mm/memcontrol.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)
mm/memcontrol.c +22 -10
index 9117249..6719e2c 100644
@@ -498,6 +498,23 @@ static inline bool mem_cgroup_is_root(struct mem_cgroup *memcg)
 	return (memcg == root_mem_cgroup);
 }
 
+static inline unsigned short mem_cgroup_id(struct mem_cgroup *memcg)
+{
+	/*
+	 * The ID of the root cgroup is 0, but memcg treat 0 as an
+	 * invalid ID, so we return (cgroup_id + 1).
+	 */
+	return memcg->css.cgroup->id + 1;
+}
+
+static inline struct mem_cgroup *mem_cgroup_from_id(unsigned short id)
+{
+	struct cgroup_subsys_state *css;
+
+	css = css_from_id(id - 1, &mem_cgroup_subsys);
+	return mem_cgroup_from_css(css);
+}
+
 /* Writing them here to avoid exposing memcg's inner layout */
 #if defined(CONFIG_INET) && defined(CONFIG_MEMCG_KMEM)
 
@@ -2850,15 +2867,10 @@ static void __mem_cgroup_cancel_local_charge(struct mem_cgroup *memcg,
  */
 static struct mem_cgroup *mem_cgroup_lookup(unsigned short id)
 {
-	struct cgroup_subsys_state *css;
-
 	/* ID 0 is unused ID */
 	if (!id)
 		return NULL;
-	css = css_lookup(&mem_cgroup_subsys, id);
-	if (!css)
-		return NULL;
-	return mem_cgroup_from_css(css);
+	return mem_cgroup_from_id(id);
 }
 
 struct mem_cgroup *try_get_mem_cgroup_from_page(struct page *page)
@@ -4368,7 +4380,7 @@ mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent, bool swapout)
 	 * css_get() was called in uncharge().
 	 */
 	if (do_swap_account && swapout && memcg)
-		swap_cgroup_record(ent, css_id(&memcg->css));
+		swap_cgroup_record(ent, mem_cgroup_id(memcg));
 }
 #endif
 
@@ -4420,8 +4432,8 @@ static int mem_cgroup_move_swap_account(swp_entry_t entry,
 {
 	unsigned short old_id, new_id;
 
-	old_id = css_id(&from->css);
-	new_id = css_id(&to->css);
+	old_id = mem_cgroup_id(from);
+	new_id = mem_cgroup_id(to);
 
 	if (swap_cgroup_cmpxchg(entry, old_id, new_id) == old_id) {
 		mem_cgroup_swap_statistics(from, false);
@@ -6571,7 +6583,7 @@ static enum mc_target_type get_mctgt_type(struct vm_area_struct *vma,
 	}
 	/* There is a swap entry and a page doesn't exist or isn't charged */
 	if (ent.val && !ret &&
-			css_id(&mc.from->css) == lookup_swap_cgroup_id(ent)) {
+	    mem_cgroup_id(mc.from) == lookup_swap_cgroup_id(ent)) {
 		ret = MC_TARGET_SWAP;
 		if (target)
 			target->ent = ent;
Signature · 7 lines
1.8.0.2

--
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>
author
#3 lore raw

PATCH v6 3/5 memcg: fail to create cgroup if the cgroup id is too big

In reply to Li Zefan

memcg requires the cgroup id to be smaller than 65536.

This is a preparation to kill css id.

---
 mm/memcontrol.c | 9 +++++++++
 1 file changed, 9 insertions(+)
mm/memcontrol.c +9 -0
index 6719e2c..4e40ebe 100644
@@ -498,6 +498,12 @@ static inline bool mem_cgroup_is_root(struct mem_cgroup *memcg)
 	return (memcg == root_mem_cgroup);
 }
 
+/*
+ * We restrict the id in the range of [1, 65535], so it can fit into
+ * an unsigned short.
+ */
+#define MEM_CGROUP_ID_MAX	USHRT_MAX
+
 static inline unsigned short mem_cgroup_id(struct mem_cgroup *memcg)
 {
 	/*
@@ -6312,6 +6318,9 @@ mem_cgroup_css_online(struct cgroup_subsys_state *css)
 	struct mem_cgroup *parent = mem_cgroup_from_css(css_parent(css));
 	int error = 0;
 
+	if (css->cgroup->id > MEM_CGROUP_ID_MAX)
+		return -ENOSPC;
+
 	if (!parent)
 		return 0;
 
Signature · 1 line
1.8.0.2
author
#4 lore raw

PATCH v6 4/5 memcg: stop using css id

In reply to Li Zefan

Now memcg uses cgroup id instead of css id. Update some comments and set mem_cgroup_subsys->use_id to 0.

---
 mm/memcontrol.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)
mm/memcontrol.c +8 -15
index 4e40ebe..32b2d33 100644
@@ -592,16 +592,11 @@ static void disarm_sock_keys(struct mem_cgroup *memcg)
 #ifdef CONFIG_MEMCG_KMEM
 /*
  * This will be the memcg's index in each cache's ->memcg_params->memcg_caches.
- * There are two main reasons for not using the css_id for this:
- *  1) this works better in sparse environments, where we have a lot of memcgs,
- *     but only a few kmem-limited. Or also, if we have, for instance, 200
- *     memcgs, and none but the 200th is kmem-limited, we'd have to have a
- *     200 entry array for that.
- *
- *  2) In order not to violate the cgroup API, we would like to do all memory
- *     allocation in ->create(). At that point, we haven't yet allocated the
- *     css_id. Having a separate index prevents us from messing with the cgroup
- *     core for this
+ * The main reason for not using cgroup id for this:
+ *  this works better in sparse environments, where we have a lot of memcgs,
+ *  but only a few kmem-limited. Or also, if we have, for instance, 200
+ *  memcgs, and none but the 200th is kmem-limited, we'd have to have a
+ *  200 entry array for that.
  *
  * The current size of the caches array is stored in
  * memcg_limited_groups_array_size.  It will double each time we have to
@@ -616,14 +611,14 @@ int memcg_limited_groups_array_size;
  * cgroups is a reasonable guess. In the future, it could be a parameter or
  * tunable, but that is strictly not necessary.
  *
- * MAX_SIZE should be as large as the number of css_ids. Ideally, we could get
+ * MAX_SIZE should be as large as the number of cgrp_ids. Ideally, we could get
  * this constant directly from cgroup, but it is understandable that this is
  * better kept as an internal representation in cgroup.c. In any case, the
- * css_id space is not getting any smaller, and we don't have to necessarily
+ * cgrp_id space is not getting any smaller, and we don't have to necessarily
  * increase ours as well if it increases.
  */
 #define MEMCG_CACHES_MIN_SIZE 4
-#define MEMCG_CACHES_MAX_SIZE 65535
+#define MEMCG_CACHES_MAX_SIZE MEM_CGROUP_ID_MAX
 
 /*
  * A lot of the calls to the cache allocation functions are expected to be
@@ -6215,7 +6210,6 @@ static void __mem_cgroup_free(struct mem_cgroup *memcg)
 	size_t size = memcg_size();
 
 	mem_cgroup_remove_from_trees(memcg);
-	free_css_id(&mem_cgroup_subsys, &memcg->css);
 
 	for_each_node(node)
 		free_mem_cgroup_per_zone_info(memcg, node);
@@ -7012,7 +7006,6 @@ struct cgroup_subsys mem_cgroup_subsys = {
 	.bind = mem_cgroup_bind,
 	.base_cftypes = mem_cgroup_files,
 	.early_init = 0,
-	.use_id = 1,
 };
 
 #ifdef CONFIG_MEMCG_SWAP
Signature · 1 line
1.8.0.2
author
#5 lore raw

PATCH v6 5/5 cgroup: kill css_id

In reply to Li Zefan

The only user of css_id was memcg, and it has been convered to use cgroup->id, so kill css_id.

---
 include/linux/cgroup.h |  37 --------
 kernel/cgroup.c        | 248 +------------------------------------------------
 2 files changed, 1 insertion(+), 284 deletions(-)
include/linux/cgroup.h +0 -37
index 3561d30..39c1d94 100644
@@ -612,11 +612,6 @@ struct cgroup_subsys {
 	int subsys_id;
 	int disabled;
 	int early_init;
-	/*
-	 * True if this subsys uses ID. ID is not available before cgroup_init()
-	 * (not available in early_init time.)
-	 */
-	bool use_id;
 
 	/*
 	 * If %false, this subsystem is properly hierarchical -
@@ -642,9 +637,6 @@ struct cgroup_subsys {
 	 */
 	struct cgroupfs_root *root;
 	struct list_head sibling;
-	/* used when use_id == true */
-	struct idr idr;
-	spinlock_t id_lock;
 
 	/* list of cftype_sets */
 	struct list_head cftsets;
@@ -875,35 +867,6 @@ int css_scan_tasks(struct cgroup_subsys_state *css,
 int cgroup_attach_task_all(struct task_struct *from, struct task_struct *);
 int cgroup_transfer_tasks(struct cgroup *to, struct cgroup *from);
 
-/*
- * CSS ID is ID for cgroup_subsys_state structs under subsys. This only works
- * if cgroup_subsys.use_id == true. It can be used for looking up and scanning.
- * CSS ID is assigned at cgroup allocation (create) automatically
- * and removed when subsys calls free_css_id() function. This is because
- * the lifetime of cgroup_subsys_state is subsys's matter.
- *
- * Looking up and scanning function should be called under rcu_read_lock().
- * Taking cgroup_mutex is not necessary for following calls.
- * But the css returned by this routine can be "not populated yet" or "being
- * destroyed". The caller should check css and cgroup's status.
- */
-
-/*
- * Typically Called at ->destroy(), or somewhere the subsys frees
- * cgroup_subsys_state.
- */
-void free_css_id(struct cgroup_subsys *ss, struct cgroup_subsys_state *css);
-
-/* Find a cgroup_subsys_state which has given ID */
-
-struct cgroup_subsys_state *css_lookup(struct cgroup_subsys *ss, int id);
-
-/* Returns true if root is ancestor of cg */
-bool css_is_ancestor(struct cgroup_subsys_state *cg,
-		     const struct cgroup_subsys_state *root);
-
-/* Get id and depth of css */
-unsigned short css_id(struct cgroup_subsys_state *css);
 struct cgroup_subsys_state *css_from_dir(struct dentry *dentry,
 					 struct cgroup_subsys *ss);
 
kernel/cgroup.c +1 -247

314 lines not on this page · raw message

Signature · 1 line
1.8.0.2
maintainer
#6 lore raw
In reply to Li Zefan

Hello,

Quoted 2 lines · Li Zefan

The whole patchset has been acked and reviewed by Michal and Tejun. Could you merge it into mm tree?

Ah... I really hoped that this had been merged during -rc1 window. Andrew, would it be okay to carry this series through cgroup tree? It doesn't really have much to do with mm proper and it's a PITA to have to keep updating css_id code from cgroup side when it's scheduled to go away. If carried in -mm, it's likely to cause conflicts with ongoing cgroup changes too.

Thanks.

Signature · 1 line
tejun
maintainer
#7 lore raw
Quoted 6 lines · Tejun Heo

Hello,

Quoted 2 lines · Li Zefan

The whole patchset has been acked and reviewed by Michal and Tejun. Could you merge it into mm tree?

Quoted 6 lines

Ah... I really hoped that this had been merged during -rc1 window. Andrew, would it be okay to carry this series through cgroup tree? It doesn't really have much to do with mm proper and it's a PITA to have to keep updating css_id code from cgroup side when it's scheduled to go away. If carried in -mm, it's likely to cause conflicts with ongoing cgroup changes too.

Also, wasn't this already in -mm during the last devel cycle? ISTR conflicts with it in -mm with other cgroup core changes. Is there any specific reason why this wasn't merged during the merge windw?

Thanks.

Signature · 1 line
tejun
author
#8 lore raw
Quoted 7 lines · Tejun Heo
Quoted 6 lines · Tejun Heo

Hello,

Quoted 2 lines · Li Zefan

The whole patchset has been acked and reviewed by Michal and Tejun. Could you merge it into mm tree?

Quoted 6 lines
Quoted 6 lines

Ah... I really hoped that this had been merged during -rc1 window. Andrew, would it be okay to carry this series through cgroup tree? It doesn't really have much to do with mm proper and it's a PITA to have to keep updating css_id code from cgroup side when it's scheduled to go away. If carried in -mm, it's likely to cause conflicts with ongoing cgroup changes too.

I would love to see this patchset go through cgroup tree. The changes to memcg is quite small, and as -mm tree is based on -next it won't cause future conflicts.

Quoted 5 lines

Also, wasn't this already in -mm during the last devel cycle? ISTR conflicts with it in -mm with other cgroup core changes. Is there any specific reason why this wasn't merged during the merge windw?

No, it never went into -mm tree... I guess it's because Andrew was too busy and overlooked this patchset?

--
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>

maintainer
#9 lore raw
Quoted 15 lines · On Tue, 24 Sep 2013 08:32:03 +0800 Li Zefan <[email protected]> wrote:
Quoted 7 lines · Tejun Heo
Quoted 6 lines · Tejun Heo

Hello,

Quoted 2 lines · Li Zefan

The whole patchset has been acked and reviewed by Michal and Tejun. Could you merge it into mm tree?

Quoted 6 lines
Quoted 6 lines

Ah... I really hoped that this had been merged during -rc1 window. Andrew, would it be okay to carry this series through cgroup tree? It doesn't really have much to do with mm proper and it's a PITA to have to keep updating css_id code from cgroup side when it's scheduled to go away. If carried in -mm, it's likely to cause conflicts with ongoing cgroup changes too.

Quoted 2 lines

I would love to see this patchset go through cgroup tree. The changes to memcg is quite small,

It seems logical to put this in the cgroup tree as that's where most of the impact occurs.

Quoted 2 lines

and as -mm tree is based on -next it won't cause
future conflicts.

That's no longer the case - I'm staging -mm patches ahead of linux-next now. Except in cases where that's impractical, such as the 3.12 memcg changes which were pretty heavily impacted by cgroups tree changes.

Quoted 8 lines
Quoted 5 lines

Also, wasn't this already in -mm during the last devel cycle? ISTR conflicts with it in -mm with other cgroup core changes. Is there any specific reason why this wasn't merged during the merge windw?

No, it never went into -mm tree... I guess it's because Andrew was too busy and overlooked this patchset?

I'm not sure what happened to the August 7 patchset, actually. I don't often overlook stuff - I'll skip things if the timing is terrible or if the review comments indicate that another version is coming. But none of that seems to be the case here. hmm...

--
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>

maintainer
#10 lore raw

Hello, Andrew.

Quoted 5 lines · Andrew Morton
Quoted 2 lines

I would love to see this patchset go through cgroup tree. The changes to memcg is quite small,

It seems logical to put this in the cgroup tree as that's where most of the impact occurs.

Cool, applying the changes to cgroup/for-3.13.

Quoted 6 lines
Quoted 2 lines

and as -mm tree is based on -next it won't cause
future conflicts.

That's no longer the case - I'm staging -mm patches ahead of linux-next now. Except in cases where that's impractical, such as the 3.12 memcg changes which were pretty heavily impacted by cgroups tree changes.

Please note that cgroup is likely to continue to go through a lot of changes for the foreseeable future and memcg is likely to be affected heavily.

Thanks.

Signature · 7 lines
tejun

--
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>
maintainer
#11 lore raw

(cc'ing Stephen, hi!)

Quoted 10 lines · Tejun Heo

Hello, Andrew.

Quoted 5 lines · Andrew Morton
Quoted 2 lines

I would love to see this patchset go through cgroup tree. The changes to memcg is quite small,

It seems logical to put this in the cgroup tree as that's where most of the impact occurs.

Cool, applying the changes to cgroup/for-3.13.

Stephen, Andrew, cgroup/for-3.13 will cause a minor conflict in mm/memcontrol.c with the patch which reverts Michal's reclaim changes.

static void __mem_cgroup_free(struct mem_cgroup *memcg)

  {
	  int node;
	  size_t size = memcg_size();

<<<<<<< HEAD
=======

	  mem_cgroup_remove_from_trees(memcg);
	  free_css_id(&mem_cgroup_subsys, &memcg->css);
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

1fa8f71dfa6e28c89afad7ac71dcb19b8c8da8b7

	  for_each_node(node)
		  free_mem_cgroup_per_zone_info(memcg, node);

It's a context conflict and just removing free_css_id() call resolves it.

static void __mem_cgroup_free(struct mem_cgroup *memcg)

  {
	  int node;
	  size_t size = memcg_size();

	  mem_cgroup_remove_from_trees(memcg);

	  for_each_node(node)
		  free_mem_cgroup_per_zone_info(memcg, node);

Thanks.

Signature · 7 lines
tejun

--
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>
#12 lore raw

Hi Tejun,

Quoted 2 lines · On Mon, 23 Sep 2013 21:52:11 -0400 Tejun Heo…

(cc'ing Stephen, hi!)

Hi :-)

Quoted 32 lines · Tejun Heo
Quoted 9 lines · Andrew Morton
Quoted 5 lines
Quoted 2 lines

I would love to see this patchset go through cgroup tree. The changes to memcg is quite small,

It seems logical to put this in the cgroup tree as that's where most of the impact occurs.

Cool, applying the changes to cgroup/for-3.13.

Stephen, Andrew, cgroup/for-3.13 will cause a minor conflict in mm/memcontrol.c with the patch which reverts Michal's reclaim changes.

static void __mem_cgroup_free(struct mem_cgroup *memcg)

  {
	  int node;
	  size_t size = memcg_size();

<<<<<<< HEAD
=======

	  mem_cgroup_remove_from_trees(memcg);
	  free_css_id(&mem_cgroup_subsys, &memcg->css);
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

1fa8f71dfa6e28c89afad7ac71dcb19b8c8da8b7

	  for_each_node(node)
		  free_mem_cgroup_per_zone_info(memcg, node);

It's a context conflict and just removing free_css_id() call resolves it.

static void __mem_cgroup_free(struct mem_cgroup *memcg)

Quoted 8 lines
  {
	  int node;
	  size_t size = memcg_size();

	  mem_cgroup_remove_from_trees(memcg);

	  for_each_node(node)
		  free_mem_cgroup_per_zone_info(memcg, node);

Thanks for the heads up, I guess I'll see that tomorrow.

Signature · 2 lines
Cheers,
Stephen Rothwell                    [email protected]
maintainer Applied
#13 lore raw
In reply to Li Zefan
Quoted 9 lines · Li Zefan

Hi Andrew,

The whole patchset has been acked and reviewed by Michal and Tejun. Could you merge it into mm tree?

===========

This patchset converts memcg to use cgroup->id, and then we remove cgroup css_id.

Applied to cgroup/for-3.13.

Thanks.

Signature · 7 lines
tejun

--
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>