kmemcg: get rid of __GFP_KMEMCG

We don't track any random page allocation, so we shouldn't track kmalloc that falls back to the page allocator.

Signed-off-by Vladimir Davydov <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Glauber Costa <[email protected]>

Cc Christoph Lameter <[email protected]>

Cc Pekka Enberg <[email protected]>

---
 include/linux/slab.h |    2 +-
 mm/memcontrol.c      |   27 +--------------------------
 mm/slub.c            |    4 ++--
 3 files changed, 4 insertions(+), 29 deletions(-)

include/linux/slab.h +1 -1

index 3dd389aa91c7..8a928ff71d93 100644
@@ -363,7 +363,7 @@ kmalloc_order(size_t size, gfp_t flags, unsigned int order)
 {
 	void *ret;
 
-	flags |= (__GFP_COMP | __GFP_KMEMCG);
+	flags |= __GFP_COMP;
 	ret = (void *) __get_free_pages(flags, order);
 	kmemleak_alloc(ret, size, 1, flags);
 	return ret;

mm/memcontrol.c +1 -26

index b4b6aef562fa..81a162d01d4d 100644
@@ -3528,35 +3528,10 @@ __memcg_kmem_newpage_charge(gfp_t gfp, struct mem_cgroup **_memcg, int order)
 
 	*_memcg = NULL;
 
-	/*
-	 * Disabling accounting is only relevant for some specific memcg
-	 * internal allocations. Therefore we would initially not have such
-	 * check here, since direct calls to the page allocator that are marked
-	 * with GFP_KMEMCG only happen outside memcg core. We are mostly
-	 * concerned with cache allocations, and by having this test at
-	 * memcg_kmem_get_cache, we are already able to relay the allocation to
-	 * the root cache and bypass the memcg cache altogether.
-	 *
-	 * There is one exception, though: the SLUB allocator does not create
-	 * large order caches, but rather service large kmallocs directly from
-	 * the page allocator. Therefore, the following sequence when backed by
-	 * the SLUB allocator:
-	 *
-	 *	memcg_stop_kmem_account();
-	 *	kmalloc(<large_number>)
-	 *	memcg_resume_kmem_account();
-	 *
-	 * would effectively ignore the fact that we should skip accounting,
-	 * since it will drive us directly to this function without passing
-	 * through the cache selector memcg_kmem_get_cache. Such large
-	 * allocations are extremely rare but can happen, for instance, for the
-	 * cache arrays. We bring this test here.
-	 */
-	if (!current->mm || current->memcg_kmem_skip_account)
+	if (!current->mm)
 		return true;
 
 	memcg = get_mem_cgroup_from_mm(current->mm);
-
 	if (!memcg_can_account_kmem(memcg)) {
 		css_put(&memcg->css);
 		return true;

mm/slub.c +2 -2

index 5e234f1f8853..c2e58a787443 100644
@@ -3325,7 +3325,7 @@ static void *kmalloc_large_node(size_t size, gfp_t flags, int node)
 	struct page *page;
 	void *ptr = NULL;
 
-	flags |= __GFP_COMP | __GFP_NOTRACK | __GFP_KMEMCG;
+	flags |= __GFP_COMP | __GFP_NOTRACK;
 	page = alloc_pages_node(node, flags, get_order(size));
 	if (page)
 		ptr = page_address(page);

@@ -3395,7 +3395,7 @@ void kfree(const void *x)
 	if (unlikely(!PageSlab(page))) {
 		BUG_ON(!PageCompound(page));
 		kfree_hook(x);
-		__free_memcg_kmem_pages(page, compound_order(page));
+		__free_pages(page, compound_order(page));
 		return;
 	}
 	slab_free(page->slab_cache, page, object, _RET_IP_);

Signature · 1 line

1.7.10.4

Quoted 2 lines · Vladimir Davydov

We don't track any random page allocation, so we shouldn't track kmalloc that falls back to the page allocator.

Why did we do that in the first place? d79923fad95b (sl[au]b: allocate objects from memcg cache) didn't tell me much.

How is memcg_kmem_skip_account removal related?

Hi Michal,

Quoted 5 lines · Michal Hocko

Quoted 2 lines · Vladimir Davydov

We don't track any random page allocation, so we shouldn't track kmalloc that falls back to the page allocator.

Why did we do that in the first place? d79923fad95b (sl[au]b: allocate objects from memcg cache) didn't tell me much.

I don't know, we'd better ask Glauber about that.

Quoted 1 line

How is memcg_kmem_skip_account removal related?

The comment this patch removes along with the memcg_kmem_skip_account check explains that pretty well IMO. In short, we only use memcg_kmem_skip_account to prevent kmalloc's from charging, which is crucial for recursion-avoidance in memcg_kmem_get_cache. Since we don't charge pages allocated from a root (not per-memcg) cache, from the first glance it would be enough to check for memcg_kmem_skip_account only in memcg_kmem_get_cache and return the root cache if it's set. However, for we can also kmalloc w/o issuing memcg_kmem_get_cache (kmalloc_large), we also need this check in memcg_kmem_newpage_charge. This patch removes kmalloc_large accounting, so we don't need this check anymore.

We don't track any random page allocation, so we shouldn't track kmalloc that falls back to the page allocator.

Signed-off-by Vladimir Davydov <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Glauber Costa <[email protected]>

Cc Christoph Lameter <[email protected]>

Cc Pekka Enberg <[email protected]>

---
 include/linux/slab.h |    2 +-
 mm/memcontrol.c      |   27 +--------------------------
 mm/slub.c            |    4 ++--
 3 files changed, 4 insertions(+), 29 deletions(-)

include/linux/slab.h +1 -1

index 3dd389aa91c7..8a928ff71d93 100644
@@ -363,7 +363,7 @@ kmalloc_order(size_t size, gfp_t flags, unsigned int order)
 {
 	void *ret;
 
-	flags |= (__GFP_COMP | __GFP_KMEMCG);
+	flags |= __GFP_COMP;
 	ret = (void *) __get_free_pages(flags, order);
 	kmemleak_alloc(ret, size, 1, flags);
 	return ret;

mm/memcontrol.c +1 -26

index b4b6aef562fa..81a162d01d4d 100644
@@ -3528,35 +3528,10 @@ __memcg_kmem_newpage_charge(gfp_t gfp, struct mem_cgroup **_memcg, int order)
 
 	*_memcg = NULL;
 
-	/*
-	 * Disabling accounting is only relevant for some specific memcg
-	 * internal allocations. Therefore we would initially not have such
-	 * check here, since direct calls to the page allocator that are marked
-	 * with GFP_KMEMCG only happen outside memcg core. We are mostly
-	 * concerned with cache allocations, and by having this test at
-	 * memcg_kmem_get_cache, we are already able to relay the allocation to
-	 * the root cache and bypass the memcg cache altogether.
-	 *
-	 * There is one exception, though: the SLUB allocator does not create
-	 * large order caches, but rather service large kmallocs directly from
-	 * the page allocator. Therefore, the following sequence when backed by
-	 * the SLUB allocator:
-	 *
-	 *	memcg_stop_kmem_account();
-	 *	kmalloc(<large_number>)
-	 *	memcg_resume_kmem_account();
-	 *
-	 * would effectively ignore the fact that we should skip accounting,
-	 * since it will drive us directly to this function without passing
-	 * through the cache selector memcg_kmem_get_cache. Such large
-	 * allocations are extremely rare but can happen, for instance, for the
-	 * cache arrays. We bring this test here.
-	 */
-	if (!current->mm || current->memcg_kmem_skip_account)
+	if (!current->mm)
 		return true;
 
 	memcg = get_mem_cgroup_from_mm(current->mm);
-
 	if (!memcg_can_account_kmem(memcg)) {
 		css_put(&memcg->css);
 		return true;

mm/slub.c +2 -2

index 5e234f1f8853..c2e58a787443 100644
@@ -3325,7 +3325,7 @@ static void *kmalloc_large_node(size_t size, gfp_t flags, int node)
 	struct page *page;
 	void *ptr = NULL;
 
-	flags |= __GFP_COMP | __GFP_NOTRACK | __GFP_KMEMCG;
+	flags |= __GFP_COMP | __GFP_NOTRACK;
 	page = alloc_pages_node(node, flags, get_order(size));
 	if (page)
 		ptr = page_address(page);

@@ -3395,7 +3395,7 @@ void kfree(const void *x)
 	if (unlikely(!PageSlab(page))) {
 		BUG_ON(!PageCompound(page));
 		kfree_hook(x);
-		__free_memcg_kmem_pages(page, compound_order(page));
+		__free_pages(page, compound_order(page));
 		return;
 	}
 	slab_free(page->slab_cache, page, object, _RET_IP_);

Signature · 1 line

1.7.10.4

Quoted 2 lines · Vladimir Davydov

We don't track any random page allocation, so we shouldn't track kmalloc that falls back to the page allocator.

This seems like a change which will leads to confusing (and arguably improper) kernel behavior. I prefer the behavior prior to this patch.

Before this change both of the following allocations are charged to memcg (assuming kmem accounting is enabled):
a = kmalloc(KMALLOC_MAX_CACHE_SIZE, GFP_KERNEL)
b = kmalloc(KMALLOC_MAX_CACHE_SIZE + 1, GFP_KERNEL)

After this change only 'a' is charged; 'b' goes directly to page allocator which no longer does accounting.

Hi Greg,

Quoted 12 lines · Greg Thelen

Quoted 2 lines · Vladimir Davydov

We don't track any random page allocation, so we shouldn't track kmalloc that falls back to the page allocator.

This seems like a change which will leads to confusing (and arguably improper) kernel behavior. I prefer the behavior prior to this patch.

Before this change both of the following allocations are charged to memcg (assuming kmem accounting is enabled):
a = kmalloc(KMALLOC_MAX_CACHE_SIZE, GFP_KERNEL)
b = kmalloc(KMALLOC_MAX_CACHE_SIZE + 1, GFP_KERNEL)

Quoted 2 lines

After this change only 'a' is charged; 'b' goes directly to page allocator which no longer does accounting.

Why do we need to charge 'b' in the first place? Can the userspace trigger such allocations massively? If there can only be one or two such allocations from a cgroup, is there any point in charging them?

In fact, do we actually need to charge every random kmem allocation? I guess not. For instance, filesystems often allocate data shared among all the FS users. It's wrong to charge such allocations to a particular memcg, IMO. That said the next step is going to be adding a per kmem cache flag specifying if allocations from this cache should be charged so that accounting will work only for those caches that are marked so explicitly.

There is one more argument for removing kmalloc_large accounting - we don't have an easy way to track such allocations, which prevents us from reparenting kmemcg charges on css offline. Of course, we could link kmalloc_large pages in some sort of per-memcg list which would allow us to find them on css offline, but I don't think such a complication is justified.

Quoted 6 lines · Vladimir Davydov

Quoted 5 lines · Greg Thelen

Before this change both of the following allocations are charged to memcg (assuming kmem accounting is enabled):
a = kmalloc(KMALLOC_MAX_CACHE_SIZE, GFP_KERNEL)
b = kmalloc(KMALLOC_MAX_CACHE_SIZE + 1, GFP_KERNEL)

Quoted 6 lines

Quoted 2 lines

After this change only 'a' is charged; 'b' goes directly to page allocator which no longer does accounting.

Why do we need to charge 'b' in the first place? Can the userspace trigger such allocations massively? If there can only be one or two such allocations from a cgroup, is there any point in charging them?

Of the top of my head I don't know of any >8KIB kmalloc()s so I can't say if they're directly triggerable by user space en masse. But we recently ran into some order:3 allocations in networking. The networking allocations used a non-generic kmem_cache (rather than kmalloc which started this discussion). For details, see ed98df3361f0 ("net: use __GFP_NORETRY for high order allocations"). I can't say if such allocations exist in device drivers, but given the networking example, it's conceivable that they may (or will) exist.

With slab this isn't a problem because sla has kmalloc kmem_caches for all supported allocation sizes. However, slub shows this issue for any kmalloc() allocations larger than 8KIB (at least on x86_64). It seems like a strange directly to take kmem accounting to say that kmalloc allocations are kmem limited, but only if they are either less than a threshold size or done with slab. Simply increasing the size of a data structure doesn't seem like it should automatically cause the memory to become exempt from kmem limits.

Quoted 7 lines

In fact, do we actually need to charge every random kmem allocation? I guess not. For instance, filesystems often allocate data shared among all the FS users. It's wrong to charge such allocations to a particular memcg, IMO. That said the next step is going to be adding a per kmem cache flag specifying if allocations from this cache should be charged so that accounting will work only for those caches that are marked so explicitly.

It's a question of what direction to approach kmem slab accounting
from: either opt-out (as the code currently is), or opt-in (with per kmem_cache flags as you suggest). I agree that some structures end up being shared (e.g. filesystem block bit map structures). In an opt-out system these are charged to a memcg initially and remain charged there until the memcg is deleted at which point the shared objects are reparented to a shared location. While this isn't perfect, it's unclear if it's better or worse than analyzing each class of allocation and deciding if they should be opt'd-in. One could (though I'm not) make the case that even dentries are easily shareable between containers and thus shouldn't be accounted to a single memcg. But given user space's ability to DoS a machine with dentires, they should be accounted.

Quoted 6 lines

There is one more argument for removing kmalloc_large accounting - we don't have an easy way to track such allocations, which prevents us from reparenting kmemcg charges on css offline. Of course, we could link kmalloc_large pages in some sort of per-memcg list which would allow us to find them on css offline, but I don't think such a complication is justified.

I assume that reparenting of such non kmem_cache allocations (e.g. large kmalloc) is difficult because such pages refer to the memcg, which we're trying to delete and the memcg has no index of such pages.
If such zombie memcg are undesirable, then an alternative to indexing
the pages is to define a kmem context object which such large pages point to. The kmem context would be reparented without needing to adjust the individual large pages. But there are plenty of options.

In fact, do we actually need to charge every random kmem allocation? I guess not. For instance, filesystems often allocate data shared among all the FS users. It's wrong to charge such allocations to a particular memcg, IMO. That said the next step is going to be adding a per kmem cache flag specifying if allocations from this cache should be charged so that accounting will work only for those caches that are marked so explicitly.

On Thu 27-03-14 11:37:11, Vladimir Davydov wrote:
[...]

Quoted 7 lines

In fact, do we actually need to charge every random kmem allocation? I guess not. For instance, filesystems often allocate data shared among all the FS users. It's wrong to charge such allocations to a particular memcg, IMO. That said the next step is going to be adding a per kmem cache flag specifying if allocations from this cache should be charged so that accounting will work only for those caches that are marked so explicitly.

How do you select which caches to track?

We have only a few places where we actually want to charge kmem so instead of intruding into the general page allocation path with __GFP_KMEMCG it's better to explictly charge kmem there. All kmem charges will be easier to follow that way.

This is a step towards removing __GFP_KMEMCG. It removes __GFP_KMEMCG from memcg caches' allocflags. Instead it makes slab allocation path call memcg_charge_kmem directly getting memcg to charge from the cache's memcg params.

Signed-off-by Vladimir Davydov <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Glauber Costa <[email protected]>

Cc Christoph Lameter <[email protected]>

Cc Pekka Enberg <[email protected]>

---
 include/linux/memcontrol.h |   24 +++++++++++++-----------
 mm/memcontrol.c            |   15 +++++++++++++++
 mm/slab.c                  |    7 ++++++-
 mm/slab_common.c           |    6 +-----
 mm/slub.c                  |   24 +++++++++++++++++-------
 5 files changed, 52 insertions(+), 24 deletions(-)

include/linux/memcontrol.h +13 -11

index e9dfcdad24c5..b8aaecc25cbf 100644
@@ -512,6 +512,9 @@ void memcg_update_array_size(int num_groups);
 struct kmem_cache *
 __memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp);
 
+int memcg_charge_slab(struct kmem_cache *s, gfp_t gfp, int order);
+void memcg_uncharge_slab(struct kmem_cache *s, int order);
+
 void mem_cgroup_destroy_cache(struct kmem_cache *cachep);
 int __kmem_cache_destroy_memcg_children(struct kmem_cache *s);
 

@@ -589,17 +592,7 @@ memcg_kmem_commit_charge(struct page *page, struct mem_cgroup *memcg, int order)
  * @cachep: the original global kmem cache
  * @gfp: allocation flags.
  *
- * This function assumes that the task allocating, which determines the memcg
- * in the page allocator, belongs to the same cgroup throughout the whole
- * process.  Misacounting can happen if the task calls memcg_kmem_get_cache()
- * while belonging to a cgroup, and later on changes. This is considered
- * acceptable, and should only happen upon task migration.
- *
- * Before the cache is created by the memcg core, there is also a possible
- * imbalance: the task belongs to a memcg, but the cache being allocated from
- * is the global cache, since the child cache is not yet guaranteed to be
- * ready. This case is also fine, since in this case the GFP_KMEMCG will not be
- * passed and the page allocator will not attempt any cgroup accounting.
+ * All memory allocated from a per-memcg cache is charged to the owner memcg.
  */
 static __always_inline struct kmem_cache *
 memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp)

@@ -667,6 +660,15 @@ memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp)
 {
 	return cachep;
 }
+
+static inline int memcg_charge_slab(struct kmem_cache *s, gfp_t gfp, int order)
+{
+	return 0;
+}
+
+static inline void memcg_uncharge_slab(struct kmem_cache *s, int order)
+{
+}
 #endif /* CONFIG_MEMCG_KMEM */
 #endif /* _LINUX_MEMCONTROL_H */
 

mm/memcontrol.c +15 -0

index 81a162d01d4d..9bbc088e3107 100644
@@ -3506,6 +3506,21 @@ out:
 }
 EXPORT_SYMBOL(__memcg_kmem_get_cache);
 
+int memcg_charge_slab(struct kmem_cache *s, gfp_t gfp, int order)
+{
+	if (is_root_cache(s))
+		return 0;
+	return memcg_charge_kmem(s->memcg_params->memcg, gfp,
+				 PAGE_SIZE << order);
+}
+
+void memcg_uncharge_slab(struct kmem_cache *s, int order)
+{
+	if (is_root_cache(s))
+		return;
+	memcg_uncharge_kmem(s->memcg_params->memcg, PAGE_SIZE << order);
+}
+
 /*
  * We need to verify if the allocation against current->mm->owner's memcg is
  * possible for the given order. But the page is not allocated yet, so we'll

mm/slab.c +6 -1

index eebc619ae33c..af126a37dafd 100644
@@ -1664,8 +1664,12 @@ static struct page *kmem_getpages(struct kmem_cache *cachep, gfp_t flags,
 	if (cachep->flags & SLAB_RECLAIM_ACCOUNT)
 		flags |= __GFP_RECLAIMABLE;
 
+	if (memcg_charge_slab(cachep, flags, cachep->gfporder))
+		return NULL;
+
 	page = alloc_pages_exact_node(nodeid, flags | __GFP_NOTRACK, cachep->gfporder);
 	if (!page) {
+		memcg_uncharge_slab(cachep, cachep->gfporder);
 		if (!(flags & __GFP_NOWARN) && printk_ratelimit())
 			slab_out_of_memory(cachep, flags, nodeid);
 		return NULL;

@@ -1724,7 +1728,8 @@ static void kmem_freepages(struct kmem_cache *cachep, struct page *page)
 	memcg_release_pages(cachep, cachep->gfporder);
 	if (current->reclaim_state)
 		current->reclaim_state->reclaimed_slab += nr_freed;
-	__free_memcg_kmem_pages(page, cachep->gfporder);
+	__free_pages(page, cachep->gfporder);
+	memcg_uncharge_slab(cachep, cachep->gfporder);
 }
 
 static void kmem_rcu_free(struct rcu_head *head)

mm/slab_common.c +1 -5

index f3cfccf76dda..6673597ac967 100644
@@ -290,12 +290,8 @@ void kmem_cache_create_memcg(struct mem_cgroup *memcg, struct kmem_cache *root_c
 				 root_cache->size, root_cache->align,
 				 root_cache->flags, root_cache->ctor,
 				 memcg, root_cache);
-	if (IS_ERR(s)) {
+	if (IS_ERR(s))
 		kfree(cache_name);
-		goto out_unlock;
-	}
-
-	s->allocflags |= __GFP_KMEMCG;
 
 out_unlock:
 	mutex_unlock(&slab_mutex);

mm/slub.c +17 -7

index c2e58a787443..6fefe3b33ce0 100644
@@ -1317,17 +1317,26 @@ static inline void slab_free_hook(struct kmem_cache *s, void *x)
 /*
  * Slab allocation and freeing
  */
-static inline struct page *alloc_slab_page(gfp_t flags, int node,
-					struct kmem_cache_order_objects oo)
+static inline struct page *alloc_slab_page(struct kmem_cache *s,
+		gfp_t flags, int node, struct kmem_cache_order_objects oo)
 {
+	struct page *page;
 	int order = oo_order(oo);
 
 	flags |= __GFP_NOTRACK;
 
+	if (memcg_charge_slab(s, flags, order))
+		return NULL;
+
 	if (node == NUMA_NO_NODE)
-		return alloc_pages(flags, order);
+		page = alloc_pages(flags, order);
 	else
-		return alloc_pages_exact_node(node, flags, order);
+		page = alloc_pages_exact_node(node, flags, order);
+
+	if (!page)
+		memcg_uncharge_slab(s, order);
+
+	return page;
 }
 
 static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)

@@ -1349,7 +1358,7 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
 	 */
 	alloc_gfp = (flags | __GFP_NOWARN | __GFP_NORETRY) & ~__GFP_NOFAIL;
 
-	page = alloc_slab_page(alloc_gfp, node, oo);
+	page = alloc_slab_page(s, alloc_gfp, node, oo);
 	if (unlikely(!page)) {
 		oo = s->min;
 		alloc_gfp = flags;

@@ -1357,7 +1366,7 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
 		 * Allocation may have failed due to fragmentation.
 		 * Try a lower order alloc if possible
 		 */
-		page = alloc_slab_page(alloc_gfp, node, oo);
+		page = alloc_slab_page(s, alloc_gfp, node, oo);
 
 		if (page)
 			stat(s, ORDER_FALLBACK);

@@ -1473,7 +1482,8 @@ static void __free_slab(struct kmem_cache *s, struct page *page)
 	page_mapcount_reset(page);
 	if (current->reclaim_state)
 		current->reclaim_state->reclaimed_slab += pages;
-	__free_memcg_kmem_pages(page, order);
+	__free_pages(page, order);
+	memcg_uncharge_slab(s, order);
 }
 
 #define need_reserve_slab_rcu						\

Signature · 1 line

1.7.10.4

Quoted 5 lines · Vladimir Davydov

We have only a few places where we actually want to charge kmem so instead of intruding into the general page allocation path with __GFP_KMEMCG it's better to explictly charge kmem there. All kmem charges will be easier to follow that way.

Quoted 4 lines

This is a step towards removing __GFP_KMEMCG. It removes __GFP_KMEMCG from memcg caches' allocflags. Instead it makes slab allocation path call memcg_charge_kmem directly getting memcg to charge from the cache's memcg params.

Yes, removing __GFP_KMEMCG is definitely a good step. I am currently at a conference and do not have much time to review this properly (even worse will be on vacation for the next 2 weeks) but where did all the static_key optimization go? What am I missing.

Quoted 196 lines

Signed-off-by Vladimir Davydov <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Glauber Costa <[email protected]>

Cc Christoph Lameter <[email protected]>

Cc Pekka Enberg <[email protected]>

---
 include/linux/memcontrol.h |   24 +++++++++++++-----------
 mm/memcontrol.c            |   15 +++++++++++++++
 mm/slab.c                  |    7 ++++++-
 mm/slab_common.c           |    6 +-----
 mm/slub.c                  |   24 +++++++++++++++++-------
 5 files changed, 52 insertions(+), 24 deletions(-)

include/linux/memcontrol.h +13 -11

index e9dfcdad24c5..b8aaecc25cbf 100644
@@ -512,6 +512,9 @@ void memcg_update_array_size(int num_groups);
 struct kmem_cache *
 __memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp);
 
+int memcg_charge_slab(struct kmem_cache *s, gfp_t gfp, int order);
+void memcg_uncharge_slab(struct kmem_cache *s, int order);
+
 void mem_cgroup_destroy_cache(struct kmem_cache *cachep);
 int __kmem_cache_destroy_memcg_children(struct kmem_cache *s);
 

@@ -589,17 +592,7 @@ memcg_kmem_commit_charge(struct page *page, struct mem_cgroup *memcg, int order)
  * @cachep: the original global kmem cache
  * @gfp: allocation flags.
  *
- * This function assumes that the task allocating, which determines the memcg
- * in the page allocator, belongs to the same cgroup throughout the whole
- * process.  Misacounting can happen if the task calls memcg_kmem_get_cache()
- * while belonging to a cgroup, and later on changes. This is considered
- * acceptable, and should only happen upon task migration.
- *
- * Before the cache is created by the memcg core, there is also a possible
- * imbalance: the task belongs to a memcg, but the cache being allocated from
- * is the global cache, since the child cache is not yet guaranteed to be
- * ready. This case is also fine, since in this case the GFP_KMEMCG will not be
- * passed and the page allocator will not attempt any cgroup accounting.
+ * All memory allocated from a per-memcg cache is charged to the owner memcg.
  */
 static __always_inline struct kmem_cache *
 memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp)

@@ -667,6 +660,15 @@ memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp)
 {
 	return cachep;
 }
+
+static inline int memcg_charge_slab(struct kmem_cache *s, gfp_t gfp, int order)
+{
+	return 0;
+}
+
+static inline void memcg_uncharge_slab(struct kmem_cache *s, int order)
+{
+}
 #endif /* CONFIG_MEMCG_KMEM */
 #endif /* _LINUX_MEMCONTROL_H */
 

mm/memcontrol.c +15 -0

index 81a162d01d4d..9bbc088e3107 100644
@@ -3506,6 +3506,21 @@ out:
 }
 EXPORT_SYMBOL(__memcg_kmem_get_cache);
 
+int memcg_charge_slab(struct kmem_cache *s, gfp_t gfp, int order)
+{
+	if (is_root_cache(s))
+		return 0;
+	return memcg_charge_kmem(s->memcg_params->memcg, gfp,
+				 PAGE_SIZE << order);
+}
+
+void memcg_uncharge_slab(struct kmem_cache *s, int order)
+{
+	if (is_root_cache(s))
+		return;
+	memcg_uncharge_kmem(s->memcg_params->memcg, PAGE_SIZE << order);
+}
+
 /*
  * We need to verify if the allocation against current->mm->owner's memcg is
  * possible for the given order. But the page is not allocated yet, so we'll

mm/slab.c +6 -1

index eebc619ae33c..af126a37dafd 100644
@@ -1664,8 +1664,12 @@ static struct page *kmem_getpages(struct kmem_cache *cachep, gfp_t flags,
 	if (cachep->flags & SLAB_RECLAIM_ACCOUNT)
 		flags |= __GFP_RECLAIMABLE;
 
+	if (memcg_charge_slab(cachep, flags, cachep->gfporder))
+		return NULL;
+
 	page = alloc_pages_exact_node(nodeid, flags | __GFP_NOTRACK, cachep->gfporder);
 	if (!page) {
+		memcg_uncharge_slab(cachep, cachep->gfporder);
 		if (!(flags & __GFP_NOWARN) && printk_ratelimit())
 			slab_out_of_memory(cachep, flags, nodeid);
 		return NULL;

@@ -1724,7 +1728,8 @@ static void kmem_freepages(struct kmem_cache *cachep, struct page *page)
 	memcg_release_pages(cachep, cachep->gfporder);
 	if (current->reclaim_state)
 		current->reclaim_state->reclaimed_slab += nr_freed;
-	__free_memcg_kmem_pages(page, cachep->gfporder);
+	__free_pages(page, cachep->gfporder);
+	memcg_uncharge_slab(cachep, cachep->gfporder);
 }
 
 static void kmem_rcu_free(struct rcu_head *head)

mm/slab_common.c +1 -5

index f3cfccf76dda..6673597ac967 100644
@@ -290,12 +290,8 @@ void kmem_cache_create_memcg(struct mem_cgroup *memcg, struct kmem_cache *root_c
 				 root_cache->size, root_cache->align,
 				 root_cache->flags, root_cache->ctor,
 				 memcg, root_cache);
-	if (IS_ERR(s)) {
+	if (IS_ERR(s))
 		kfree(cache_name);
-		goto out_unlock;
-	}
-
-	s->allocflags |= __GFP_KMEMCG;
 
 out_unlock:
 	mutex_unlock(&slab_mutex);

mm/slub.c +17 -7

index c2e58a787443..6fefe3b33ce0 100644
@@ -1317,17 +1317,26 @@ static inline void slab_free_hook(struct kmem_cache *s, void *x)
 /*
  * Slab allocation and freeing
  */
-static inline struct page *alloc_slab_page(gfp_t flags, int node,
-					struct kmem_cache_order_objects oo)
+static inline struct page *alloc_slab_page(struct kmem_cache *s,
+		gfp_t flags, int node, struct kmem_cache_order_objects oo)
 {
+	struct page *page;
 	int order = oo_order(oo);
 
 	flags |= __GFP_NOTRACK;
 
+	if (memcg_charge_slab(s, flags, order))
+		return NULL;
+
 	if (node == NUMA_NO_NODE)
-		return alloc_pages(flags, order);
+		page = alloc_pages(flags, order);
 	else
-		return alloc_pages_exact_node(node, flags, order);
+		page = alloc_pages_exact_node(node, flags, order);
+
+	if (!page)
+		memcg_uncharge_slab(s, order);
+
+	return page;
 }
 
 static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)

@@ -1349,7 +1358,7 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
 	 */
 	alloc_gfp = (flags | __GFP_NOWARN | __GFP_NORETRY) & ~__GFP_NOFAIL;
 
-	page = alloc_slab_page(alloc_gfp, node, oo);
+	page = alloc_slab_page(s, alloc_gfp, node, oo);
 	if (unlikely(!page)) {
 		oo = s->min;
 		alloc_gfp = flags;

@@ -1357,7 +1366,7 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
 		 * Allocation may have failed due to fragmentation.
 		 * Try a lower order alloc if possible
 		 */
-		page = alloc_slab_page(alloc_gfp, node, oo);
+		page = alloc_slab_page(s, alloc_gfp, node, oo);
 
 		if (page)
 			stat(s, ORDER_FALLBACK);

@@ -1473,7 +1482,8 @@ static void __free_slab(struct kmem_cache *s, struct page *page)
 	page_mapcount_reset(page);
 	if (current->reclaim_state)
 		current->reclaim_state->reclaimed_slab += pages;
-	__free_memcg_kmem_pages(page, order);
+	__free_pages(page, order);
+	memcg_uncharge_slab(s, order);
 }
 

Quoted 4 lines

mm/slub.c +0 -0

 #define need_reserve_slab_rcu						\

Signature · 1 line

1.7.10.4

Quoted 6 lines · Michal Hocko

Quoted 5 lines · Vladimir Davydov

We have only a few places where we actually want to charge kmem so instead of intruding into the general page allocation path with __GFP_KMEMCG it's better to explictly charge kmem there. All kmem charges will be easier to follow that way.

Quoted 8 lines

Quoted 4 lines

This is a step towards removing __GFP_KMEMCG. It removes __GFP_KMEMCG from memcg caches' allocflags. Instead it makes slab allocation path call memcg_charge_kmem directly getting memcg to charge from the cache's memcg params.

Yes, removing __GFP_KMEMCG is definitely a good step. I am currently at a conference and do not have much time to review this properly (even worse will be on vacation for the next 2 weeks) but where did all the static_key optimization go? What am I missing.

I expected this question, because I want somebody to confirm if we really need such kind of optimization in the slab allocation path. From my POV, since we thrash cpu caches there anyway by calling alloc_pages, wrapping memcg_charge_slab in a static branch wouldn't result in any noticeable performance boost.

I do admit we benefit from static branching in memcg_kmem_get_cache, because this one is called on every kmem object allocation, but slab allocations happen much rarer.

I don't insist on that though, so if you say "no", I'll just add __memcg_charge_slab and make memcg_charge_slab call it if the static key is on, but may be, we can avoid such code bloating?

We have only a few places where we actually want to charge kmem so instead of intruding into the general page allocation path with __GFP_KMEMCG it's better to explictly charge kmem there. All kmem charges will be easier to follow that way.

This is a step toward removing __GFP_KMEMCG. It makes fork charge task threadinfo pages explicitly instead of passing __GFP_KMEMCG to alloc_pages.

Signed-off-by Vladimir Davydov <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Glauber Costa <[email protected]>

---
 kernel/fork.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

kernel/fork.c +10 -3

index f4b09bc15f3a..8209780cf732 100644
@@ -150,15 +150,22 @@ void __weak arch_release_thread_info(struct thread_info *ti)
 static struct thread_info *alloc_thread_info_node(struct task_struct *tsk,
 						  int node)
 {
-	struct page *page = alloc_pages_node(node, THREADINFO_GFP_ACCOUNTED,
-					     THREAD_SIZE_ORDER);
+	struct page *page;
+	struct mem_cgroup *memcg = NULL;
 
+	if (!memcg_kmem_newpage_charge(THREADINFO_GFP_ACCOUNTED, &memcg,
+				       THREAD_SIZE_ORDER))
+		return NULL;
+	page = alloc_pages_node(node, THREADINFO_GFP, THREAD_SIZE_ORDER);
+	memcg_kmem_commit_charge(page, memcg, THREAD_SIZE_ORDER);
 	return page ? page_address(page) : NULL;
 }
 
 static inline void free_thread_info(struct thread_info *ti)
 {
-	free_memcg_kmem_pages((unsigned long)ti, THREAD_SIZE_ORDER);
+	if (ti)
+		memcg_kmem_uncharge_pages(virt_to_page(ti), THREAD_SIZE_ORDER);
+	free_pages((unsigned long)ti, THREAD_SIZE_ORDER);
 }
 # else
 static struct kmem_cache *thread_info_cache;

Signature · 1 line

1.7.10.4

Quoted 8 lines · Vladimir Davydov

We have only a few places where we actually want to charge kmem so instead of intruding into the general page allocation path with __GFP_KMEMCG it's better to explictly charge kmem there. All kmem charges will be easier to follow that way.

This is a step toward removing __GFP_KMEMCG. It makes fork charge task threadinfo pages explicitly instead of passing __GFP_KMEMCG to alloc_pages.

Looks good from a quick glance. I would also remove THREADINFO_GFP_ACCOUNTED in this patch.

Quoted 29 lines

Signed-off-by Vladimir Davydov <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Glauber Costa <[email protected]>

---
 kernel/fork.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

kernel/fork.c +7 -2

index f4b09bc15f3a..8209780cf732 100644
@@ -150,15 +150,22 @@ void __weak arch_release_thread_info(struct thread_info *ti)
 static struct thread_info *alloc_thread_info_node(struct task_struct *tsk,
 						  int node)
 {
-	struct page *page = alloc_pages_node(node, THREADINFO_GFP_ACCOUNTED,
-					     THREAD_SIZE_ORDER);
+	struct page *page;
+	struct mem_cgroup *memcg = NULL;
 
+	if (!memcg_kmem_newpage_charge(THREADINFO_GFP_ACCOUNTED, &memcg,
+				       THREAD_SIZE_ORDER))
+		return NULL;
+	page = alloc_pages_node(node, THREADINFO_GFP, THREAD_SIZE_ORDER);
+	memcg_kmem_commit_charge(page, memcg, THREAD_SIZE_ORDER);
 	return page ? page_address(page) : NULL;
 }
 

Quoted 12 lines

kernel/fork.c +3 -1

 static inline void free_thread_info(struct thread_info *ti)
 {
-	free_memcg_kmem_pages((unsigned long)ti, THREAD_SIZE_ORDER);
+	if (ti)
+		memcg_kmem_uncharge_pages(virt_to_page(ti), THREAD_SIZE_ORDER);
+	free_pages((unsigned long)ti, THREAD_SIZE_ORDER);
 }
 # else
 static struct kmem_cache *thread_info_cache;

Signature · 1 line

1.7.10.4