slub: make dead caches discard free slabs immediately

PATCH In review Vladimir Davydov 8 participants 26 messages Jan 28, 2015 – Apr 1, 2016

Subsystems (2)

Memory management Slab allocator

lore

Share & tools

lore

Vladimir Davydov

author

2015-01-28 16:23 UTC #0 lore raw

v2 cover slub: make dead caches discard free slabs immediately

Hi,

The kmem extension of the memory cgroup is almost usable now. There is, in fact, the only serious issue left: per memcg kmem caches may pin the owner cgroup for indefinitely long. This is, because a slab cache may keep empty slab pages in its private structures to optimize performance, while we take a css reference per each charged kmem page.

The issue is only relevant to SLUB, because SLAB periodically reaps empty slabs. This patch set fixes this issue for SLUB. For details, please see patch 3.

Changes in v2:
- address Christoph's concerns regarding kmem_cache_shrink
- fix race between put_cpu_partial reading ->cpu_partial and kmem_cache_shrink updating it as proposed by Joonsoo

v1: https://lkml.org/lkml/2015/1/26/317

Thanks,

Vladimir Davydov (3):
slub: never fail to shrink cache
slub: fix kmem_cache_shrink return value
slub: make dead caches discard free slabs immediately

 mm/slab.c        |    4 +--
 mm/slab.h        |    2 +-
 mm/slab_common.c |   15 +++++++--
 mm/slob.c        |    2 +-
 mm/slub.c        |   94 +++++++++++++++++++++++++++++++++++-------------------
 5 files changed, 78 insertions(+), 39 deletions(-)

Signature · 7 lines

1.7.10.4

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

Vladimir Davydov

author

2015-01-28 16:23 UTC #1 lore raw

PATCH v2 1/3 slub: never fail to shrink cache

SLUB's version of __kmem_cache_shrink() not only removes empty slabs, but also tries to rearrange the partial lists to place slabs filled up most to the head to cope with fragmentation. To achieve that, it allocates a temporary array of lists used to sort slabs by the number of objects in use. If the allocation fails, the whole procedure is aborted.

This is unacceptable for the kernel memory accounting extension of the memory cgroup, where we want to make sure that kmem_cache_shrink() successfully discarded empty slabs. Although the allocation failure is utterly unlikely with the current page allocator implementation, which retries GFP_KERNEL allocations of order <= 2 infinitely, it is better not to rely on that.

This patch therefore makes __kmem_cache_shrink() allocate the array on stack instead of calling kmalloc, which may fail. The array size is chosen to be equal to 32, because most SLUB caches store not more than 32 objects per slab page. Slab pages with <= 32 free objects are sorted using the array by the number of objects in use and promoted to the head of the partial list, while slab pages with > 32 free objects are left in the end of the list without any ordering imposed on them.

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

---
 mm/slub.c |   57 ++++++++++++++++++++++++++++++---------------------------
 1 file changed, 30 insertions(+), 27 deletions(-)

mm/slub.c +30 ~~-27~~

index 1562955fe099..dbf9334b6a5c 100644
@@ -3358,11 +3358,12 @@ void kfree(const void *x)
 }
 EXPORT_SYMBOL(kfree);
 
+#define SHRINK_PROMOTE_MAX 32
+
 /*
- * kmem_cache_shrink removes empty slabs from the partial lists and sorts
- * the remaining slabs by the number of items in use. The slabs with the
- * most items in use come first. New allocations will then fill those up
- * and thus they can be removed from the partial lists.
+ * kmem_cache_shrink discards empty slabs and promotes the slabs filled
+ * up most to the head of the partial lists. New allocations will then
+ * fill those up and thus they can be removed from the partial lists.
  *
  * The slabs with the least items are placed last. This results in them
  * being allocated from last increasing the chance that the last objects

@@ -3375,51 +3376,56 @@ int __kmem_cache_shrink(struct kmem_cache *s)
 	struct kmem_cache_node *n;
 	struct page *page;
 	struct page *t;
-	int objects = oo_objects(s->max);
-	struct list_head *slabs_by_inuse =
-		kmalloc(sizeof(struct list_head) * objects, GFP_KERNEL);
+	LIST_HEAD(discard);
+	struct list_head promote[SHRINK_PROMOTE_MAX];
 	unsigned long flags;
 
-	if (!slabs_by_inuse)
-		return -ENOMEM;
+	for (i = 0; i < SHRINK_PROMOTE_MAX; i++)
+		INIT_LIST_HEAD(promote + i);
 
 	flush_all(s);
 	for_each_kmem_cache_node(s, node, n) {
 		if (!n->nr_partial)
 			continue;
 
-		for (i = 0; i < objects; i++)
-			INIT_LIST_HEAD(slabs_by_inuse + i);
-
 		spin_lock_irqsave(&n->list_lock, flags);
 
 		/*
-		 * Build lists indexed by the items in use in each slab.
+		 * Build lists of slabs to discard or promote.
 		 *
 		 * Note that concurrent frees may occur while we hold the
 		 * list_lock. page->inuse here is the upper limit.
 		 */
 		list_for_each_entry_safe(page, t, &n->partial, lru) {
-			list_move(&page->lru, slabs_by_inuse + page->inuse);
-			if (!page->inuse)
+			int free = page->objects - page->inuse;
+
+			/* Do not reread page->inuse */
+			barrier();
+
+			/* We do not keep full slabs on the list */
+			BUG_ON(free <= 0);
+
+			if (free == page->objects) {
+				list_move(&page->lru, &discard);
 				n->nr_partial--;
+			} else if (free <= SHRINK_PROMOTE_MAX)
+				list_move(&page->lru, promote + free - 1);
 		}
 
 		/*
-		 * Rebuild the partial list with the slabs filled up most
-		 * first and the least used slabs at the end.
+		 * Promote the slabs filled up most to the head of the
+		 * partial list.
 		 */
-		for (i = objects - 1; i > 0; i--)
-			list_splice(slabs_by_inuse + i, n->partial.prev);
+		for (i = SHRINK_PROMOTE_MAX - 1; i >= 0; i--)
+			list_splice_init(promote + i, &n->partial);
 
 		spin_unlock_irqrestore(&n->list_lock, flags);
 
 		/* Release empty slabs */
-		list_for_each_entry_safe(page, t, slabs_by_inuse, lru)
+		list_for_each_entry_safe(page, t, &discard, lru)
 			discard_slab(s, page);
 	}
 
-	kfree(slabs_by_inuse);
 	return 0;
 }

@@ -4682,12 +4688,9 @@ static ssize_t shrink_show(struct kmem_cache *s, char *buf)
 static ssize_t shrink_store(struct kmem_cache *s,
 			const char *buf, size_t length)
 {
-	if (buf[0] == '1') {
-		int rc = kmem_cache_shrink(s);
-
-		if (rc)
-			return rc;
-	} else
+	if (buf[0] == '1')
+		kmem_cache_shrink(s);
+	else
 		return -EINVAL;
 	return length;
 }

Signature · 7 lines

1.7.10.4

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

Christoph Lameter

Acked-by on v2 1/3

2015-01-28 16:31 UTC #2 lore raw

Re: [PATCH -mm v2 1/3] slub: never fail to shrink cache

Quoted 7 lines · Vladimir Davydov

This patch therefore makes __kmem_cache_shrink() allocate the array on stack instead of calling kmalloc, which may fail. The array size is chosen to be equal to 32, because most SLUB caches store not more than 32 objects per slab page. Slab pages with <= 32 free objects are sorted using the array by the number of objects in use and promoted to the head of the partial list, while slab pages with > 32 free objects are left in the end of the list without any ordering imposed on them.

Acked-by Christoph Lameter <[email protected]>

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

Pekka Enberg

Acked-by

2015-01-28 18:29 UTC #3 lore raw

Re: [PATCH -mm v2 1/3] slub: never fail to shrink cache

Quoted 10 lines · Christoph Lameter

Quoted 7 lines · Vladimir Davydov
This patch therefore makes __kmem_cache_shrink() allocate the array on stack instead of calling kmalloc, which may fail. The array size is chosen to be equal to 32, because most SLUB caches store not more than 32 objects per slab page. Slab pages with <= 32 free objects are sorted using the array by the number of objects in use and promoted to the head of the partial list, while slab pages with > 32 free objects are left in the end of the list without any ordering imposed on them.
Acked-by Christoph Lameter <[email protected]>

Acked-by Pekka Enberg <[email protected]>

Christoph Lameter

2015-01-28 16:37 UTC #4 lore raw

Re: [PATCH -mm v2 1/3] slub: never fail to shrink cache

In reply to Vladimir Davydov

mm/slub.c int __kmem_cache_shrink(struct kmem_cache *s) · line 3406

Quoted 2 lines · Vladimir Davydov

mm/slub.c +2 -0

+			/* We do not keep full slabs on the list */
+			BUG_ON(free <= 0);

Well sorry we do actually keep a number of empty slabs on the partial lists. See the min_partial field in struct kmem_cache.

Vladimir Davydov

author

2015-01-28 17:32 UTC #5 lore raw

Re: [PATCH -mm v2 1/3] slub: never fail to shrink cache

Quoted 7 lines · Christoph Lameter

mm/slub.c int __kmem_cache_shrink(struct kmem_cache *s) · line 3406
Quoted 2 lines · Vladimir Davydov
mm/slub.c +2 -0
+			/* We do not keep full slabs on the list */
+			BUG_ON(free <= 0);
Well sorry we do actually keep a number of empty slabs on the partial lists. See the min_partial field in struct kmem_cache.

It's not about empty slabs, it's about full slabs: free == 0 means slab is full.

Thanks,
Vladimir

Christoph Lameter

2015-01-28 19:20 UTC #6 lore raw

Re: [PATCH -mm v2 1/3] slub: never fail to shrink cache

Quoted 11 lines · Vladimir Davydov

Quoted 7 lines · Christoph Lameter
mm/slub.c int __kmem_cache_shrink(struct kmem_cache *s) · line 3406
Quoted 2 lines · Vladimir Davydov
mm/slub.c +2 -0
+			/* We do not keep full slabs on the list */
+			BUG_ON(free <= 0);
Well sorry we do actually keep a number of empty slabs on the partial lists. See the min_partial field in struct kmem_cache.
It's not about empty slabs, it's about full slabs: free == 0 means slab is full.

Correct. I already acked the patch.

Andrew Morton

maintainer

2015-01-28 21:57 UTC #7 lore raw

Re: [PATCH -mm v2 1/3] slub: never fail to shrink cache

In reply to Vladimir Davydov

Quoted 24 lines · On Wed, 28 Jan 2015 19:22:49 +0300 Vladimir Davydov <[email protected]>…

SLUB's version of __kmem_cache_shrink() not only removes empty slabs, but also tries to rearrange the partial lists to place slabs filled up most to the head to cope with fragmentation. To achieve that, it allocates a temporary array of lists used to sort slabs by the number of objects in use. If the allocation fails, the whole procedure is aborted.
This is unacceptable for the kernel memory accounting extension of the memory cgroup, where we want to make sure that kmem_cache_shrink() successfully discarded empty slabs. Although the allocation failure is utterly unlikely with the current page allocator implementation, which retries GFP_KERNEL allocations of order <= 2 infinitely, it is better not to rely on that.
This patch therefore makes __kmem_cache_shrink() allocate the array on stack instead of calling kmalloc, which may fail. The array size is chosen to be equal to 32, because most SLUB caches store not more than 32 objects per slab page. Slab pages with <= 32 free objects are sorted using the array by the number of objects in use and promoted to the head of the partial list, while slab pages with > 32 free objects are left in the end of the list without any ordering imposed on them.
...

mm/slub.c int __kmem_cache_shrink(struct kmem_cache *s) · line 3380

Quoted 8 lines

mm/slub.c +2 -3

@@ -3375,51 +3376,56 @@ int __kmem_cache_shrink(struct kmem_cache *s)
 	struct kmem_cache_node *n;
 	struct page *page;
 	struct page *t;
-	int objects = oo_objects(s->max);
-	struct list_head *slabs_by_inuse =
-		kmalloc(sizeof(struct list_head) * objects, GFP_KERNEL);
+	LIST_HEAD(discard);
+	struct list_head promote[SHRINK_PROMOTE_MAX];

512 bytes of stack. The call paths leading to __kmem_cache_shrink() are many and twisty. How do we know this isn't a problem?

The logic behind choosing "32" sounds rather rubbery. What goes wrong if we use, say, "4"?

Christoph Lameter

2015-01-28 22:56 UTC #8 lore raw

Re: [PATCH -mm v2 1/3] slub: never fail to shrink cache

Quoted 2 lines · Andrew Morton

The logic behind choosing "32" sounds rather rubbery. What goes wrong if we use, say, "4"?

Like more fragmentation of slab pages.

Vladimir Davydov

author

2015-01-29 08:07 UTC #9 lore raw

Re: [PATCH -mm v2 1/3] slub: never fail to shrink cache

In reply to Andrew Morton

Quoted 13 lines · Andrew Morton

mm/slub.c int __kmem_cache_shrink(struct kmem_cache *s) · line 3380
Quoted 9 lines · On Wed, 28 Jan 2015 19:22:49 +0300 Vladimir Davydov <[email protected]>…
mm/slub.c +2 -3
@@ -3375,51 +3376,56 @@ int __kmem_cache_shrink(struct kmem_cache *s)
 	struct kmem_cache_node *n;
 	struct page *page;
 	struct page *t;
-	int objects = oo_objects(s->max);
-	struct list_head *slabs_by_inuse =
-		kmalloc(sizeof(struct list_head) * objects, GFP_KERNEL);
+	LIST_HEAD(discard);
+	struct list_head promote[SHRINK_PROMOTE_MAX];
512 bytes of stack. The call paths leading to __kmem_cache_shrink() are many and twisty. How do we know this isn't a problem?

Because currently __kmem_cache_shrink is only called just from a couple of places, each of which isn't supposed to have a great stack depth AFAIU, namely:

- slab_mem_going_offline_callback - MEM_GOING_OFFLINE handler
- shrink_store - invoked upon write to /sys/kernel/slab/cache/shrink
- acpi_os_purge_cache - only called on acpi init
- memcg_deactivate_kmem_caches - called from cgroup_destroy_wq

Quoted 2 lines

The logic behind choosing "32" sounds rather rubbery. What goes wrong if we use, say, "4"?

We could, but kmem_cache_shrink would cope with fragmentation less efficiently.

Come to think of it, do we really need to optimize slab placement in kmem_cache_shrink? None of its users except shrink_store expects it - they just want to purge the cache before destruction, that's it. May be, we'd better move slab placement optimization to a separate SLUB's private function that would be called only by shrink_store, where we can put up with kmalloc failures? Christoph, what do you think?

Thanks,
Vladimir

Christoph Lameter

2015-01-29 15:56 UTC #10 lore raw

Re: [PATCH -mm v2 1/3] slub: never fail to shrink cache

Quoted 6 lines · Vladimir Davydov

Come to think of it, do we really need to optimize slab placement in kmem_cache_shrink? None of its users except shrink_store expects it - they just want to purge the cache before destruction, that's it. May be, we'd better move slab placement optimization to a separate SLUB's private function that would be called only by shrink_store, where we can put up with kmalloc failures? Christoph, what do you think?

The slabinfo tool invokes kmem_cache_shrink to optimize placement.

Run

slabinfo -s

which can then be used to reduce the fragmentation.

Vladimir Davydov

author

2015-01-29 16:17 UTC #11 lore raw

Re: [PATCH -mm v2 1/3] slub: never fail to shrink cache

Quoted 9 lines · Christoph Lameter

Quoted 6 lines · Vladimir Davydov
Come to think of it, do we really need to optimize slab placement in kmem_cache_shrink? None of its users except shrink_store expects it - they just want to purge the cache before destruction, that's it. May be, we'd better move slab placement optimization to a separate SLUB's private function that would be called only by shrink_store, where we can put up with kmalloc failures? Christoph, what do you think?

Quoted 7 lines

The slabinfo tool invokes kmem_cache_shrink to optimize placement.
Run
slabinfo -s
which can then be used to reduce the fragmentation.

Yeah, but the tool just writes 1 to /sys/kernel/slab/cache/shrink, i.e. invokes shrink_store(), and I don't propose to remove slab placement optimization from there. What I propose is to move slab placement optimization from kmem_cache_shrink() to shrink_store(), because other users of kmem_cache_shrink() don't seem to need it at all - they just want to release empty slabs. Such a change wouldn't affect the behavior of slabinfo -s at all.

Thanks,
Vladimir

Christoph Lameter

2015-01-29 16:22 UTC #12 lore raw

Re: [PATCH -mm v2 1/3] slub: never fail to shrink cache

Quoted 7 lines · Vladimir Davydov

Yeah, but the tool just writes 1 to /sys/kernel/slab/cache/shrink, i.e. invokes shrink_store(), and I don't propose to remove slab placement optimization from there. What I propose is to move slab placement optimization from kmem_cache_shrink() to shrink_store(), because other users of kmem_cache_shrink() don't seem to need it at all - they just want to release empty slabs. Such a change wouldn't affect the behavior of slabinfo -s at all.

Well we have to go through the chain of partial slabs anyways so its easy to do the optimization at that point.

Vladimir Davydov

author

2015-01-29 18:21 UTC #13 lore raw

Re: [PATCH -mm v2 1/3] slub: never fail to shrink cache

Quoted 10 lines · Christoph Lameter

Quoted 7 lines · Vladimir Davydov
Yeah, but the tool just writes 1 to /sys/kernel/slab/cache/shrink, i.e. invokes shrink_store(), and I don't propose to remove slab placement optimization from there. What I propose is to move slab placement optimization from kmem_cache_shrink() to shrink_store(), because other users of kmem_cache_shrink() don't seem to need it at all - they just want to release empty slabs. Such a change wouldn't affect the behavior of slabinfo -s at all.

Quoted 2 lines

Well we have to go through the chain of partial slabs anyways so its easy to do the optimization at that point.

That's true, but we can introduce a separate function that would both release empty slabs and optimize slab placement, like the patch below does. It would increase the code size a bit though, so I don't insist.

mm/slub.c +29 -3

index 1562955fe099..2cd401d82a41 100644
@@ -3359,7 +3359,7 @@ void kfree(const void *x)
 EXPORT_SYMBOL(kfree);
 
 /*
- * kmem_cache_shrink removes empty slabs from the partial lists and sorts
+ * shrink_slab_cache removes empty slabs from the partial lists and sorts
  * the remaining slabs by the number of items in use. The slabs with the
  * most items in use come first. New allocations will then fill those up
  * and thus they can be removed from the partial lists.

@@ -3368,7 +3368,7 @@ EXPORT_SYMBOL(kfree);
  * being allocated from last increasing the chance that the last objects
  * are freed in them.
  */
-int __kmem_cache_shrink(struct kmem_cache *s)
+static int shrink_slab_cache(struct kmem_cache *s)
 {
 	int node;
 	int i;

@@ -3423,6 +3423,32 @@ int __kmem_cache_shrink(struct kmem_cache *s)
 	return 0;
 }
 
+static int __kmem_cache_shrink(struct kmem_cache *s)
+{
+	int node;
+	struct kmem_cache_node *n;
+	struct page *page, *t;
+	LIST_HEAD(discard);
+	unsigned long flags;
+	int ret = 0;
+
+	flush_all(s);
+	for_each_kmem_cache_node(s, node, n) {
+		spin_lock_irqsave(&n->list_lock, flags);
+		list_for_each_entry_safe(page, t, &n->partial, lru)
+			if (!page->inuse)
+				list_move(&page->lru, &discard);
+		spin_unlock_irqrestore(&n->list_lock, flags);
+
+		list_for_each_entry_safe(page, t, &discard, lru)
+			discard_slab(s, page);
+
+		if (slabs_node(s, node))
+			ret = 1;
+	}
+	return ret;
+}
+
 static int slab_mem_going_offline_callback(void *arg)
 {
 	struct kmem_cache *s;

@@ -4683,7 +4709,7 @@ static ssize_t shrink_store(struct kmem_cache *s,
 			const char *buf, size_t length)
 {
 	if (buf[0] == '1') {
-		int rc = kmem_cache_shrink(s);
+		int rc = shrink_slab_cache(s);
 
 		if (rc)
 			return rc;

Christoph Lameter

2015-01-29 19:10 UTC #14 lore raw

Re: [PATCH -mm v2 1/3] slub: never fail to shrink cache

Quoted 6 lines · Vladimir Davydov

Quoted 2 lines
Well we have to go through the chain of partial slabs anyways so its easy to do the optimization at that point.
That's true, but we can introduce a separate function that would both release empty slabs and optimize slab placement, like the patch below does. It would increase the code size a bit though, so I don't insist.

It would also change what slabinfo -s does.

Balbir Singh

2015-01-29 08:32 UTC #15 lore raw

Re: [PATCH -mm v2 1/3] slub: never fail to shrink cache

In reply to Andrew Morton

Quoted 38 lines · Andrew Morton

Quoted 24 lines · On Wed, 28 Jan 2015 19:22:49 +0300 Vladimir Davydov <[email protected]>…
SLUB's version of __kmem_cache_shrink() not only removes empty slabs, but also tries to rearrange the partial lists to place slabs filled up most to the head to cope with fragmentation. To achieve that, it allocates a temporary array of lists used to sort slabs by the number of objects in use. If the allocation fails, the whole procedure is aborted.
This is unacceptable for the kernel memory accounting extension of the memory cgroup, where we want to make sure that kmem_cache_shrink() successfully discarded empty slabs. Although the allocation failure is utterly unlikely with the current page allocator implementation, which retries GFP_KERNEL allocations of order <= 2 infinitely, it is better not to rely on that.
This patch therefore makes __kmem_cache_shrink() allocate the array on stack instead of calling kmalloc, which may fail. The array size is chosen to be equal to 32, because most SLUB caches store not more than 32 objects per slab page. Slab pages with <= 32 free objects are sorted using the array by the number of objects in use and promoted to the head of the partial list, while slab pages with > 32 free objects are left in the end of the list without any ordering imposed on them.
...
mm/slub.c int __kmem_cache_shrink(struct kmem_cache *s) · line 3380
Quoted 8 lines
mm/slub.c +2 -3
@@ -3375,51 +3376,56 @@ int __kmem_cache_shrink(struct kmem_cache *s)
      struct kmem_cache_node *n;
      struct page *page;
      struct page *t;
-     int objects = oo_objects(s->max);
-     struct list_head *slabs_by_inuse =
-             kmalloc(sizeof(struct list_head) * objects, GFP_KERNEL);
+     LIST_HEAD(discard);
+     struct list_head promote[SHRINK_PROMOTE_MAX];
512 bytes of stack. The call paths leading to __kmem_cache_shrink() are many and twisty. How do we know this isn't a problem?

Quoted 2 lines

The logic behind choosing "32" sounds rather rubbery. What goes wrong if we use, say, "4"?

This much space in the stack may be fertile grounds for kernel stack overflow code execution :) Another perspective could be that there should be allocations that are not penalized to a particular cgroup (from an accounting perspective), should come from the reserved pool.

Balbir Singh.

Sasha Levin

2015-02-15 03:55 UTC #16 lore raw

Re: [PATCH -mm v2 1/3] slub: never fail to shrink cache

In reply to Vladimir Davydov

Hi Vladimir,

Quoted 21 lines · Vladimir Davydov

SLUB's version of __kmem_cache_shrink() not only removes empty slabs, but also tries to rearrange the partial lists to place slabs filled up most to the head to cope with fragmentation. To achieve that, it allocates a temporary array of lists used to sort slabs by the number of objects in use. If the allocation fails, the whole procedure is aborted.
This is unacceptable for the kernel memory accounting extension of the memory cgroup, where we want to make sure that kmem_cache_shrink() successfully discarded empty slabs. Although the allocation failure is utterly unlikely with the current page allocator implementation, which retries GFP_KERNEL allocations of order <= 2 infinitely, it is better not to rely on that.
This patch therefore makes __kmem_cache_shrink() allocate the array on stack instead of calling kmalloc, which may fail. The array size is chosen to be equal to 32, because most SLUB caches store not more than 32 objects per slab page. Slab pages with <= 32 free objects are sorted using the array by the number of objects in use and promoted to the head of the partial list, while slab pages with > 32 free objects are left in the end of the list without any ordering imposed on them.

Quoted 1 line

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

It seems that this patch causes shrink to corrupt memory:

# echo 1 > /sys/kernel/slab/kmalloc-64/shrink
#
[   60.331433] =============================================================================
[   60.333052] BUG kmalloc-64 (Not tainted): Padding overwritten. 0xffff880051018f50-0xffff880051018fff
[   60.335714] -----------------------------------------------------------------------------
[   60.335714]
[   60.338530] Disabling lock debugging due to kernel taint
[   60.340140] INFO: Slab 0xffffea0001440600 objects=32767 used=65408 fp=0x          (null) flags=0x5fffff80000000
[   60.343095] CPU: 0 PID: 8634 Comm: sh Tainted: G    B           3.19.0-next-20150213-sasha-00045-g897c679-dirty #1920
[   60.345315]  ffff88001fc0ee40 00000000542c5de6 ffff88001114f908 ffffffffabb74948
[   60.346454]  0000000000000000 ffffea0001440600 ffff88001114f9e8 ffffffffa1764b4f
[   60.347582]  0000000000000007 ffff880000000028 ffff88001114f9f8 ffff88001114f9a8
[   60.349842] Call Trace:
[   60.350243]  [<ffffffffabb74948>] dump_stack+0x4f/0x7b
[   60.350975]  [<ffffffffa1764b4f>] slab_err+0xaf/0xd0
[   60.351714]  [<ffffffffa307cce0>] ? memchr_inv+0x2c0/0x360
[   60.352592]  [<ffffffffa17671b0>] slab_pad_check+0x120/0x1c0
[   60.353418]  [<ffffffffa17673a4>] __free_slab+0x154/0x1f0
[   60.354209]  [<ffffffffa141c365>] ? trace_hardirqs_on_caller+0x475/0x610
[   60.355168]  [<ffffffffa1767478>] discard_slab+0x38/0x60
[   60.355909]  [<ffffffffa176d478>] __kmem_cache_shrink+0x258/0x300
[   60.356801]  [<ffffffffa1764770>] ? print_tracking+0x70/0x70
[   60.357621]  [<ffffffffa1764770>] ? print_tracking+0x70/0x70
[   60.358448]  [<ffffffffa16c8460>] kmem_cache_shrink+0x20/0x30
[   60.359279]  [<ffffffffa17665db>] shrink_store+0x1b/0x30
[   60.360048]  [<ffffffffa17647af>] slab_attr_store+0x3f/0xf0
[   60.360951]  [<ffffffffa1764770>] ? print_tracking+0x70/0x70
[   60.361778]  [<ffffffffa193c56a>] sysfs_kf_write+0x11a/0x180
[   60.362601]  [<ffffffffa193c450>] ? sysfs_file_ops+0x170/0x170
[   60.363447]  [<ffffffffa1939ea1>] kernfs_fop_write+0x271/0x3b0
[   60.364348]  [<ffffffffa17ba386>] vfs_write+0x186/0x5d0
[   60.365112]  [<ffffffffa17bd146>] SyS_write+0x126/0x270
[   60.365837]  [<ffffffffa17bd020>] ? SyS_read+0x270/0x270
[   60.366608]  [<ffffffffa141c365>] ? trace_hardirqs_on_caller+0x475/0x610
[   60.367580]  [<ffffffffa3091d7b>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[   60.368566]  [<ffffffffabbea3ad>] system_call_fastpath+0x16/0x1b
[   60.369435] Padding ffff880051018f50: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[   60.370750] Padding ffff880051018f60: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[   60.372077] Padding ffff880051018f70: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
[   60.373450] Padding ffff880051018f80: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5  kkkkkkkkkkkkkkk.
[   60.374763] Padding ffff880051018f90: bb bb bb bb bb bb bb bb c8 8d 01 51 00 88 ff ff  ...........Q....
[   60.376083] Padding ffff880051018fa0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[   60.377457] Padding ffff880051018fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[   60.378768] Padding ffff880051018fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[   60.380084] Padding ffff880051018fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[   60.381475] Padding ffff880051018fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[   60.382798] Padding ffff880051018ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[   60.384121] FIX kmalloc-64: Restoring 0xffff880051018f50-0xffff880051018fff=0x5a
[...]

And basically a lot more of the above.

Thanks,
Sasha

Vladimir Davydov

author

2015-02-15 09:48 UTC #17 lore raw

Re: [PATCH -mm v2 1/3] slub: never fail to shrink cache

Hi,

Quoted 1 line · Sasha Levin

It seems that this patch causes shrink to corrupt memory:

Yes, it does :-(

The fix can be found here: https://lkml.org/lkml/2015/2/11/347

It must have already been merged to the -mm tree:

Quoted 7 lines · On Thu, Feb 12, 2015 at 02:14:54PM -0800, [email protected] wrote:

The patch titled
Subject: slub: kmem_cache_shrink: fix crash due to uninitialized discard list
has been removed from the -mm tree. Its filename was
slub-never-fail-to-shrink-cache-init-discard-list-after-freeing-slabs.patch
This patch was dropped because it was folded into slub-never-fail-to-shrink-cache.patch

Thanks,
Vladimir

Vladimir Davydov

author

2015-01-28 16:23 UTC #18 lore raw

PATCH v2 2/3 slub: fix kmem_cache_shrink return value

In reply to Vladimir Davydov

It is supposed to return 0 if the cache has no remaining objects and 1 otherwise, while currently it always returns 0. Fix it.

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

---
 mm/slub.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

mm/slub.c +5 -1

index dbf9334b6a5c..63abe52c2951 100644
@@ -3379,6 +3379,7 @@ int __kmem_cache_shrink(struct kmem_cache *s)
 	LIST_HEAD(discard);
 	struct list_head promote[SHRINK_PROMOTE_MAX];
 	unsigned long flags;
+	int ret = 0;
 
 	for (i = 0; i < SHRINK_PROMOTE_MAX; i++)
 		INIT_LIST_HEAD(promote + i);

@@ -3419,6 +3420,9 @@ int __kmem_cache_shrink(struct kmem_cache *s)
 		for (i = SHRINK_PROMOTE_MAX - 1; i >= 0; i--)
 			list_splice_init(promote + i, &n->partial);
 
+		if (n->nr_partial || slabs_node(s, node))
+			ret = 1;
+
 		spin_unlock_irqrestore(&n->list_lock, flags);
 
 		/* Release empty slabs */

@@ -3426,7 +3430,7 @@ int __kmem_cache_shrink(struct kmem_cache *s)
 			discard_slab(s, page);
 	}
 
-	return 0;
+	return ret;
 }
 
 static int slab_mem_going_offline_callback(void *arg)

Signature · 7 lines

1.7.10.4

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

Christoph Lameter

2015-01-28 16:33 UTC #19 lore raw

Re: [PATCH -mm v2 2/3] slub: fix kmem_cache_shrink return value

mm/slub.c int __kmem_cache_shrink(struct kmem_cache *s) · line 3423

Quoted 5 lines · Vladimir Davydov

mm/slub.c +1 -0

@@ -3419,6 +3420,9 @@ int __kmem_cache_shrink(struct kmem_cache *s)
 		for (i = SHRINK_PROMOTE_MAX - 1; i >= 0; i--)
 			list_splice_init(promote + i, &n->partial);
 
+		if (n->nr_partial || slabs_node(s, node))

The total number of slabs obtained via slabs_node always contains the number of partial ones. So no need to check n->nr_partial.

Vladimir Davydov

author

2015-01-28 17:46 UTC #20 lore raw

Re: [PATCH -mm v2 2/3] slub: fix kmem_cache_shrink return value

Quoted 10 lines · Christoph Lameter

Review int __kmem_cache_shrink(struct kmem_cache *s) · line 3423
Quoted 5 lines · Vladimir Davydov
(quoted diff) +1 -0
@@ -3419,6 +3420,9 @@ int __kmem_cache_shrink(struct kmem_cache *s)
 		for (i = SHRINK_PROMOTE_MAX - 1; i >= 0; i--)
 			list_splice_init(promote + i, &n->partial);
 
+		if (n->nr_partial || slabs_node(s, node))
The total number of slabs obtained via slabs_node always contains the number of partial ones. So no need to check n->nr_partial.

Yeah, right. In addition to that I misplaced the check - it should go after discard_slab, where we decrement nr_slabs. Here goes the updated patch:

From: Vladimir Davydov <[email protected]>
Subject: [PATCH] slub: fix kmem_cache_shrink return value

It is supposed to return 0 if the cache has no remaining objects and 1 otherwise, while currently it always returns 0. Fix it.

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

---
 mm/slub.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

mm/slub.c +5 -1

index dbf9334b6a5c..5626588db884 100644
@@ -3379,6 +3379,7 @@ int __kmem_cache_shrink(struct kmem_cache *s)
 	LIST_HEAD(discard);
 	struct list_head promote[SHRINK_PROMOTE_MAX];
 	unsigned long flags;
+	int ret = 0;
 
 	for (i = 0; i < SHRINK_PROMOTE_MAX; i++)
 		INIT_LIST_HEAD(promote + i);

@@ -3424,9 +3425,12 @@ int __kmem_cache_shrink(struct kmem_cache *s)
 		/* Release empty slabs */
 		list_for_each_entry_safe(page, t, &discard, lru)
 			discard_slab(s, page);
+
+		if (slabs_node(s, node))
+			ret = 1;
 	}
 
-	return 0;
+	return ret;
 }
 
 static int slab_mem_going_offline_callback(void *arg)

Signature · 7 lines

1.7.10.4

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

Christoph Lameter

Acked-by

2015-01-28 19:19 UTC #21 lore raw

Re: [PATCH -mm v2 2/3] slub: fix kmem_cache_shrink return value

Quoted 9 lines · Vladimir Davydov

Yeah, right. In addition to that I misplaced the check - it should go after discard_slab, where we decrement nr_slabs. Here goes the updated patch:
From: Vladimir Davydov <[email protected]>
Subject: [PATCH] slub: fix kmem_cache_shrink return value
It is supposed to return 0 if the cache has no remaining objects and 1 otherwise, while currently it always returns 0. Fix it.

Acked-by Christoph Lameter <[email protected]>

Vladimir Davydov

author

2015-01-28 16:23 UTC #22 lore raw

PATCH v2 3/3 slub: make dead caches discard free slabs immediately

In reply to Vladimir Davydov

To speed up further allocations SLUB may store empty slabs in per cpu/node partial lists instead of freeing them immediately. This prevents per memcg caches destruction, because kmem caches created for a memory cgroup are only destroyed after the last page charged to the cgroup is freed.

To fix this issue, this patch resurrects approach first proposed in [1]. It forbids SLUB to cache empty slabs after the memory cgroup that the cache belongs to was destroyed. It is achieved by setting kmem_cache's cpu_partial and min_partial constants to 0 and tuning put_cpu_partial() so that it would drop frozen empty slabs immediately if cpu_partial = 0.

The runtime overhead is minimal. From all the hot functions, we only touch relatively cold put_cpu_partial(): we make it call unfreeze_partials() after freezing a slab that belongs to an offline memory cgroup. Since slab freezing exists to avoid moving slabs from/to a partial list on free/alloc, and there can't be allocations from dead caches, it shouldn't cause any overhead. We do have to disable preemption for put_cpu_partial() to achieve that though.

The original patch was accepted well and even merged to the mm tree. However, I decided to withdraw it due to changes happening to the memcg core at that time. I had an idea of introducing per-memcg shrinkers for kmem caches, but now, as memcg has finally settled down, I do not see it as an option, because SLUB shrinker would be too costly to call since SLUB does not keep free slabs on a separate list. Besides, we currently do not even call per-memcg shrinkers for offline memcgs. Overall, it would introduce much more complexity to both SLUB and memcg than this small patch.

Regarding to SLAB, there's no problem with it, because it shrinks per-cpu/node caches periodically. Thanks to list_lru reparenting, we no longer keep entries for offline cgroups in per-memcg arrays (such as memcg_cache_params->memcg_caches), so we do not have to bother if a per-memcg cache will be shrunk a bit later than it could be.

[1] http://thread.gmane.org/gmane.linux.kernel.mm/118649/focus=118650

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

---
 mm/slab.c        |    4 ++--
 mm/slab.h        |    2 +-
 mm/slab_common.c |   15 +++++++++++++--
 mm/slob.c        |    2 +-
 mm/slub.c        |   31 ++++++++++++++++++++++++++-----
 5 files changed, 43 insertions(+), 11 deletions(-)

mm/slab.c +2 -2

index 7894017bc160..c4b89eaf4c96 100644
@@ -2382,7 +2382,7 @@ out:
 	return nr_freed;
 }
 
-int __kmem_cache_shrink(struct kmem_cache *cachep)
+int __kmem_cache_shrink(struct kmem_cache *cachep, bool deactivate)
 {
 	int ret = 0;
 	int node;

@@ -2404,7 +2404,7 @@ int __kmem_cache_shutdown(struct kmem_cache *cachep)
 {
 	int i;
 	struct kmem_cache_node *n;
-	int rc = __kmem_cache_shrink(cachep);
+	int rc = __kmem_cache_shrink(cachep, false);
 
 	if (rc)
 		return rc;

mm/slab.h +1 -1

index 0a56d76ac0e9..4c3ac12dd644 100644
@@ -138,7 +138,7 @@ static inline unsigned long kmem_cache_flags(unsigned long object_size,
 #define CACHE_CREATE_MASK (SLAB_CORE_FLAGS | SLAB_DEBUG_FLAGS | SLAB_CACHE_FLAGS)
 
 int __kmem_cache_shutdown(struct kmem_cache *);
-int __kmem_cache_shrink(struct kmem_cache *);
+int __kmem_cache_shrink(struct kmem_cache *, bool);
 void slab_kmem_cache_release(struct kmem_cache *);
 
 struct seq_file;

mm/slab_common.c +13 -2

index 0dd9eb4e0f87..9395842cfc6b 100644
@@ -549,10 +549,13 @@ void memcg_deactivate_kmem_caches(struct mem_cgroup *memcg)
 {
 	int idx;
 	struct memcg_cache_array *arr;
-	struct kmem_cache *s;
+	struct kmem_cache *s, *c;
 
 	idx = memcg_cache_id(memcg);
 
+	get_online_cpus();
+	get_online_mems();
+
 	mutex_lock(&slab_mutex);
 	list_for_each_entry(s, &slab_caches, list) {
 		if (!is_root_cache(s))

@@ -560,9 +563,17 @@ void memcg_deactivate_kmem_caches(struct mem_cgroup *memcg)
 
 		arr = rcu_dereference_protected(s->memcg_params.memcg_caches,
 						lockdep_is_held(&slab_mutex));
+		c = arr->entries[idx];
+		if (!c)
+			continue;
+
+		__kmem_cache_shrink(c, true);
 		arr->entries[idx] = NULL;
 	}
 	mutex_unlock(&slab_mutex);
+
+	put_online_mems();
+	put_online_cpus();
 }
 
 void memcg_destroy_kmem_caches(struct mem_cgroup *memcg)

@@ -649,7 +660,7 @@ int kmem_cache_shrink(struct kmem_cache *cachep)
 
 	get_online_cpus();
 	get_online_mems();
-	ret = __kmem_cache_shrink(cachep);
+	ret = __kmem_cache_shrink(cachep, false);
 	put_online_mems();
 	put_online_cpus();
 	return ret;

mm/slob.c +1 -1

index 96a86206a26b..94a7fede6d48 100644
@@ -618,7 +618,7 @@ int __kmem_cache_shutdown(struct kmem_cache *c)
 	return 0;
 }
 
-int __kmem_cache_shrink(struct kmem_cache *d)
+int __kmem_cache_shrink(struct kmem_cache *d, bool deactivate)
 {
 	return 0;
 }

mm/slub.c +26 -5

index 63abe52c2951..a91789f59ab6 100644
@@ -2007,6 +2007,7 @@ static void put_cpu_partial(struct kmem_cache *s, struct page *page, int drain)
 	int pages;
 	int pobjects;
 
+	preempt_disable();
 	do {
 		pages = 0;
 		pobjects = 0;

@@ -2040,6 +2041,14 @@ static void put_cpu_partial(struct kmem_cache *s, struct page *page, int drain)
 
 	} while (this_cpu_cmpxchg(s->cpu_slab->partial, oldpage, page)
 								!= oldpage);
+	if (unlikely(!s->cpu_partial)) {
+		unsigned long flags;
+
+		local_irq_save(flags);
+		unfreeze_partials(s, this_cpu_ptr(s->cpu_slab));
+		local_irq_restore(flags);
+	}
+	preempt_enable();
 #endif
 }

@@ -3369,7 +3378,7 @@ EXPORT_SYMBOL(kfree);
  * being allocated from last increasing the chance that the last objects
  * are freed in them.
  */
-int __kmem_cache_shrink(struct kmem_cache *s)
+int __kmem_cache_shrink(struct kmem_cache *s, bool deactivate)
 {
 	int node;
 	int i;

@@ -3381,14 +3390,26 @@ int __kmem_cache_shrink(struct kmem_cache *s)
 	unsigned long flags;
 	int ret = 0;
 
+	if (deactivate) {
+		/*
+		 * Disable empty slabs caching. Used to avoid pinning offline
+		 * memory cgroups by kmem pages that can be freed.
+		 */
+		s->cpu_partial = 0;
+		s->min_partial = 0;
+
+		/*
+		 * s->cpu_partial is checked locklessly (see put_cpu_partial),
+		 * so we have to make sure the change is visible.
+		 */
+		kick_all_cpus_sync();
+	}
+
 	for (i = 0; i < SHRINK_PROMOTE_MAX; i++)
 		INIT_LIST_HEAD(promote + i);
 
 	flush_all(s);
 	for_each_kmem_cache_node(s, node, n) {
-		if (!n->nr_partial)
-			continue;
-
 		spin_lock_irqsave(&n->list_lock, flags);
 
 		/*

@@ -3439,7 +3460,7 @@ static int slab_mem_going_offline_callback(void *arg)
 
 	mutex_lock(&slab_mutex);
 	list_for_each_entry(s, &slab_caches, list)
-		__kmem_cache_shrink(s);
+		__kmem_cache_shrink(s, false);
 	mutex_unlock(&slab_mutex);
 
 	return 0;

Signature · 7 lines

1.7.10.4

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

Peter Zijlstra

2016-04-01 09:04 UTC #23 lore raw

Quoted 41 lines · Vladimir Davydov

+++ b/mm/slub.c

mm/slub.c +14 -1

@@ -2007,6 +2007,7 @@ static void put_cpu_partial(struct kmem_cache *s, struct page *page, int drain)
 	int pages;
 	int pobjects;
 
+	preempt_disable();
 	do {
 		pages = 0;
 		pobjects = 0;

@@ -2040,6 +2041,14 @@ static void put_cpu_partial(struct kmem_cache *s, struct page *page, int drain)
 
 	} while (this_cpu_cmpxchg(s->cpu_slab->partial, oldpage, page)
 								!= oldpage);
+	if (unlikely(!s->cpu_partial)) {
+		unsigned long flags;
+
+		local_irq_save(flags);
+		unfreeze_partials(s, this_cpu_ptr(s->cpu_slab));
+		local_irq_restore(flags);
+	}
+	preempt_enable();
 #endif
 }

@@ -3369,7 +3378,7 @@ EXPORT_SYMBOL(kfree);
  * being allocated from last increasing the chance that the last objects
  * are freed in them.
  */
-int __kmem_cache_shrink(struct kmem_cache *s)
+int __kmem_cache_shrink(struct kmem_cache *s, bool deactivate)
 {
 	int node;
 	int i;

@@ -3381,14 +3390,26 @@ int __kmem_cache_shrink(struct kmem_cache *s)
 	unsigned long flags;
 	int ret = 0;
 
+	if (deactivate) {
+		/*
+		 * Disable empty slabs caching. Used to avoid pinning offline
+		 * memory cgroups by kmem pages that can be freed.

mm/slub.c int __kmem_cache_shrink(struct kmem_cache *s) · line 3406

Quoted 10 lines

mm/slub.c +10 -0

+		 */
+		s->cpu_partial = 0;
+		s->min_partial = 0;
+
+		/*
+		 * s->cpu_partial is checked locklessly (see put_cpu_partial),
+		 * so we have to make sure the change is visible.
+		 */
+		kick_all_cpus_sync();
+	}

Argh! what the heck! and without a single mention in the changelog.

Why are you spraying IPIs across the entire machine? Why isn't synchronize_sched() good enough, that would allow you to get rid of the local_irq_save/restore as well.

Vladimir Davydov

2016-04-01 10:55 UTC #24 lore raw

Quoted 38 lines · Peter Zijlstra

Quoted 37 lines · Vladimir Davydov

+++ b/mm/slub.c

mm/slub.c +10 -1

@@ -2007,6 +2007,7 @@ static void put_cpu_partial(struct kmem_cache *s, struct page *page, int drain)
 	int pages;
 	int pobjects;
 
+	preempt_disable();
 	do {
 		pages = 0;
 		pobjects = 0;

@@ -2040,6 +2041,14 @@ static void put_cpu_partial(struct kmem_cache *s, struct page *page, int drain)
 
 	} while (this_cpu_cmpxchg(s->cpu_slab->partial, oldpage, page)
 								!= oldpage);
+	if (unlikely(!s->cpu_partial)) {
+		unsigned long flags;
+
+		local_irq_save(flags);
+		unfreeze_partials(s, this_cpu_ptr(s->cpu_slab));
+		local_irq_restore(flags);
+	}
+	preempt_enable();
 #endif
 }

@@ -3369,7 +3378,7 @@ EXPORT_SYMBOL(kfree);
  * being allocated from last increasing the chance that the last objects
  * are freed in them.
  */
-int __kmem_cache_shrink(struct kmem_cache *s)
+int __kmem_cache_shrink(struct kmem_cache *s, bool deactivate)
 {
 	int node;
 	int i;

@@ -3381,14 +3390,26 @@ int __kmem_cache_shrink(struct kmem_cache *s)
 	unsigned long flags;
 	int ret = 0;

Quoted 16 lines

Quoted 4 lines

mm/slub.c +4 -0

+	if (deactivate) {
+		/*
+		 * Disable empty slabs caching. Used to avoid pinning offline
+		 * memory cgroups by kmem pages that can be freed.

mm/slub.c int __kmem_cache_shrink(struct kmem_cache *s) · line 3406

Quoted 10 lines

mm/slub.c +10 -0

+		 */
+		s->cpu_partial = 0;
+		s->min_partial = 0;
+
+		/*
+		 * s->cpu_partial is checked locklessly (see put_cpu_partial),
+		 * so we have to make sure the change is visible.
+		 */
+		kick_all_cpus_sync();
+	}

Argh! what the heck! and without a single mention in the changelog.

This function is only called when a memory cgroup is removed, which is rather a rare event. I didn't think it would cause any pain. Sorry.

Quoted 4 lines

Why are you spraying IPIs across the entire machine? Why isn't synchronize_sched() good enough, that would allow you to get rid of the local_irq_save/restore as well.

synchronize_sched() is slower. Calling it for every per memcg kmem cache would slow down cleanup on cgroup removal. The latter is async, so I'm not sure if it would be a problem though. I think we can try to replace kick_all_cpus_sync() with synchronize_sched() here.

Regarding local_irq_save/restore - synchronize_sched() wouldn't allow us to get rid of them, because unfreeze_partials() must be called with irqs disabled.

Come to think of it, kick_all_cpus_sync() is used as a memory barrier here, so as to make sure that after it's finished all cpus will use the new ->cpu_partial value, which makes me wonder if we could replace it with a simple smp_mb. I mean, this_cpu_cmpxchg(), which is used by put_cpu_partial to add a page to per-cpu partial list, must issue a full memory barrier (am I correct?), so we have two possibilities here:

Case 1: smp_mb is called before this_cpu_cmpxchg is called on another cpu executing put_cpu_partial. In this case, put_cpu_partial will see cpu_partial == 0 and hence call the second unfreeze_partials, flushing per cpu partial list.

Case 2: smp_mb is called after this_cpu_cmpxchg. Then __kmem_cache_shrink ->flush_all -> has_cpu_slab should see (thanks to the barriers) that there's a slab on a per-cpu list and so flush it (provided it hasn't already been flushed by put_cpu_partial).

In any case, after __kmem_cache_shrink has finished, we are guaranteed to not have any slabs on per cpu partial lists.

Does it make sense?

Thanks,
Vladimir

Peter Zijlstra

2016-04-01 11:41 UTC #25 lore raw

Quoted 17 lines · Vladimir Davydov

Quoted 8 lines
Quoted 8 lines
mm/slub.c +8 -0
+	if (deactivate) {
+		/*
+		 * Disable empty slabs caching. Used to avoid pinning offline
+		 * memory cgroups by kmem pages that can be freed.
+		 */
+		s->cpu_partial = 0;
+		s->min_partial = 0;
+
Quoted 8 lines
mm/slub.c int __kmem_cache_shrink(struct kmem_cache *s) · line 3406
Quoted 6 lines
mm/slub.c +6 -0
+		/*
+		 * s->cpu_partial is checked locklessly (see put_cpu_partial),
+		 * so we have to make sure the change is visible.
+		 */
+		kick_all_cpus_sync();
+	}
Argh! what the heck! and without a single mention in the changelog.

Quoted 2 lines

This function is only called when a memory cgroup is removed, which is rather a rare event. I didn't think it would cause any pain. Sorry.

Suppose you have a bunch of CPUs running HPC/RT code and someone causes the admin CPUs to create/destroy a few cgroups.

Quoted 6 lines

Quoted 3 lines
Why are you spraying IPIs across the entire machine? Why isn't synchronize_sched() good enough, that would allow you to get rid of the local_irq_save/restore as well.
synchronize_sched() is slower. Calling it for every per memcg kmem cache would slow down cleanup on cgroup removal.

Right, but who cares? cgroup removal isn't a fast path by any standard.

Quoted 3 lines

Regarding local_irq_save/restore - synchronize_sched() wouldn't allow us to get rid of them, because unfreeze_partials() must be called with irqs disabled.

OK, I figured it was because it needed to be serialized against this kick_all_cpus_sync() IPI.

Quoted 6 lines

Come to think of it, kick_all_cpus_sync() is used as a memory barrier here, so as to make sure that after it's finished all cpus will use the new ->cpu_partial value, which makes me wonder if we could replace it with a simple smp_mb. I mean, this_cpu_cmpxchg(), which is used by put_cpu_partial to add a page to per-cpu partial list, must issue a full memory barrier (am I correct?), so we have two possibilities here:

Nope, this_cpu_cmpxchg() does not imply a memory barrier.