[PATCH 1/3] mm/slab: Fix drain freelist excessively

[PATCH 1/3] mm/slab: Fix drain freelist excessively

[Wanpeng Li]

The drain_freelist is called to drain slabs_free lists for cache reap, 
cache shrink, memory hotplug callback etc. The tofree parameter is the 
number of slab objects to free instead of the number of slabs to free. 
The parameter transfered from callers is n->free_objects or n->freelimit 
+ 5 * (searchp->num - 1) / (5 * searchp->num), and both of them mean 
the number of slabs objects. I add printk to dump drain information:

[  122.864255] tofree is 2, actually free is 52, cache size is 26

The number of objects which caller prefer to drain is 2, however, actually 
52 objects are drained, this destroy the cache locality.This patch fix it 
by compare the number of slabs objects which already drained instead of 
compare the number of slabs to the number of slab objects prefer to drain.

Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
---
 mm/slab.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index be12f68..18628da 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2479,7 +2479,7 @@ static void drain_cpu_caches(struct kmem_cache *cachep)
 
 /*
  * Remove slabs from the list of free slabs.
- * Specify the number of slabs to drain in tofree.
+ * Specify the number of slab objects to drain in tofree.
  *
  * Returns the actual number of slabs released.
  */
@@ -2491,7 +2491,7 @@ static int drain_freelist(struct kmem_cache *cache,
 	struct slab *slabp;
 
 	nr_freed = 0;
-	while (nr_freed < tofree && !list_empty(&n->slabs_free)) {
+	while (nr_freed * cache->num < tofree && !list_empty(&n->slabs_free)) {
 
 		spin_lock_irq(&n->list_lock);
 		p = n->slabs_free.prev;
-- 
1.7.10.4

[PATCH 2/3] mm/slab: Sharing s_next and s_stop between slab and slub

[Wanpeng Li]

This patch shares s_next and s_stop between slab and slub.

Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
---
 mm/slab.c        |   10 ----------
 mm/slab.h        |    3 +++
 mm/slab_common.c |    4 ++--
 3 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 18628da..fe68e60 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -4430,16 +4430,6 @@ static int leaks_show(struct seq_file *m, void *p)
 	return 0;
 }
 
-static void *s_next(struct seq_file *m, void *p, loff_t *pos)
-{
-	return seq_list_next(p, &slab_caches, pos);
-}
-
-static void s_stop(struct seq_file *m, void *p)
-{
-	mutex_unlock(&slab_mutex);
-}
-
 static const struct seq_operations slabstats_op = {
 	.start = leaks_start,
 	.next = s_next,
diff --git a/mm/slab.h b/mm/slab.h
index f96b49e..95c8860 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -271,3 +271,6 @@ struct kmem_cache_node {
 #endif
 
 };
+
+void *s_next(struct seq_file *m, void *p, loff_t *pos);
+void s_stop(struct seq_file *m, void *p);
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 2d41450..d161b81 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -531,12 +531,12 @@ static void *s_start(struct seq_file *m, loff_t *pos)
 	return seq_list_start(&slab_caches, *pos);
 }
 
-static void *s_next(struct seq_file *m, void *p, loff_t *pos)
+void *s_next(struct seq_file *m, void *p, loff_t *pos)
 {
 	return seq_list_next(p, &slab_caches, pos);
 }
 
-static void s_stop(struct seq_file *m, void *p)
+void s_stop(struct seq_file *m, void *p)
 {
 	mutex_unlock(&slab_mutex);
 }
-- 
1.7.10.4

Re: [PATCH 2/3] mm/slab: Sharing s_next and s_stop between slab and slub

[David Rientjes]

On Mon, 24 Jun 2013, Wanpeng Li wrote:

> This patch shares s_next and s_stop between slab and slub.
> 

Just about the entire kernel includes slab.h, so I think you'll need to 
give these slab-specific names instead of exporting "s_next" and "s_stop" 
to everybody.

Re: [PATCH 2/3] mm/slab: Sharing s_next and s_stop between slab and slub

[Christoph Lameter]

On Mon, 24 Jun 2013, David Rientjes wrote:

> On Mon, 24 Jun 2013, Wanpeng Li wrote:
>
> > This patch shares s_next and s_stop between slab and slub.
> >
>
> Just about the entire kernel includes slab.h, so I think you'll need to
> give these slab-specific names instead of exporting "s_next" and "s_stop"
> to everybody.

He put the export into mm/slab.h. The headerfile is only included by
mm/sl?b.c .

Re: [PATCH 2/3] mm/slab: Sharing s_next and s_stop between slab and slub

[Pekka Enberg]

On Mon, Jul 1, 2013 at 6:48 PM, Christoph Lameter <cl@linux.com> wrote:
> On Mon, 24 Jun 2013, David Rientjes wrote:
>
>> On Mon, 24 Jun 2013, Wanpeng Li wrote:
>>
>> > This patch shares s_next and s_stop between slab and slub.
>> >
>>
>> Just about the entire kernel includes slab.h, so I think you'll need to
>> give these slab-specific names instead of exporting "s_next" and "s_stop"
>> to everybody.
>
> He put the export into mm/slab.h. The headerfile is only included by
> mm/sl?b.c .

But he then went on to add globally visible symbols "s_next" and
"s_stop" which is bad...

Please send me an incremental patch on top of slab/next to fix this
up. Otherwise I'll revert it before sending a pull request to Linus.

                      Pekka

Re: [PATCH 2/3] mm/slab: Sharing s_next and s_stop between slab and slub

[Pekka Enberg]

On 07/08/2013 03:16 AM, Wanpeng Li wrote:
> On Sun, Jul 07, 2013 at 07:41:54PM +0300, Pekka Enberg wrote:
>> On Mon, Jul 1, 2013 at 6:48 PM, Christoph Lameter <cl@linux.com> wrote:
>>> On Mon, 24 Jun 2013, David Rientjes wrote:
>>>
>>>> On Mon, 24 Jun 2013, Wanpeng Li wrote:
>>>>
>>>>> This patch shares s_next and s_stop between slab and slub.
>>>>>
>>>>
>>>> Just about the entire kernel includes slab.h, so I think you'll need to
>>>> give these slab-specific names instead of exporting "s_next" and "s_stop"
>>>> to everybody.
>>>
>>> He put the export into mm/slab.h. The headerfile is only included by
>>> mm/sl?b.c .
>>
>> But he then went on to add globally visible symbols "s_next" and
>> "s_stop" which is bad...
>>
>> Please send me an incremental patch on top of slab/next to fix this
>> up. Otherwise I'll revert it before sending a pull request to Linus.
>
> I attach the incremental patch in attachment. ;-)

Applied, thanks!

			Pekka

[PATCH 3/3] mm/slab: Fix /proc/slabinfo unwriteable for slab

[Wanpeng Li]

Slab have some tunables like limit, batchcount, and sharedfactor can be 
tuned through function slabinfo_write. Commit (b7454ad3: mm/sl[au]b: Move 
slabinfo processing to slab_common.c) uncorrectly change /proc/slabinfo 
unwriteable for slab, this patch fix it by revert to original mode.

Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
---
 mm/slab_common.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/mm/slab_common.c b/mm/slab_common.c
index d161b81..7fdde79 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -631,10 +631,20 @@ static const struct file_operations proc_slabinfo_operations = {
 	.release	= seq_release,
 };
 
+#ifdef CONFIG_SLAB
+static int __init slab_proc_init(void)
+{
+	proc_create("slabinfo", S_IWUSR | S_IRUSR, NULL, &proc_slabinfo_operations);
+	return 0;
+}
+#endif
+#ifdef CONFIG_SLUB
 static int __init slab_proc_init(void)
 {
 	proc_create("slabinfo", S_IRUSR, NULL, &proc_slabinfo_operations);
 	return 0;
 }
+#endif
+
 module_init(slab_proc_init);
 #endif /* CONFIG_SLABINFO */
-- 
1.7.10.4

Re: [PATCH 3/3] mm/slab: Fix /proc/slabinfo unwriteable for slab

[Christoph Lameter]

On Mon, 24 Jun 2013, Wanpeng Li wrote:

>  1 file changed, 10 insertions(+)
>
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index d161b81..7fdde79 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -631,10 +631,20 @@ static const struct file_operations proc_slabinfo_operations = {
>  	.release	= seq_release,
>  };
>
> +#ifdef CONFIG_SLAB
> +static int __init slab_proc_init(void)
> +{
> +	proc_create("slabinfo", S_IWUSR | S_IRUSR, NULL, &proc_slabinfo_operations);
> +	return 0;
> +}
> +#endif
> +#ifdef CONFIG_SLUB
>  static int __init slab_proc_init(void)
>  {
>  	proc_create("slabinfo", S_IRUSR, NULL, &proc_slabinfo_operations);
>  	return 0;
>  }

It may be easier to define a macro SLABINFO_RIGHTS and use #ifdefs to
assign the correct one. That way we have only one slab_proc_init().

Re: [PATCH 1/3] mm/slab: Fix drain freelist excessively

[Christoph Lameter]

On Mon, 24 Jun 2013, Wanpeng Li wrote:

> The drain_freelist is called to drain slabs_free lists for cache reap,
> cache shrink, memory hotplug callback etc. The tofree parameter is the
> number of slab objects to free instead of the number of slabs to free.

Well its intended to be the number of slabs to free. The patch does not
fix the callers that pass the number of slabs.

I think the best approach would be to fix the callers that pass # of
objects. Make sure they pass # of slabs.