[patch 1/2] slub: move min_partial to struct kmem_cache

[patch 1/2] slub: move min_partial to struct kmem_cache

[David Rientjes]

Although it allows for better cacheline use, it is unnecessary to save a
copy of the cache's min_partial value in each kmem_cache_node.

Cc: Christoph Lameter <cl@linux-foundation.org>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 include/linux/slub_def.h |    2 +-
 mm/slub.c                |   29 ++++++++++++++++-------------
 2 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
--- a/include/linux/slub_def.h
+++ b/include/linux/slub_def.h
@@ -46,7 +46,6 @@ struct kmem_cache_cpu {
 struct kmem_cache_node {
 	spinlock_t list_lock;	/* Protect partial list and nr_partial */
 	unsigned long nr_partial;
-	unsigned long min_partial;
 	struct list_head partial;
 #ifdef CONFIG_SLUB_DEBUG
 	atomic_long_t nr_slabs;
@@ -89,6 +88,7 @@ struct kmem_cache {
 	void (*ctor)(void *);
 	int inuse;		/* Offset to metadata */
 	int align;		/* Alignment */
+	unsigned long min_partial;
 	const char *name;	/* Name (only for display!) */
 	struct list_head list;	/* List of slab caches */
 #ifdef CONFIG_SLUB_DEBUG
diff --git a/mm/slub.c b/mm/slub.c
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1335,7 +1335,7 @@ static struct page *get_any_partial(struct kmem_cache *s, gfp_t flags)
 		n = get_node(s, zone_to_nid(zone));
 
 		if (n && cpuset_zone_allowed_hardwall(zone, flags) &&
-				n->nr_partial > n->min_partial) {
+				n->nr_partial > s->min_partial) {
 			page = get_partial_node(n);
 			if (page)
 				return page;
@@ -1387,7 +1387,7 @@ static void unfreeze_slab(struct kmem_cache *s, struct page *page, int tail)
 		slab_unlock(page);
 	} else {
 		stat(c, DEACTIVATE_EMPTY);
-		if (n->nr_partial < n->min_partial) {
+		if (n->nr_partial < s->min_partial) {
 			/*
 			 * Adding an empty slab to the partial slabs in order
 			 * to avoid page allocator overhead. This slab needs
@@ -1928,17 +1928,6 @@ static void
 init_kmem_cache_node(struct kmem_cache_node *n, struct kmem_cache *s)
 {
 	n->nr_partial = 0;
-
-	/*
-	 * The larger the object size is, the more pages we want on the partial
-	 * list to avoid pounding the page allocator excessively.
-	 */
-	n->min_partial = ilog2(s->size);
-	if (n->min_partial < MIN_PARTIAL)
-		n->min_partial = MIN_PARTIAL;
-	else if (n->min_partial > MAX_PARTIAL)
-		n->min_partial = MAX_PARTIAL;
-
 	spin_lock_init(&n->list_lock);
 	INIT_LIST_HEAD(&n->partial);
 #ifdef CONFIG_SLUB_DEBUG
@@ -2181,6 +2170,15 @@ static int init_kmem_cache_nodes(struct kmem_cache *s, gfp_t gfpflags)
 }
 #endif
 
+static void calculate_min_partial(struct kmem_cache *s, unsigned long min)
+{
+	if (min < MIN_PARTIAL)
+		min = MIN_PARTIAL;
+	else if (min > MAX_PARTIAL)
+		min = MAX_PARTIAL;
+	s->min_partial = min;
+}
+
 /*
  * calculate_sizes() determines the order and the distribution of data within
  * a slab object.
@@ -2319,6 +2317,11 @@ static int kmem_cache_open(struct kmem_cache *s, gfp_t gfpflags,
 	if (!calculate_sizes(s, -1))
 		goto error;
 
+	/*
+	 * The larger the object size is, the more pages we want on the partial
+	 * list to avoid pounding the page allocator excessively.
+	 */
+	calculate_min_partial(s, ilog2(s->size));
 	s->refcount = 1;
 #ifdef CONFIG_NUMA
 	s->remote_node_defrag_ratio = 1000;

[patch 2/2] slub: add min_partial sysfs tunable

[David Rientjes]

Now that a cache's min_partial has been moved to struct kmem_cache, it's
possible to easily tune it from userspace by adding a sysfs attribute.

It may not be desirable to keep a large number of partial slabs around if
a cache is used infrequently and memory, especially when constrained by a
cgroup, is scarce.  It's better to allow userspace to set the minimum
policy per cache instead of relying explicitly on kmem_cache_shrink().

Cc: Christoph Lameter <cl@linux-foundation.org>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 mm/slub.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3839,6 +3839,26 @@ static ssize_t order_show(struct kmem_cache *s, char *buf)
 }
 SLAB_ATTR(order);
 
+static ssize_t min_partial_show(struct kmem_cache *s, char *buf)
+{
+	return sprintf(buf, "%lu\n", s->min_partial);
+}
+
+static ssize_t min_partial_store(struct kmem_cache *s, const char *buf,
+				 size_t length)
+{
+	unsigned long min;
+	int err;
+
+	err = strict_strtoul(buf, 10, &min);
+	if (err)
+		return err;
+
+	calculate_min_partial(s, min);
+	return length;
+}
+SLAB_ATTR(min_partial);
+
 static ssize_t ctor_show(struct kmem_cache *s, char *buf)
 {
 	if (s->ctor) {
@@ -4154,6 +4174,7 @@ static struct attribute *slab_attrs[] = {
 	&object_size_attr.attr,
 	&objs_per_slab_attr.attr,
 	&order_attr.attr,
+	&min_partial_attr.attr,
 	&objects_attr.attr,
 	&objects_partial_attr.attr,
 	&total_objects_attr.attr,

Re: [patch 2/2] slub: add min_partial sysfs tunable

[Pekka Enberg]

Hi David,

On Sun, 2009-02-22 at 17:40 -0800, David Rientjes wrote:
> Now that a cache's min_partial has been moved to struct kmem_cache, it's
> possible to easily tune it from userspace by adding a sysfs attribute.
> 
> It may not be desirable to keep a large number of partial slabs around if
> a cache is used infrequently and memory, especially when constrained by a
> cgroup, is scarce.  It's better to allow userspace to set the minimum
> policy per cache instead of relying explicitly on kmem_cache_shrink().

The patches look good but the description is bit lacking. Does this
actually fix up something? Why don't we fix the limit calculations
instead?

I'm a sucker for numbers so I'm easily fooled into merging patches with
statements of the form "this shaves off N bytes/kb/mb on XYZ systems".

			Pekka

Re: [patch 2/2] slub: add min_partial sysfs tunable

[David Rientjes]

On Mon, 23 Feb 2009, Pekka Enberg wrote:

> The patches look good but the description is bit lacking. Does this
> actually fix up something? Why don't we fix the limit calculations
> instead?
> 
> I'm a sucker for numbers so I'm easily fooled into merging patches with
> statements of the form "this shaves off N bytes/kb/mb on XYZ systems".
> 

The memory savings from simply moving min_partial from struct 
kmem_cache_node to struct kmem_cache is obviously not significant (unless 
maybe you're from SGI or something), at the largest it's

	# allocated caches * (MAX_NUMNODES - 1) * sizeof(unsigned long)

The true savings occurs when userspace reduces the number of partial slabs 
that would otherwise be wasted, especially on machines with a large 
number of nodes (ia64 with CONFIG_NODES_SHIFT at 10 for default?).  As 
well as the kernel estimates ideal values for n->min_partial and ensures 
it's within a sane range, userspace has no other input other than writing 
to /sys/kernel/slab/cache/shrink.

There simply isn't any better heuristic to add when calculating the 
partial values for a better estimate that works for all possible caches.  
And since it's currently a static value, the user really has no way of 
reclaiming that wasted space, which can be significant when constrained by 
a cgroup (either cpusets or, later, memory controller slab limits) without 
shrinking it entirely.

This also allows the user to specify that increased fragmentation and more 
partial slabs are actually desired to avoid the cost of allocating new 
slabs at runtime for specific caches.

There's also no reason why this should be a per-struct kmem_cache_node 
value in the first place.  You could argue that a machine would have such 
node size asymmetries that it should be specified on a per-node basis, but 
we know nobody is doing that right now since it's a purely static value at 
the moment and there's no convenient way to tune that via slub's sysfs 
interface.

Re: [patch 2/2] slub: add min_partial sysfs tunable

[Pekka Enberg]

Hi David,

On Mon, 23 Feb 2009, Pekka Enberg wrote:
> > The patches look good but the description is bit lacking. Does this
> > actually fix up something? Why don't we fix the limit calculations
> > instead?
> > 
> > I'm a sucker for numbers so I'm easily fooled into merging patches with
> > statements of the form "this shaves off N bytes/kb/mb on XYZ systems".

On Mon, 2009-02-23 at 01:58 -0800, David Rientjes wrote:
> The memory savings from simply moving min_partial from struct 
> kmem_cache_node to struct kmem_cache is obviously not significant (unless 
> maybe you're from SGI or something), at the largest it's
> 
> 	# allocated caches * (MAX_NUMNODES - 1) * sizeof(unsigned long)
> 
> The true savings occurs when userspace reduces the number of partial slabs 
> that would otherwise be wasted, especially on machines with a large 
> number of nodes (ia64 with CONFIG_NODES_SHIFT at 10 for default?).  As 
> well as the kernel estimates ideal values for n->min_partial and ensures 
> it's within a sane range, userspace has no other input other than writing 
> to /sys/kernel/slab/cache/shrink.

Applied with the above explanation added to the changelog. Thanks!

			Pekka

Re: [patch 1/2] slub: move min_partial to struct kmem_cache

[Christoph Lameter]

On Sun, 22 Feb 2009, David Rientjes wrote:

> +static void calculate_min_partial(struct kmem_cache *s, unsigned long min)
> +{
> +	if (min < MIN_PARTIAL)
> +		min = MIN_PARTIAL;
> +	else if (min > MAX_PARTIAL)
> +		min = MAX_PARTIAL;
> +	s->min_partial = min;
> +}
> +
>  /*
>   * calculate_sizes() determines the order and the distribution of data within
>   * a slab object.
> @@ -2319,6 +2317,11 @@ static int kmem_cache_open(struct kmem_cache *s, gfp_t gfpflags,
>  	if (!calculate_sizes(s, -1))
>  		goto error;
>
> +	/*
> +	 * The larger the object size is, the more pages we want on the partial
> +	 * list to avoid pounding the page allocator excessively.
> +	 */
> +	calculate_min_partial(s, ilog2(s->size));
>  	s->refcount = 1;
>  #ifdef CONFIG_NUMA
>  	s->remote_node_defrag_ratio = 1000;
>

Move the ilog2 use into calculate_min_partial()? Otherweise
calculate_min_partial is merely enforcing limits and not doing any
calculations.

Re: [patch 2/2] slub: add min_partial sysfs tunable

[Christoph Lameter]

Note also that the min_partial is something useful to reduce the page
allocator overhead. If we can get the page allocator faster then the
min_partial defaults can be reduced.

Re: [patch 1/2] slub: move min_partial to struct kmem_cach

[Christoph Lameter]

On Mon, 23 Feb 2009, David Rientjes wrote:

> > Move the ilog2 use into calculate_min_partial()? Otherweise
> > calculate_min_partial is merely enforcing limits and not doing any
> > calculations.
> >
>
> The second patch in this series as a /sys/kernel/slab/cache/min_partial
> tunable that uses calculate_min_partial() to enforce a sane range, so
> while it may not actually be doing any calculations, the ilog2() is
> appropriately placed in kmem_cache_open().  We don't want to be taking the
> log of the user's min_partial value.

Then Rename the function to set_min_partial()?

Re: [patch 1/2] slub: move min_partial to struct kmem_cache

[David Rientjes]

On Mon, 23 Feb 2009, Christoph Lameter wrote:

> On Sun, 22 Feb 2009, David Rientjes wrote:
> 
> > +static void calculate_min_partial(struct kmem_cache *s, unsigned long min)
> > +{
> > +	if (min < MIN_PARTIAL)
> > +		min = MIN_PARTIAL;
> > +	else if (min > MAX_PARTIAL)
> > +		min = MAX_PARTIAL;
> > +	s->min_partial = min;
> > +}
> > +
> >  /*
> >   * calculate_sizes() determines the order and the distribution of data within
> >   * a slab object.
> > @@ -2319,6 +2317,11 @@ static int kmem_cache_open(struct kmem_cache *s, gfp_t gfpflags,
> >  	if (!calculate_sizes(s, -1))
> >  		goto error;
> >
> > +	/*
> > +	 * The larger the object size is, the more pages we want on the partial
> > +	 * list to avoid pounding the page allocator excessively.
> > +	 */
> > +	calculate_min_partial(s, ilog2(s->size));
> >  	s->refcount = 1;
> >  #ifdef CONFIG_NUMA
> >  	s->remote_node_defrag_ratio = 1000;
> >
> 
> Move the ilog2 use into calculate_min_partial()? Otherweise
> calculate_min_partial is merely enforcing limits and not doing any
> calculations.
> 

The second patch in this series as a /sys/kernel/slab/cache/min_partial 
tunable that uses calculate_min_partial() to enforce a sane range, so 
while it may not actually be doing any calculations, the ilog2() is 
appropriately placed in kmem_cache_open().  We don't want to be taking the 
log of the user's min_partial value.

Re: [patch 1/2] slub: move min_partial to struct kmem_cach

[David Rientjes]

On Mon, 23 Feb 2009, Christoph Lameter wrote:

> > The second patch in this series as a /sys/kernel/slab/cache/min_partial
> > tunable that uses calculate_min_partial() to enforce a sane range, so
> > while it may not actually be doing any calculations, the ilog2() is
> > appropriately placed in kmem_cache_open().  We don't want to be taking the
> > log of the user's min_partial value.
> 
> Then Rename the function to set_min_partial()?
> 

I don't have any strong opinions as to the specific name, so I'll leave 
this up to Pekka.  set_min_partial(), to me, seems to indicate that 
s->min_partial will unconditionally set to the value passed, which isn't 
necessarily true.


slub: rename calculate_min_partial() to set_min_partial()

Cc: Christoph Lameter <cl@linux-foundation.org>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 mm/slub.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2170,7 +2170,7 @@ static int init_kmem_cache_nodes(struct kmem_cache *s, gfp_t gfpflags)
 }
 #endif
 
-static void calculate_min_partial(struct kmem_cache *s, unsigned long min)
+static void set_min_partial(struct kmem_cache *s, unsigned long min)
 {
 	if (min < MIN_PARTIAL)
 		min = MIN_PARTIAL;
@@ -2321,7 +2321,7 @@ static int kmem_cache_open(struct kmem_cache *s, gfp_t gfpflags,
 	 * The larger the object size is, the more pages we want on the partial
 	 * list to avoid pounding the page allocator excessively.
 	 */
-	calculate_min_partial(s, ilog2(s->size));
+	set_min_partial(s, ilog2(s->size));
 	s->refcount = 1;
 #ifdef CONFIG_NUMA
 	s->remote_node_defrag_ratio = 1000;
@@ -3854,7 +3854,7 @@ static ssize_t min_partial_store(struct kmem_cache *s, const char *buf,
 	if (err)
 		return err;
 
-	calculate_min_partial(s, min);
+	set_min_partial(s, min);
 	return length;
 }
 SLAB_ATTR(min_partial);

Re: [patch 1/2] slub: move min_partial to struct kmem_cach

[Pekka Enberg]

On Mon, 23 Feb 2009, Christoph Lameter wrote:
> > > The second patch in this series as a /sys/kernel/slab/cache/min_partial
> > > tunable that uses calculate_min_partial() to enforce a sane range, so
> > > while it may not actually be doing any calculations, the ilog2() is
> > > appropriately placed in kmem_cache_open().  We don't want to be taking the
> > > log of the user's min_partial value.
> > 
> > Then Rename the function to set_min_partial()?

On Mon, 2009-02-23 at 08:37 -0800, David Rientjes wrote:
> I don't have any strong opinions as to the specific name, so I'll leave 
> this up to Pekka.  set_min_partial(), to me, seems to indicate that 
> s->min_partial will unconditionally set to the value passed, which isn't 
> necessarily true.

Applied, thanks!