The new cgroup slab memory controller

To convert memcg and lruvec slab counters to bytes there must be a way to change these counters without touching node counters. Factor out __mod_memcg_lruvec_state() out of __mod_lruvec_state().

Signed-off-by Roman Gushchin <[email protected]>

To convert memcg and lruvec slab counters to bytes there must be a way to change these counters without touching node counters. Factor out __mod_memcg_lruvec_state() out of __mod_lruvec_state().

Signed-off-by Roman Gushchin <[email protected]>

mm/memcontrol.c +0 -6

@@ -713,30 +713,14 @@ parent_nodeinfo(struct mem_cgroup_per_node *pn, int nid)
 	return mem_cgroup_nodeinfo(parent, nid);
 }
 
-/**
- * __mod_lruvec_state - update lruvec memory statistics
- * @lruvec: the lruvec
- * @idx: the stat item
- * @val: delta to add to the counter, can be negative
- *

To implement per-object slab memory accounting, we need to convert slab vmstat counters to bytes. Actually, out of 4 levels of counters: global, per-node, per-memcg and per-lruvec only two last levels will require byte-sized counters. It's because global and per-node counters will be counting the number of slab pages, and per-memcg and per-lruvec will be counting the amount of memory taken by charged slab objects.

Converting all vmstat counters to bytes or even all slab counters to bytes would introduce an additional overhead. So instead let's store global and per-node counters in pages, and memcg and lruvec counters in bytes.

To make the API clean all access helpers (both on the read and write sides) are dealing with bytes.

To avoid back-and-forth conversions a new flavor of helpers is introduced, which always returns values in pages:
node_page_state_pages() and global_node_page_state_pages().

Actually new helpers are just reading raw values. Old helpers are simple wrappers, which perform a conversion if the vmstat items are in bytes. Because at the moment no one actually need bytes, there are WARN_ON_ONCE() macroses inside to warn about inappropriate use cases.

Thanks to Johannes Weiner for the idea of having the byte-sized API on top of the page-sized internal storage.

Signed-off-by Roman Gushchin <[email protected]>

To implement per-object slab memory accounting, we need to convert slab vmstat counters to bytes. Actually, out of 4 levels of counters: global, per-node, per-memcg and per-lruvec only two last levels will require byte-sized counters. It's because global and per-node counters will be counting the number of slab pages, and per-memcg and per-lruvec will be counting the amount of memory taken by charged slab objects.

Converting all vmstat counters to bytes or even all slab counters to bytes would introduce an additional overhead. So instead let's store global and per-node counters in pages, and memcg and lruvec counters in bytes.

To make the API clean all access helpers (both on the read and write sides) are dealing with bytes.

To avoid back-and-forth conversions a new flavor of helpers is introduced, which always returns values in pages:
node_page_state_pages() and global_node_page_state_pages().

Actually new helpers are just reading raw values. Old helpers are simple wrappers, which perform a conversion if the vmstat items are in bytes. Because at the moment no one actually need bytes, there are WARN_ON_ONCE() macroses inside to warn about inappropriate use cases.

Thanks to Johannes Weiner for the idea of having the byte-sized API on top of the page-sized internal storage.

Signed-off-by Roman Gushchin <[email protected]>

include/linux/mmzone.h +4 -0

+{
+	return false;
+}
+
 /*
  * We do arithmetic on the LRU lists in various places in the code,
  * so it is important to keep the active lists LRU_ACTIVE higher in

Quoted 29 lines · Roman Gushchin

To implement per-object slab memory accounting, we need to convert slab vmstat counters to bytes. Actually, out of 4 levels of counters: global, per-node, per-memcg and per-lruvec only two last levels will require byte-sized counters. It's because global and per-node counters will be counting the number of slab pages, and per-memcg and per-lruvec will be counting the amount of memory taken by charged slab objects.

Converting all vmstat counters to bytes or even all slab counters to bytes would introduce an additional overhead. So instead let's store global and per-node counters in pages, and memcg and lruvec counters in bytes.

To make the API clean all access helpers (both on the read and write sides) are dealing with bytes.

To avoid back-and-forth conversions a new flavor of helpers is introduced, which always returns values in pages:
node_page_state_pages() and global_node_page_state_pages().

Actually new helpers are just reading raw values. Old helpers are simple wrappers, which perform a conversion if the vmstat items are in bytes. Because at the moment no one actually need bytes, there are WARN_ON_ONCE() macroses inside to warn about inappropriate use cases.

Thanks to Johannes Weiner for the idea of having the byte-sized API on top of the page-sized internal storage.

Quoted 1 line

Signed-off-by Roman Gushchin <[email protected]>

Reviewed-by Vlastimil Babka <[email protected]>

But it's somewhat complicated, so it would be great to document it in comments of e.g. include/linux/vmstat.h that what the API returns as unsigned long, can be either bytes or pages depending on vmstat_item_in_bytes().

In order to prepare for per-object slab memory accounting, convert NR_SLAB_RECLAIMABLE and NR_SLAB_UNRECLAIMABLE vmstat items to bytes.

To make it obvious, rename them to NR_SLAB_RECLAIMABLE_B and NR_SLAB_UNRECLAIMABLE_B (similar to NR_KERNEL_STACK_KB).

Internally global and per-node counters are stored in pages, however memcg and lruvec counters are stored in bytes. This scheme may look weird, but only for now. As soon as slab pages will be shared between multiple cgroups, global and node counters will reflect the total number of slab pages. However memcg and lruvec counters will be used for per-memcg slab memory tracking, which will take separate kernel objects in the account. Keeping global and node counters in pages helps to avoid additional overhead.

The size of slab memory shouldn't exceed 4Gb on 32-bit machines, so it will fit into atomic_long_t we use for vmstats.

Signed-off-by Roman Gushchin <[email protected]>

In order to prepare for per-object slab memory accounting, convert NR_SLAB_RECLAIMABLE and NR_SLAB_UNRECLAIMABLE vmstat items to bytes.

To make it obvious, rename them to NR_SLAB_RECLAIMABLE_B and NR_SLAB_UNRECLAIMABLE_B (similar to NR_KERNEL_STACK_KB).

Internally global and per-node counters are stored in pages, however memcg and lruvec counters are stored in bytes. This scheme may look weird, but only for now. As soon as slab pages will be shared between multiple cgroups, global and node counters will reflect the total number of slab pages. However memcg and lruvec counters will be used for per-memcg slab memory tracking, which will take separate kernel objects in the account. Keeping global and node counters in pages helps to avoid additional overhead.

The size of slab memory shouldn't exceed 4Gb on 32-bit machines, so it will fit into atomic_long_t we use for vmstats.

Signed-off-by Roman Gushchin <[email protected]>

---
 drivers/base/node.c     |  4 ++--
 fs/proc/meminfo.c       |  4 ++--
 include/linux/mmzone.h  | 16 +++++++++++++---
 kernel/power/snapshot.c |  2 +-
 mm/memcontrol.c         | 11 ++++-------
 mm/oom_kill.c           |  2 +-
 mm/page_alloc.c         |  8 ++++----
 mm/slab.h               | 15 ++++++++-------
 mm/slab_common.c        |  4 ++--
 mm/slob.c               | 12 ++++++------
 mm/slub.c               |  8 ++++----
 mm/vmscan.c             |  3 ++-
 mm/workingset.c         |  6 ++++--
 13 files changed, 53 insertions(+), 42 deletions(-)

include/linux/mmzone.h +2 -1

@@ -206,7 +206,17 @@ enum node_stat_item {
 
 static __always_inline bool vmstat_item_in_bytes(enum node_stat_item item)
 {
-	return false;
+	/*
+	 * Global and per-node slab counters track slab pages.

mm/memcontrol.c +2 -3

@@ -1409,9 +1409,8 @@ static char *memory_stat_format(struct mem_cgroup *memcg)
 		       (u64)memcg_page_state(memcg, MEMCG_KERNEL_STACK_KB) *
 		       1024);
 	seq_buf_printf(&s, "slab %llu\n",
-		       (u64)(memcg_page_state(memcg, NR_SLAB_RECLAIMABLE) +
-			     memcg_page_state(memcg, NR_SLAB_UNRECLAIMABLE)) *
-		       PAGE_SIZE);
+		       (u64)(memcg_page_state(memcg, NR_SLAB_RECLAIMABLE_B) +
+			     memcg_page_state(memcg, NR_SLAB_UNRECLAIMABLE_B)));
 	seq_buf_printf(&s, "sock %llu\n",
 		       (u64)memcg_page_state(memcg, MEMCG_SOCK) *
 		       PAGE_SIZE);

Quoted 39 lines · Roman Gushchin

In order to prepare for per-object slab memory accounting, convert NR_SLAB_RECLAIMABLE and NR_SLAB_UNRECLAIMABLE vmstat items to bytes.

To make it obvious, rename them to NR_SLAB_RECLAIMABLE_B and NR_SLAB_UNRECLAIMABLE_B (similar to NR_KERNEL_STACK_KB).

Internally global and per-node counters are stored in pages, however memcg and lruvec counters are stored in bytes. This scheme may look weird, but only for now. As soon as slab pages will be shared between multiple cgroups, global and node counters will reflect the total number of slab pages. However memcg and lruvec counters will be used for per-memcg slab memory tracking, which will take separate kernel objects in the account. Keeping global and node counters in pages helps to avoid additional overhead.

The size of slab memory shouldn't exceed 4Gb on 32-bit machines, so it will fit into atomic_long_t we use for vmstats.

Signed-off-by Roman Gushchin <[email protected]>

---
 drivers/base/node.c     |  4 ++--
 fs/proc/meminfo.c       |  4 ++--
 include/linux/mmzone.h  | 16 +++++++++++++---
 kernel/power/snapshot.c |  2 +-
 mm/memcontrol.c         | 11 ++++-------
 mm/oom_kill.c           |  2 +-
 mm/page_alloc.c         |  8 ++++----
 mm/slab.h               | 15 ++++++++-------
 mm/slab_common.c        |  4 ++--
 mm/slob.c               | 12 ++++++------
 mm/slub.c               |  8 ++++----
 mm/vmscan.c             |  3 ++-
 mm/workingset.c         |  6 ++++--
 13 files changed, 53 insertions(+), 42 deletions(-)

(quoted diff) +0 -0

@@ -206,7 +206,17 @@ enum node_stat_item {
 

Quoted 4 lines

include/linux/mmzone.h +1 -1

 static __always_inline bool vmstat_item_in_bytes(enum node_stat_item item)
 {
-	return false;
+	/*

include/linux/mmzone.h enum node_stat_item { · line 219

Quoted 10 lines

include/linux/mmzone.h +10 -0

+	 * Global and per-node slab counters track slab pages.
+	 * It's expected that changes are multiples of PAGE_SIZE.
+	 * Internally values are stored in pages.
+	 *
+	 * Per-memcg and per-lruvec counters track memory, consumed
+	 * by individual slab objects. These counters are actually
+	 * byte-precise.
+	 */
+	return (item == NR_SLAB_RECLAIMABLE_B ||
+		item == NR_SLAB_UNRECLAIMABLE_B);

Review enum node_stat_item { · line 220

Quoted 1 line

(quoted diff) +0 -0

 }

Ok, so this is no longer a no-op, but __always_inline here and inline in global_node_page_state() should hopefully mean that for all users of global_node_page_state(<constant>) the compiler will eliminate the branch for non-slab counters. But there are also functions such as si_mem_available() that use non-constant item. Maybe compiler is smart enough anyway, but perhaps it's better to use global_node_page_state_pages() in such callers?

However __mod_node_page_state() and mode_node_state() will now branch always. I wonder if the "API clean" goal is worth it...

Quoted 18 lines

mm/memcontrol.c +2 -3

@@ -1409,9 +1409,8 @@ static char *memory_stat_format(struct mem_cgroup *memcg)
 		       (u64)memcg_page_state(memcg, MEMCG_KERNEL_STACK_KB) *
 		       1024);
 	seq_buf_printf(&s, "slab %llu\n",
-		       (u64)(memcg_page_state(memcg, NR_SLAB_RECLAIMABLE) +
-			     memcg_page_state(memcg, NR_SLAB_UNRECLAIMABLE)) *
-		       PAGE_SIZE);
+		       (u64)(memcg_page_state(memcg, NR_SLAB_RECLAIMABLE_B) +
+			     memcg_page_state(memcg, NR_SLAB_UNRECLAIMABLE_B)));
 	seq_buf_printf(&s, "sock %llu\n",
 		       (u64)memcg_page_state(memcg, MEMCG_SOCK) *
 		       PAGE_SIZE);

@@ -1445,11 +1444,9 @@ static char *memory_stat_format(struct mem_cgroup *memcg)
 			       PAGE_SIZE);
 

mm/memcontrol.c static char *memory_stat_format(struct mem_cgroup *memcg) · line 1449

Quoted 8 lines

mm/memcontrol.c +2 -4

 	seq_buf_printf(&s, "slab_reclaimable %llu\n",
-		       (u64)memcg_page_state(memcg, NR_SLAB_RECLAIMABLE) *
-		       PAGE_SIZE);
+		       (u64)memcg_page_state(memcg, NR_SLAB_RECLAIMABLE_B));
 	seq_buf_printf(&s, "slab_unreclaimable %llu\n",
-		       (u64)memcg_page_state(memcg, NR_SLAB_UNRECLAIMABLE) *
-		       PAGE_SIZE);
+		       (u64)memcg_page_state(memcg, NR_SLAB_UNRECLAIMABLE_B));

So here we are now printing in bytes instead of pages, right? That's fine for OOM report, but in sysfs aren't we breaking existing users?

Quoted 3 lines · Vlastimil Babka

However __mod_node_page_state() and mode_node_state() will now branch always. I wonder if the "API clean" goal is worth it...

You mean just adding a special write-side helper which will perform a conversion and put VM_WARN_ON_ONCE() into generic write-side helpers?

Quoted 18 lines

Quoted 18 lines

mm/memcontrol.c +2 -3

@@ -1409,9 +1409,8 @@ static char *memory_stat_format(struct mem_cgroup *memcg)
 		       (u64)memcg_page_state(memcg, MEMCG_KERNEL_STACK_KB) *
 		       1024);
 	seq_buf_printf(&s, "slab %llu\n",
-		       (u64)(memcg_page_state(memcg, NR_SLAB_RECLAIMABLE) +
-			     memcg_page_state(memcg, NR_SLAB_UNRECLAIMABLE)) *
-		       PAGE_SIZE);
+		       (u64)(memcg_page_state(memcg, NR_SLAB_RECLAIMABLE_B) +
+			     memcg_page_state(memcg, NR_SLAB_UNRECLAIMABLE_B)));
 	seq_buf_printf(&s, "sock %llu\n",
 		       (u64)memcg_page_state(memcg, MEMCG_SOCK) *
 		       PAGE_SIZE);

@@ -1445,11 +1444,9 @@ static char *memory_stat_format(struct mem_cgroup *memcg)
 			       PAGE_SIZE);
 

Quoted 12 lines

mm/memcontrol.c static char *memory_stat_format(struct mem_cgroup *memcg) · line 1449

Quoted 8 lines

mm/memcontrol.c +2 -4

 	seq_buf_printf(&s, "slab_reclaimable %llu\n",
-		       (u64)memcg_page_state(memcg, NR_SLAB_RECLAIMABLE) *
-		       PAGE_SIZE);
+		       (u64)memcg_page_state(memcg, NR_SLAB_RECLAIMABLE_B));
 	seq_buf_printf(&s, "slab_unreclaimable %llu\n",
-		       (u64)memcg_page_state(memcg, NR_SLAB_UNRECLAIMABLE) *
-		       PAGE_SIZE);
+		       (u64)memcg_page_state(memcg, NR_SLAB_UNRECLAIMABLE_B));

So here we are now printing in bytes instead of pages, right? That's fine for OOM report, but in sysfs aren't we breaking existing users?

Hm, but it was in bytes previously, look at that x * PAGE_SIZE.

Or do you mean that now it can be not rounded to PAGE_SIZE? If so, I don't think it breaks any expectations.

Thanks!

Quoted 5 lines · Roman Gushchin

Quoted 3 lines · Vlastimil Babka

However __mod_node_page_state() and mode_node_state() will now branch always. I wonder if the "API clean" goal is worth it...

Quoted 2 lines

You mean just adding a special write-side helper which will perform a conversion and put VM_WARN_ON_ONCE() into generic write-side helpers?

What I mean is that maybe node/global helpers should assume page granularity, and lruvec/memcg helpers do the check is they should convert from bytes to pages when calling node/global helpers. Then there would be no extra branches in node/global helpers. But maybe it's not worth saving those branches, dunno.

include/linux/slub_def.h struct kmem_cache { · line 90

Quoted 7 lines · Roman Gushchin

enum stat_item {

include/linux/slub_def.h +1 -0

 	ALLOC_FASTPATH,		/* Allocation from cpu slab */

@@ -86,6 +87,7 @@ struct kmem_cache {
 	unsigned long min_partial;
 	unsigned int size;	/* The size of an object including metadata */
 	unsigned int object_size;/* The size of an object without metadata */
+	struct reciprocal_value reciprocal_size;

This needs to be moved further back since it is not an item that needs to be cache hot for the hotpaths.

Place it with "align", inuse etc?

Hmmm. the same applies to min_partial maybe?

Quoted 13 lines · Christoph Lameter

include/linux/slub_def.h struct kmem_cache { · line 90

Quoted 7 lines · Roman Gushchin

enum stat_item {

include/linux/slub_def.h +1 -0

 	ALLOC_FASTPATH,		/* Allocation from cpu slab */

@@ -86,6 +87,7 @@ struct kmem_cache {
 	unsigned long min_partial;
 	unsigned int size;	/* The size of an object including metadata */
 	unsigned int object_size;/* The size of an object without metadata */
+	struct reciprocal_value reciprocal_size;

This needs to be moved further back since it is not an item that needs to be cache hot for the hotpaths.

It could be relatively hot, because it's accessed for reading on every accounted allocation.

Quoted 15 lines · Roman Gushchin

Quoted 13 lines · Christoph Lameter

include/linux/slub_def.h struct kmem_cache { · line 90

Quoted 7 lines · Roman Gushchin

enum stat_item {

include/linux/slub_def.h +1 -0

 	ALLOC_FASTPATH,		/* Allocation from cpu slab */

@@ -86,6 +87,7 @@ struct kmem_cache {
 	unsigned long min_partial;
 	unsigned int size;	/* The size of an object including metadata */
 	unsigned int object_size;/* The size of an object without metadata */
+	struct reciprocal_value reciprocal_size;

This needs to be moved further back since it is not an item that needs to be cache hot for the hotpaths.

Quoted 2 lines

It could be relatively hot, because it's accessed for reading on every accounted allocation.

The patch seems to only use it for setup and debugging? It is used for every "accounted" allocation???? Where? And what is an "accounted" allocation?

Quoted 5 lines

The patch seems to only use it for setup and debugging? It is used for every "accounted" allocation???? Where? And what is an "accounted" allocation?

Please, take a look at the whole series:
https://lore.kernel.org/linux-mm/[email protected]/T/#t

I'm sorry, I had to cc you directly for the whole thing. Your feedback will be highly appreciated.

It's used to calculate the offset of the memcg pointer for every slab object which is charged to a memory cgroup. So it must be quite hot.

Quoted 6 lines · Roman Gushchin

Quoted 5 lines

The patch seems to only use it for setup and debugging? It is used for every "accounted" allocation???? Where? And what is an "accounted" allocation?

Quoted 8 lines

Please, take a look at the whole series:
https://lore.kernel.org/linux-mm/[email protected]/T/#t

I'm sorry, I had to cc you directly for the whole thing. Your feedback will be highly appreciated.

It's used to calculate the offset of the memcg pointer for every slab object which is charged to a memory cgroup. So it must be quite hot.

Ahh... Thanks. I just looked at it.

You need this because you have a separate structure attached to a page that tracks membership of the slab object to the cgroup. This is used to calculate the offset into that array....

Why do you need this? Just slap a pointer to the cgroup as additional metadata onto the slab object. Is that not much simpler, safer and faster?

Quoted 18 lines · Christoph Lameter

Quoted 6 lines · Roman Gushchin

Quoted 5 lines

The patch seems to only use it for setup and debugging? It is used for every "accounted" allocation???? Where? And what is an "accounted" allocation?

Quoted 8 lines

Please, take a look at the whole series:
https://lore.kernel.org/linux-mm/[email protected]/T/#t

I'm sorry, I had to cc you directly for the whole thing. Your feedback will be highly appreciated.

It's used to calculate the offset of the memcg pointer for every slab object which is charged to a memory cgroup. So it must be quite hot.

Quoted 9 lines

Ahh... Thanks. I just looked at it.

You need this because you have a separate structure attached to a page that tracks membership of the slab object to the cgroup. This is used to calculate the offset into that array....

Why do you need this? Just slap a pointer to the cgroup as additional metadata onto the slab object. Is that not much simpler, safer and faster?

So, the problem is that not all slab objects are accounted, and sometimes we don't know if advance if they are accounted or not (with the current semantics of __GFP_ACCOUNT and SLAB_ACCOUNT flags). So we either have to increase the size of ALL slab objects, either create a pair of slab caches for each size.

The first option is not that cheap in terms of the memory overhead. Especially for those who disable cgroups using a boot-time option.
The second should be fine, but it will be less simple in terms of the code complexity (in comparison to the final result of the current proposal).

I'm not strictly against of either approach, but I'd look for a broader consensus on what's the best approach here.

Quoted 24 lines · Christoph Lameter

Quoted 6 lines · Roman Gushchin

Quoted 5 lines

The patch seems to only use it for setup and debugging? It is used for every "accounted" allocation???? Where? And what is an "accounted" allocation?

Quoted 8 lines

Please, take a look at the whole series:
https://lore.kernel.org/linux-mm/[email protected]/T/#t

I'm sorry, I had to cc you directly for the whole thing. Your feedback will be highly appreciated.

It's used to calculate the offset of the memcg pointer for every slab object which is charged to a memory cgroup. So it must be quite hot.

Ahh... Thanks. I just looked at it.

You need this because you have a separate structure attached to a page that tracks membership of the slab object to the cgroup. This is used to calculate the offset into that array....

Quoted 3 lines

Why do you need this? Just slap a pointer to the cgroup as additional metadata onto the slab object. Is that not much simpler, safer and faster?

So, the problem is that not all slab objects are accounted, and sometimes we don't know if advance if they are accounted or not (with the current semantics of __GFP_ACCOUNT and SLAB_ACCOUNT flags). So we either have to increase the size of ALL slab objects, either create a pair of slab caches for each size.

Quoted 3 lines

Why do you need this? Just slap a pointer to the cgroup as additional metadata onto the slab object. Is that not much simpler, safer and faster?

So, the problem is that not all slab objects are accounted, and sometimes we don't know if advance if they are accounted or not (with the current semantics of __GFP_ACCOUNT and SLAB_ACCOUNT flags). So we either have to increase the size of ALL slab objects, either create a pair of slab caches for each size.

The first option is not that cheap in terms of the memory overhead. Especially for those who disable cgroups using a boot-time option.

Quoted 4 lines · Roman Gushchin

Quoted 3 lines

Why do you need this? Just slap a pointer to the cgroup as additional metadata onto the slab object. Is that not much simpler, safer and faster?

Quoted 8 lines

So, the problem is that not all slab objects are accounted, and sometimes we don't know if advance if they are accounted or not (with the current semantics of __GFP_ACCOUNT and SLAB_ACCOUNT flags). So we either have to increase the size of ALL slab objects, either create a pair of slab caches for each size.

The first option is not that cheap in terms of the memory overhead. Especially for those who disable cgroups using a boot-time option.

If the cgroups are disabled on boot time then you can switch back to the compact version. Otherwise just add a pointer to each object. It will make it consistent and there is not much memory wastage.

The problem comes about with the power of 2 caches in the kmalloc array.

If one keeps the "natural alignment" instead of going for the normal alignment of slab caches then the alignment will cause a lot of memory wastage and thus the scheme of off slab metadata is likely going to be unavoidable.

But I think we are just stacking one bad idea onto another here making things much more complex than they could be. Well at least this justifies all our jobs .... (not mine I am out of work... hehehe)

Sorry, but what exactly do you mean?

Quoted 1 line · Roman Gushchin

Sorry, but what exactly do you mean?

I think the right approach is to add a pointer to each slab object for memcg support.

Quoted 4 lines · Christoph Lameter

Quoted 1 line · Roman Gushchin

Sorry, but what exactly do you mean?

Quoted 3 lines

I think the right approach is to add a pointer to each slab object for memcg support.

As I understand, embedding the memcg pointer will hopefully make allocations cheaper in terms of CPU, but will require more memory. And you think that it's worth it. Is it a correct understanding?

Can you, please, describe a bit more detailed how it should be done from your point of view?

I mean where to store the pointer, should it be SLAB/SLUB-specific code or a generic code, what do to with kmallocs alignments, should we merge slabs which had a different size before and now have the same because of the memcg pointer and aligment, etc.

I'm happy to follow your advice and perform some tests to get an idea of how significant the memory overhead is and how big are CPU savings. I guess with these numbers it will be easy to make a decision.

Quoted 5 lines · Roman Gushchin

Quoted 4 lines · Christoph Lameter

Quoted 1 line · Roman Gushchin

Sorry, but what exactly do you mean?

Quoted 7 lines

Quoted 3 lines

I think the right approach is to add a pointer to each slab object for memcg support.

As I understand, embedding the memcg pointer will hopefully make allocations cheaper in terms of CPU, but will require more memory. And you think that it's worth it. Is it a correct understanding?

It definitely makes the code less complex. The additional memory is minimal. In many cases you have already some space wasted at the end of the object that could be used for the pointer.

Quoted 2 lines

Can you, please, describe a bit more detailed how it should be done from your point of view?

Add it to the metadata at the end of the object. Like the debugging information or the pointer for RCU freeing.

Quoted 5 lines

I mean where to store the pointer, should it be SLAB/SLUB-specific code or a generic code, what do to with kmallocs alignments, should we merge slabs which had a different size before and now have the same because of the memcg pointer and aligment, etc.

Both SLAB and SLUB have the same capabilities there. Slabs that had different sizes before will now have different sizes as well. So the merging does not change.

Quoted 4 lines

I'm happy to follow your advice and perform some tests to get an idea of how significant the memory overhead is and how big are CPU savings. I guess with these numbers it will be easy to make a decision.

Sure. The main issue are the power of two kmalloc caches and how to add the pointer to these caches in order not to waste memory. SLAB has done this in the past by creating additional structues in a page frame.

Quoted 2 lines

Add it to the metadata at the end of the object. Like the debugging information or the pointer for RCU freeing.

Enabling debugging metadata currently disables the cache merging. I doubt that it's acceptable to sacrifice the cache merging in order to embed the memcg pointer?

Figuring out all these details will likely take several weeks, so the whole thing will be delayed for one-two major releases (in the best case). Given that the current implementation saves ~40% of slab memory, I think there is some value in delivering it as it is. So I wonder if the idea of embedding the pointer should be considered a blocker, or it can be implemented of top of the proposed code (given it's not a user-facing api or something like this)?

Quoted 6 lines · Roman Gushchin

Quoted 2 lines

Add it to the metadata at the end of the object. Like the debugging information or the pointer for RCU freeing.

Enabling debugging metadata currently disables the cache merging. I doubt that it's acceptable to sacrifice the cache merging in order to embed the memcg pointer?

Well then keep the merging even if you have a memcg pointer.

The disabling for debugging is only to simplify debugging. You dont have to deal with multiple caches actually using the same storage structures.

Quoted 6 lines

Figuring out all these details will likely take several weeks, so the whole thing will be delayed for one-two major releases (in the best case). Given that the current implementation saves ~40% of slab memory, I think there is some value in delivering it as it is. So I wonder if the idea of embedding the pointer should be considered a blocker, or it can be implemented of top of the proposed code (given it's not a user-facing api or something like this)?

Sorry no idea from my end here.

Btw, I'm trying to build up a prototype with an embedded memcg pointer, but it seems to be way more tricky than I thought. It requires changes to shrinkers (as they rely on getting the memcg pointer by an arbitrary kernel address, not necessarily aligned to the head of slab allocation), figuring out cache merging, adding SLAB support, natural alignment of kmallocs etc.

Figuring out all these details will likely take several weeks, so the whole thing will be delayed for one-two major releases (in the best case). Given that the current implementation saves ~40% of slab memory, I think there is some value in delivering it as it is. So I wonder if the idea of embedding the pointer should be considered a blocker, or it can be implemented of top of the proposed code (given it's not a user-facing api or something like this)?

Thanks!

Quoted 7 lines · Roman Gushchin

Btw, I'm trying to build up a prototype with an embedded memcg pointer, but it seems to be way more tricky than I thought. It requires changes to shrinkers (as they rely on getting the memcg pointer by an arbitrary kernel address, not necessarily aligned to the head of slab allocation), figuring out cache merging, adding SLAB support, natural alignment of kmallocs etc.

Is the natural alignment of kmallocs a problem right now? As kmalloc() allocations are AFAIK not kmemcg-accounted? Or does your implementation add memcg awareness to everything, even if non-__GFP_ACCOUNT allocations just get a root memcg pointer?

Quoted 5 lines · Roman Gushchin

Quoted 4 lines · Christoph Lameter

Quoted 1 line · Roman Gushchin

Sorry, but what exactly do you mean?

Quoted 7 lines

Quoted 3 lines

I think the right approach is to add a pointer to each slab object for memcg support.

As I understand, embedding the memcg pointer will hopefully make allocations cheaper in terms of CPU, but will require more memory. And you think that it's worth it. Is it a correct understanding?

It definitely makes the code less complex. The additional memory is minimal. In many cases you have already some space wasted at the end of the object that could be used for the pointer.

Quoted 2 lines

Can you, please, describe a bit more detailed how it should be done from your point of view?

Add it to the metadata at the end of the object. Like the debugging information or the pointer for RCU freeing.

Review struct kmem_cache { · line 90

Quoted 7 lines · Roman Gushchin

enum stat_item {

(quoted diff) +1 -0

 	ALLOC_FASTPATH,		/* Allocation from cpu slab */

@@ -86,6 +87,7 @@ struct kmem_cache {
 	unsigned long min_partial;
 	unsigned int size;	/* The size of an object including metadata */
 	unsigned int object_size;/* The size of an object without metadata */
+	struct reciprocal_value reciprocal_size;

This needs to be moved further back since it is not an item that needs to be cache hot for the hotpaths. Place it with "align", inuse etc?

Hmmm. the same applies to min_partial maybe?

Something like this?

This commit implements SLUB version of the obj_to_index() function, which will be required to calculate the offset of obj_cgroup in the obj_cgroups vector to store/obtain the objcg ownership data.

To make it faster, let's repeat the SLAB's trick introduced by commit 6a2d7a955d8d ("[PATCH] SLAB: use a multiply instead of a divide in obj_to_index()") and avoid an expensive division.

Signed-off-by Roman Gushchin <[email protected]>

Acked-by Christoph Lameter <[email protected]>

Acked-by Johannes Weiner <[email protected]>

---
 include/linux/slub_def.h | 9 +++++++++
 mm/slub.c                | 1 +
 2 files changed, 10 insertions(+)

include/linux/slub_def.h +9 -0

index d2153789bd9f..200ea292f250 100644
@@ -8,6 +8,7 @@
  * (C) 2007 SGI, Christoph Lameter
  */
 #include <linux/kobject.h>
+#include <linux/reciprocal_div.h>
 
 enum stat_item {
 	ALLOC_FASTPATH,		/* Allocation from cpu slab */

@@ -86,6 +87,7 @@ struct kmem_cache {
 	unsigned long min_partial;
 	unsigned int size;	/* The size of an object including metadata */
 	unsigned int object_size;/* The size of an object without metadata */
+	struct reciprocal_value reciprocal_size;
 	unsigned int offset;	/* Free pointer offset */
 #ifdef CONFIG_SLUB_CPU_PARTIAL
 	/* Number of per cpu partial objects to keep around */

@@ -182,4 +184,11 @@ static inline void *nearest_obj(struct kmem_cache *cache, struct page *page,
 	return result;
 }
 
+static inline unsigned int obj_to_index(const struct kmem_cache *cache,
+					const struct page *page, void *obj)
+{
+	return reciprocal_divide(kasan_reset_tag(obj) - page_address(page),
+				 cache->reciprocal_size);
+}
+
 #endif /* _LINUX_SLUB_DEF_H */

mm/slub.c +1 -0

index 03071ae5ff07..8d16babe1829 100644
@@ -3660,6 +3660,7 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
 	 */
 	size = ALIGN(size, s->align);
 	s->size = size;
+	s->reciprocal_size = reciprocal_value(size);
 	if (forced_order >= 0)
 		order = forced_order;
 	else

Quoted 11 lines · Roman Gushchin

This commit implements SLUB version of the obj_to_index() function, which will be required to calculate the offset of obj_cgroup in the obj_cgroups vector to store/obtain the objcg ownership data.

To make it faster, let's repeat the SLAB's trick introduced by commit 6a2d7a955d8d ("[PATCH] SLAB: use a multiply instead of a divide in obj_to_index()") and avoid an expensive division.

Signed-off-by Roman Gushchin <[email protected]>

Acked-by Christoph Lameter <[email protected]>

Acked-by Johannes Weiner <[email protected]>

There's already a slab_index() doing the same without the trick, with only SLUB_DEBUG callers. Maybe just improve it and perhaps rename? (obj_to_index() seems more descriptive). The difference is that it takes the result of page_addr() instead of doing that, as it's being called in a loop on objects from a single page, so you'd have to perhaps split to obj_to_index(page) and __obj_to_index(addr) or something.

From beeaecdac85c3a395dcfb99944dc8c858b541cbf Mon Sep 17 00:00:00 2001
From: Roman Gushchin <[email protected]>
Date: Mon, 29 Jul 2019 18:18:42 -0700
Subject: [PATCH v3.2 04/19] mm: slub: implement SLUB version of obj_to_index()

This commit implements SLUB version of the obj_to_index() function, which will be required to calculate the offset of obj_cgroup in the obj_cgroups vector to store/obtain the objcg ownership data.

To make it faster, let's repeat the SLAB's trick introduced by commit 6a2d7a955d8d ("[PATCH] SLAB: use a multiply instead of a divide in obj_to_index()") and avoid an expensive division.

Vlastimil Babka noticed, that SLUB does have already a similar function called slab_index(), which is defined only if SLUB_DEBUG is enabled. The function does a similar math, but with a division, and it also takes a page address instead of a page pointer.

Let's remove slab_index() and replace it with the new helper __obj_to_index(), which takes a page address. obj_to_index() will be a simple wrapper taking a page pointer and passing page_address(page) into __obj_to_index().

Signed-off-by Roman Gushchin <[email protected]>

---
 include/linux/slub_def.h | 16 ++++++++++++++++
 mm/slub.c                | 15 +++++----------
 2 files changed, 21 insertions(+), 10 deletions(-)

include/linux/slub_def.h +16 -0

index d2153789bd9f..30e91c83d401 100644
@@ -8,6 +8,7 @@
  * (C) 2007 SGI, Christoph Lameter
  */
 #include <linux/kobject.h>
+#include <linux/reciprocal_div.h>
 
 enum stat_item {
 	ALLOC_FASTPATH,		/* Allocation from cpu slab */

@@ -86,6 +87,7 @@ struct kmem_cache {
 	unsigned long min_partial;
 	unsigned int size;	/* The size of an object including metadata */
 	unsigned int object_size;/* The size of an object without metadata */
+	struct reciprocal_value reciprocal_size;
 	unsigned int offset;	/* Free pointer offset */
 #ifdef CONFIG_SLUB_CPU_PARTIAL
 	/* Number of per cpu partial objects to keep around */

@@ -182,4 +184,18 @@ static inline void *nearest_obj(struct kmem_cache *cache, struct page *page,
 	return result;
 }
 
+/* Determine object index from a given position */
+static inline unsigned int __obj_to_index(const struct kmem_cache *cache,
+					  void *addr, void *obj)
+{
+	return reciprocal_divide(kasan_reset_tag(obj) - addr,
+				 cache->reciprocal_size);
+}
+
+static inline unsigned int obj_to_index(const struct kmem_cache *cache,
+					const struct page *page, void *obj)
+{
+	return __obj_to_index(cache, page_address(page), obj);
+}
+
 #endif /* _LINUX_SLUB_DEF_H */

mm/slub.c +5 -10

index 2df4d4a420d1..d605d18b3c1b 100644
@@ -313,12 +313,6 @@ static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp)
 		__p < (__addr) + (__objects) * (__s)->size; \
 		__p += (__s)->size)
 
-/* Determine object index from a given position */
-static inline unsigned int slab_index(void *p, struct kmem_cache *s, void *addr)
-{
-	return (kasan_reset_tag(p) - addr) / s->size;
-}
-
 static inline unsigned int order_objects(unsigned int order, unsigned int size)
 {
 	return ((unsigned int)PAGE_SIZE << order) / size;

@@ -461,7 +455,7 @@ static unsigned long *get_map(struct kmem_cache *s, struct page *page)
 	bitmap_zero(object_map, page->objects);
 
 	for (p = page->freelist; p; p = get_freepointer(s, p))
-		set_bit(slab_index(p, s, addr), object_map);
+		set_bit(__obj_to_index(s, addr, p), object_map);
 
 	return object_map;
 }

@@ -3682,6 +3676,7 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
 	 */
 	size = ALIGN(size, s->align);
 	s->size = size;
+	s->reciprocal_size = reciprocal_value(size);
 	if (forced_order >= 0)
 		order = forced_order;
 	else

@@ -3788,7 +3783,7 @@ static void list_slab_objects(struct kmem_cache *s, struct page *page,
 	map = get_map(s, page);
 	for_each_object(p, s, addr, page->objects) {
 
-		if (!test_bit(slab_index(p, s, addr), map)) {
+		if (!test_bit(__obj_to_index(s, addr, p), map)) {
 			pr_err("INFO: Object 0x%p @offset=%tu\n", p, p - addr);
 			print_tracking(s, p);
 		}

@@ -4513,7 +4508,7 @@ static void validate_slab(struct kmem_cache *s, struct page *page)
 	/* Now we know that a valid freelist exists */
 	map = get_map(s, page);
 	for_each_object(p, s, addr, page->objects) {
-		u8 val = test_bit(slab_index(p, s, addr), map) ?
+		u8 val = test_bit(__obj_to_index(s, addr, p), map) ?
 			 SLUB_RED_INACTIVE : SLUB_RED_ACTIVE;
 
 		if (!check_object(s, page, p, val))

@@ -4704,7 +4699,7 @@ static void process_slab(struct loc_track *t, struct kmem_cache *s,
 
 	map = get_map(s, page);
 	for_each_object(p, s, addr, page->objects)
-		if (!test_bit(slab_index(p, s, addr), map))
+		if (!test_bit(__obj_to_index(s, addr, p), map))
 			add_location(t, s, get_track(s, p, alloc));
 	put_map(map);
 }

Quoted 24 lines · Roman Gushchin

From beeaecdac85c3a395dcfb99944dc8c858b541cbf Mon Sep 17 00:00:00 2001
From: Roman Gushchin <[email protected]>
Date: Mon, 29 Jul 2019 18:18:42 -0700
Subject: [PATCH v3.2 04/19] mm: slub: implement SLUB version of obj_to_index()

This commit implements SLUB version of the obj_to_index() function, which will be required to calculate the offset of obj_cgroup in the obj_cgroups vector to store/obtain the objcg ownership data.

To make it faster, let's repeat the SLAB's trick introduced by commit 6a2d7a955d8d ("[PATCH] SLAB: use a multiply instead of a divide in obj_to_index()") and avoid an expensive division.

Vlastimil Babka noticed, that SLUB does have already a similar function called slab_index(), which is defined only if SLUB_DEBUG is enabled. The function does a similar math, but with a division, and it also takes a page address instead of a page pointer.

Let's remove slab_index() and replace it with the new helper __obj_to_index(), which takes a page address. obj_to_index() will be a simple wrapper taking a page pointer and passing page_address(page) into __obj_to_index().

Quoted 1 line

Signed-off-by Roman Gushchin <[email protected]>

Looks good!

Reviewed-by Vlastimil Babka <[email protected]>

mm/memcontrol.c +3 -0

@@ -257,6 +257,78 @@ struct cgroup_subsys_state *vmpressure_to_css(struct vmpressure *vmpr)
 }
 
 #ifdef CONFIG_MEMCG_KMEM
+extern spinlock_t css_set_lock;
+
+static void obj_cgroup_release(struct percpu_ref *ref)

mm/memcontrol.c +13 -0

@@ -2723,6 +2820,30 @@ struct mem_cgroup *mem_cgroup_from_obj(void *p)
 	return page->mem_cgroup;
 }
 
+__always_inline struct obj_cgroup *get_obj_cgroup_from_current(void)
+{
+	struct obj_cgroup *objcg = NULL;
+	struct mem_cgroup *memcg;
+
+	if (unlikely(!current->mm))
+		return NULL;
+
+	rcu_read_lock();
+	if (unlikely(current->active_memcg))
+		memcg = rcu_dereference(current->active_memcg);
+	else
+		memcg = mem_cgroup_from_task(current);

mm/memcontrol.c +11 -0

+int obj_cgroup_charge(struct obj_cgroup *objcg, gfp_t gfp, size_t size)
+{
+	struct mem_cgroup *memcg;
+	unsigned int nr_pages, nr_bytes;
+	int ret;
+
+	if (consume_obj_stock(objcg, size))
+		return 0;
+
+	rcu_read_lock();
+	memcg = obj_cgroup_memcg(objcg);

Quoted 15 lines · Roman Gushchin

mm/memcontrol.c +9 -0

@@ -257,6 +257,78 @@ struct cgroup_subsys_state *vmpressure_to_css(struct vmpressure *vmpr)
 }
 
 #ifdef CONFIG_MEMCG_KMEM
+extern spinlock_t css_set_lock;
+
+static void obj_cgroup_release(struct percpu_ref *ref)
+{
+	struct obj_cgroup *objcg = container_of(ref, struct obj_cgroup, refcnt);
+	struct mem_cgroup *memcg;
+	unsigned int nr_bytes;
+	unsigned int nr_pages;
+	unsigned long flags;

mm/memcontrol.c struct cgroup_subsys_state *vmpressure_to_css(struct vmpressure *vmpr) · line 272

Quoted 4 lines

mm/memcontrol.c +4 -0

+
+	nr_bytes = atomic_read(&objcg->nr_charged_bytes);
+	WARN_ON_ONCE(nr_bytes & (PAGE_SIZE - 1));
+	nr_pages = nr_bytes >> PAGE_SHIFT;

What guarantees that we don't have a partial page in there at this point? I guess any outstanding allocations would pin the objcg, so when it's released all objects have been freed.

But if that's true, how can we have full pages remaining in there now?

Quoted 18 lines

mm/memcontrol.c +13 -0

@@ -2723,6 +2820,30 @@ struct mem_cgroup *mem_cgroup_from_obj(void *p)
 	return page->mem_cgroup;
 }
 
+__always_inline struct obj_cgroup *get_obj_cgroup_from_current(void)
+{
+	struct obj_cgroup *objcg = NULL;
+	struct mem_cgroup *memcg;
+
+	if (unlikely(!current->mm))
+		return NULL;
+
+	rcu_read_lock();
+	if (unlikely(current->active_memcg))
+		memcg = rcu_dereference(current->active_memcg);
+	else
+		memcg = mem_cgroup_from_task(current);

mm/memcontrol.c struct mem_cgroup *mem_cgroup_from_obj(void *p) · line 2845

Quoted 10 lines

mm/memcontrol.c +10 -0

+
+	for (; memcg != root_mem_cgroup; memcg = parent_mem_cgroup(memcg)) {
+		objcg = rcu_dereference(memcg->objcg);
+		if (objcg && obj_cgroup_tryget(objcg))
+			break;
+	}
+	rcu_read_unlock();
+
+	return objcg;
+}

Thanks for moving this here from one of the later patches, it helps understanding the life cycle of obj_cgroup better.

Quoted 17 lines

mm/memcontrol.c +17 -0

+int obj_cgroup_charge(struct obj_cgroup *objcg, gfp_t gfp, size_t size)
+{
+	struct mem_cgroup *memcg;
+	unsigned int nr_pages, nr_bytes;
+	int ret;
+
+	if (consume_obj_stock(objcg, size))
+		return 0;
+
+	rcu_read_lock();
+	memcg = obj_cgroup_memcg(objcg);
+	css_get(&memcg->css);
+	rcu_read_unlock();
+
+	nr_pages = size >> PAGE_SHIFT;
+	nr_bytes = size & (PAGE_SIZE - 1);
+

mm/memcontrol.c void __memcg_kmem_uncharge_page(struct page *page, int order) · line 3232

Quoted 4 lines

mm/memcontrol.c +4 -0

+	if (nr_bytes)
+		nr_pages += 1;
+
+	ret = __memcg_kmem_charge(memcg, gfp, nr_pages);

If consume_obj_stock() fails because some other memcg is cached, should this try to consume the partial page in objcg->nr_charged_bytes before getting more pages?

mm/memcontrol.c void __memcg_kmem_uncharge_page(struct page *page, int order) · line 3234

Quoted 3 lines

mm/memcontrol.c +2 -0

+	if (!ret && nr_bytes)
+		refill_obj_stock(objcg, PAGE_SIZE - nr_bytes);

This will put the cgroup into the cache if the allocation resulted in a partially free page.

But if this was a page allocation, we may have objcg->nr_cache_bytes from a previous subpage allocation that we should probably put back into the stock.

It's not a common case, I'm just trying to understand what objcg->nr_cache_bytes holds and when it does so.

The rest of this patch looks good to me!

Quoted 16 lines · Johannes Weiner

Quoted 15 lines · Roman Gushchin

mm/memcontrol.c +9 -0

@@ -257,6 +257,78 @@ struct cgroup_subsys_state *vmpressure_to_css(struct vmpressure *vmpr)
 }
 
 #ifdef CONFIG_MEMCG_KMEM
+extern spinlock_t css_set_lock;
+
+static void obj_cgroup_release(struct percpu_ref *ref)
+{
+	struct obj_cgroup *objcg = container_of(ref, struct obj_cgroup, refcnt);
+	struct mem_cgroup *memcg;
+	unsigned int nr_bytes;
+	unsigned int nr_pages;
+	unsigned long flags;

Quoted 8 lines

mm/memcontrol.c struct cgroup_subsys_state *vmpressure_to_css(struct vmpressure *vmpr) · line 272

Quoted 4 lines

mm/memcontrol.c +4 -0

+
+	nr_bytes = atomic_read(&objcg->nr_charged_bytes);
+	WARN_ON_ONCE(nr_bytes & (PAGE_SIZE - 1));
+	nr_pages = nr_bytes >> PAGE_SHIFT;

What guarantees that we don't have a partial page in there at this point? I guess any outstanding allocations would pin the objcg, so when it's released all objects have been freed.

Right, this is exactly the reason why there can't be a partial page at this point.

Quoted 2 lines

But if that's true, how can we have full pages remaining in there now?

Imagine the following sequence:
1) CPU0: objcg == stock->cached_objcg
2) CPU1: we do a small allocation (e.g. 92 bytes), page is charged
3) CPU1: a process from another memcg is allocating something, stock if flushed, objcg->nr_charged_bytes = PAGE_SIZE - 92
5) CPU0: we do release this object, 92 bytes are added to stock->nr_bytes
6) CPU0: stock is flushed, 92 bytes are added to objcg->nr_charged_bytes

In the result, nr_charged_bytes == PAGE_SIZE. This PAGE will be uncharged in obj_cgroup_release().

I've double checked this, it's actually pretty easy to trigger in the real life.

Quoted 17 lines

Quoted 17 lines

mm/memcontrol.c +17 -0

+int obj_cgroup_charge(struct obj_cgroup *objcg, gfp_t gfp, size_t size)
+{
+	struct mem_cgroup *memcg;
+	unsigned int nr_pages, nr_bytes;
+	int ret;
+
+	if (consume_obj_stock(objcg, size))
+		return 0;
+
+	rcu_read_lock();
+	memcg = obj_cgroup_memcg(objcg);
+	css_get(&memcg->css);
+	rcu_read_unlock();
+
+	nr_pages = size >> PAGE_SHIFT;
+	nr_bytes = size & (PAGE_SIZE - 1);
+

Quoted 8 lines

mm/memcontrol.c void __memcg_kmem_uncharge_page(struct page *page, int order) · line 3232

Quoted 4 lines

mm/memcontrol.c +4 -0

+	if (nr_bytes)
+		nr_pages += 1;
+
+	ret = __memcg_kmem_charge(memcg, gfp, nr_pages);

If consume_obj_stock() fails because some other memcg is cached, should this try to consume the partial page in objcg->nr_charged_bytes before getting more pages?

We can definitely do it, but I'm not sure if it's good for the performance.

Dealing with nr_charged_bytes will require up to two atomic writes, so calling __memcg_kmem_charge() can be faster if memcg is cached on percpu stock.

Quoted 9 lines

mm/memcontrol.c void __memcg_kmem_uncharge_page(struct page *page, int order) · line 3234

Quoted 2 lines

mm/memcontrol.c +2 -0

+	if (!ret && nr_bytes)
+		refill_obj_stock(objcg, PAGE_SIZE - nr_bytes);

This will put the cgroup into the cache if the allocation resulted in a partially free page.

But if this was a page allocation, we may have objcg->nr_cache_bytes from a previous subpage allocation that we should probably put back into the stock.

Yeah, we can do this, but I don't know if there will be any benefits.

Actually we don't wanna to touch objcg->nr_cache_bytes too often, as it can become a contention point if there are many threads allocating in the memory cgroup.

So maybe we want to do the opposite: relax it a bit and stop flushing it on every stock refill and flush only if it exceeds a certain value.

Quoted 2 lines

It's not a common case, I'm just trying to understand what objcg->nr_cache_bytes holds and when it does so.

So it's actually a centralized leftover from the rounding of the actual charge to the page size.

Quoted 40 lines · Roman Gushchin

Quoted 16 lines · Johannes Weiner

Quoted 15 lines · Roman Gushchin

mm/memcontrol.c +9 -0

@@ -257,6 +257,78 @@ struct cgroup_subsys_state *vmpressure_to_css(struct vmpressure *vmpr)
 }
 
 #ifdef CONFIG_MEMCG_KMEM
+extern spinlock_t css_set_lock;
+
+static void obj_cgroup_release(struct percpu_ref *ref)
+{
+	struct obj_cgroup *objcg = container_of(ref, struct obj_cgroup, refcnt);
+	struct mem_cgroup *memcg;
+	unsigned int nr_bytes;
+	unsigned int nr_pages;
+	unsigned long flags;

Quoted 8 lines

mm/memcontrol.c struct cgroup_subsys_state *vmpressure_to_css(struct vmpressure *vmpr) · line 272

Quoted 4 lines

mm/memcontrol.c +4 -0

+
+	nr_bytes = atomic_read(&objcg->nr_charged_bytes);
+	WARN_ON_ONCE(nr_bytes & (PAGE_SIZE - 1));
+	nr_pages = nr_bytes >> PAGE_SHIFT;

What guarantees that we don't have a partial page in there at this point? I guess any outstanding allocations would pin the objcg, so when it's released all objects have been freed.

Right, this is exactly the reason why there can't be a partial page at this point.

Quoted 2 lines

But if that's true, how can we have full pages remaining in there now?

Imagine the following sequence:
1) CPU0: objcg == stock->cached_objcg
2) CPU1: we do a small allocation (e.g. 92 bytes), page is charged
3) CPU1: a process from another memcg is allocating something, stock if flushed, objcg->nr_charged_bytes = PAGE_SIZE - 92
5) CPU0: we do release this object, 92 bytes are added to stock->nr_bytes
6) CPU0: stock is flushed, 92 bytes are added to objcg->nr_charged_bytes

Quoted 4 lines

In the result, nr_charged_bytes == PAGE_SIZE. This PAGE will be uncharged in obj_cgroup_release().

I've double checked this, it's actually pretty easy to trigger in the real life.

Ah, so no outstanding allocations, but a full page split between the percpu cache and objcg->nr_charged_bytes.

Would it simplify things if refill_obj_stock() drained on >= PAGE_SIZE stock instead of > PAGE_SIZE?

Otherwise, the scenario above would be good to have as a comment as the drain on release is not self-explanatory.

Quoted 26 lines

Quoted 17 lines

Quoted 17 lines

mm/memcontrol.c +17 -0

+int obj_cgroup_charge(struct obj_cgroup *objcg, gfp_t gfp, size_t size)
+{
+	struct mem_cgroup *memcg;
+	unsigned int nr_pages, nr_bytes;
+	int ret;
+
+	if (consume_obj_stock(objcg, size))
+		return 0;
+
+	rcu_read_lock();
+	memcg = obj_cgroup_memcg(objcg);
+	css_get(&memcg->css);
+	rcu_read_unlock();
+
+	nr_pages = size >> PAGE_SHIFT;
+	nr_bytes = size & (PAGE_SIZE - 1);
+

Quoted 8 lines

mm/memcontrol.c struct cgroup_subsys_state *vmpressure_to_css(struct vmpressure *vmpr) · line 293

Quoted 4 lines

mm/memcontrol.c +4 -0

+	if (nr_bytes)
+		nr_pages += 1;
+
+	ret = __memcg_kmem_charge(memcg, gfp, nr_pages);

If consume_obj_stock() fails because some other memcg is cached, should this try to consume the partial page in objcg->nr_charged_bytes before getting more pages?

Quoted 5 lines

We can definitely do it, but I'm not sure if it's good for the performance.

Dealing with nr_charged_bytes will require up to two atomic writes, so calling __memcg_kmem_charge() can be faster if memcg is cached on percpu stock.

Hm, but it's the slowpath. And sooner or later somebody has to deal with the remaining memory in there.

Quoted 11 lines

Quoted 9 lines

mm/memcontrol.c struct cgroup_subsys_state *vmpressure_to_css(struct vmpressure *vmpr) · line 295

Quoted 2 lines

mm/memcontrol.c +2 -0

+	if (!ret && nr_bytes)
+		refill_obj_stock(objcg, PAGE_SIZE - nr_bytes);

This will put the cgroup into the cache if the allocation resulted in a partially free page.

But if this was a page allocation, we may have objcg->nr_cache_bytes from a previous subpage allocation that we should probably put back into the stock.

Yeah, we can do this, but I don't know if there will be any benefits.

It's mostly about understanding the code.

Quoted 6 lines

Actually we don't wanna to touch objcg->nr_cache_bytes too often, as it can become a contention point if there are many threads allocating in the memory cgroup.

So maybe we want to do the opposite: relax it a bit and stop flushing it on every stock refill and flush only if it exceeds a certain value.

That could be useful, yes.

Quoted 5 lines

Quoted 2 lines

It's not a common case, I'm just trying to understand what objcg->nr_cache_bytes holds and when it does so.

So it's actually a centralized leftover from the rounding of the actual charge to the page size.

It would be good to add code comments explaining this.

Allocate and release memory to store obj_cgroup pointers for each non-root slab page. Reuse page->mem_cgroup pointer to store a pointer to the allocated space.

To distinguish between obj_cgroups and memcg pointers in case when it's not obvious which one is used (as in page_cgroup_ino()), let's always set the lowest bit in the obj_cgroup case.

Signed-off-by Roman Gushchin <[email protected]>

---
 include/linux/mm_types.h |  5 ++++-
 include/linux/slab_def.h |  5 +++++
 include/linux/slub_def.h |  2 ++
 mm/memcontrol.c          | 17 +++++++++++---
 mm/slab.c                |  3 ++-
 mm/slab.h                | 48 ++++++++++++++++++++++++++++++++++++++++
 mm/slub.c                |  5 +++++
 7 files changed, 80 insertions(+), 5 deletions(-)

mm/slub.c +5 -0

index 8d16babe1829..68c2c45dfac1 100644
@@ -5992,4 +5992,9 @@ ssize_t slabinfo_write(struct file *file, const char __user *buffer,
 {
 	return -EIO;
 }
+
+int objs_per_slab(struct kmem_cache *cache)
+{
+	return oo_objects(cache->oo);
+}
 #endif /* CONFIG_SLUB_DEBUG */

Signature · 1 line

2.25.3

Allocate and release memory to store obj_cgroup pointers for each non-root slab page. Reuse page->mem_cgroup pointer to store a pointer to the allocated space.

To distinguish between obj_cgroups and memcg pointers in case when it's not obvious which one is used (as in page_cgroup_ino()), let's always set the lowest bit in the obj_cgroup case.

Signed-off-by Roman Gushchin <[email protected]>

include/linux/slub_def.h +2 -0

@@ -191,4 +191,6 @@ static inline unsigned int obj_to_index(const struct kmem_cache *cache,
 				 cache->reciprocal_size);
 }
 
+extern int objs_per_slab(struct kmem_cache *cache);
+
 #endif /* _LINUX_SLUB_DEF_H */

mm/memcontrol.c +0 -0

index 7f87a0eeafec..63826e460b3f 100644

Quoted 4 lines · Roman Gushchin

Allocate and release memory to store obj_cgroup pointers for each non-root slab page. Reuse page->mem_cgroup pointer to store a pointer to the allocated space.

Quoted 5 lines

To distinguish between obj_cgroups and memcg pointers in case when it's not obvious which one is used (as in page_cgroup_ino()), let's always set the lowest bit in the obj_cgroup case.

Signed-off-by Roman Gushchin <[email protected]>

Reviewed-by Vlastimil Babka <[email protected]>

But I have a suggestion:

...

Quoted 11 lines

include/linux/slub_def.h +2 -0

@@ -191,4 +191,6 @@ static inline unsigned int obj_to_index(const struct kmem_cache *cache,
 				 cache->reciprocal_size);
 }
 
+extern int objs_per_slab(struct kmem_cache *cache);
+
 #endif /* _LINUX_SLUB_DEF_H */

mm/memcontrol.c +0 -0

index 7f87a0eeafec..63826e460b3f 100644

...

mm/slub.c ssize_t slabinfo_write(struct file *file, const char __user *buffer, · line 6000

Quoted 13 lines

mm/slub.c +5 -0

@@ -5992,4 +5992,9 @@ ssize_t slabinfo_write(struct file *file, const char __user *buffer,
 {
 	return -EIO;
 }
+
+int objs_per_slab(struct kmem_cache *cache)
+{
+	return oo_objects(cache->oo);
+}
 #endif /* CONFIG_SLUB_DEBUG */

It's somewhat unfortunate to function call just for this. Although perhaps compiler can be smart enough as charge_slab_page() (that callse objs_per_slab()) is inline and called from alloc_slab_page() which is also in mm/slub.c.

But it might be also a bit wasteful in case SLUB doesn't manage to allocate its desired order, but smaller. The actual number of objects is then in page->objects.

So ideally this should use something like objs_per_slab_page(cache, page) where SLAB supplies cache->num and SLUB page->objects, both implementations inline, and ignoring the other parameter?

Quoted 9 lines · Roman Gushchin

Allocate and release memory to store obj_cgroup pointers for each non-root slab page. Reuse page->mem_cgroup pointer to store a pointer to the allocated space.

To distinguish between obj_cgroups and memcg pointers in case when it's not obvious which one is used (as in page_cgroup_ino()), let's always set the lowest bit in the obj_cgroup case.

Signed-off-by Roman Gushchin <[email protected]>

Reviewed-by Vlastimil Babka <[email protected]>

But I have a suggestion:

...

Quoted 11 lines

include/linux/slub_def.h +2 -0

@@ -191,4 +191,6 @@ static inline unsigned int obj_to_index(const struct kmem_cache *cache,
 				 cache->reciprocal_size);
 }
 
+extern int objs_per_slab(struct kmem_cache *cache);
+
 #endif /* _LINUX_SLUB_DEF_H */

mm/memcontrol.c +0 -0

index 7f87a0eeafec..63826e460b3f 100644

...

mm/slub.c ssize_t slabinfo_write(struct file *file, const char __user *buffer, · line 6000

Quoted 13 lines

mm/slub.c +5 -0

@@ -5992,4 +5992,9 @@ ssize_t slabinfo_write(struct file *file, const char __user *buffer,
 {
 	return -EIO;
 }
+
+int objs_per_slab(struct kmem_cache *cache)
+{
+	return oo_objects(cache->oo);
+}
 #endif /* CONFIG_SLUB_DEBUG */

It's somewhat unfortunate to function call just for this. Although perhaps compiler can be smart enough as charge_slab_page() (that callse objs_per_slab()) is inline and called from alloc_slab_page() which is also in mm/slub.c.

But it might be also a bit wasteful in case SLUB doesn't manage to allocate its desired order, but smaller. The actual number of objects is then in page->objects.

So ideally this should use something like objs_per_slab_page(cache, page) where SLAB supplies cache->num and SLUB page->objects, both implementations inline, and ignoring the other parameter?