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 <[email protected]>
---
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
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 <[email protected]>
---
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
This patch shares s_next and s_stop between slab and slub.
Signed-off-by: Wanpeng Li <[email protected]>
---
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
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.
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.
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 .
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().
On Mon, Jul 1, 2013 at 6:48 PM, Christoph Lameter <[email protected]> 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
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 <[email protected]> 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