From: Glauber Costa <glommer@parallels.com>
To: Christoph Lameter <cl@linux.com>
Cc: Pekka Enberg <penberg@kernel.org>, Joonsoo Kim <js1304@gmail.com>,
linux-mm@kvack.org, David Rientjes <rientjes@google.com>
Subject: Re: CK1 [13/13] Common function to create the kmalloc array
Date: Fri, 28 Sep 2012 12:51:51 +0400 [thread overview]
Message-ID: <50656527.9030705@parallels.com> (raw)
In-Reply-To: <0000013a04454f8b-658325fb-88d6-42a2-a7ab-81f82493edb4-000000@email.amazonses.com>
On 09/27/2012 12:29 AM, Christoph Lameter wrote:
> The kmalloc array is created in similar ways in both SLAB
> and SLUB. Create a common function and have both allocators
> call that function.
>
> Signed-off-by: Christoph Lameter <cl@linux.com>
>
The code looks fine and becomes natural at this point, after the
previous preparation changes.
Reviewed-by: Glauber Costa <glommer@parallels.com>
> Index: linux/mm/slab.c
> ===================================================================
> --- linux.orig/mm/slab.c 2012-09-20 08:58:38.621735414 -0500
> +++ linux/mm/slab.c 2012-09-20 08:58:38.673736492 -0500
> @@ -1623,30 +1623,6 @@ void __init kmem_cache_init(void)
>
> slab_early_init = 0;
>
> - for (i = 1; i < PAGE_SHIFT + MAX_ORDER; i++) {
> - size_t cs_size = kmalloc_size(i);
> -
> - if (cs_size < KMALLOC_MIN_SIZE)
> - continue;
> -
> - if (!kmalloc_caches[i]) {
> - /*
> - * For performance, all the general caches are L1 aligned.
> - * This should be particularly beneficial on SMP boxes, as it
> - * eliminates "false sharing".
> - * Note for systems short on memory removing the alignment will
> - * allow tighter packing of the smaller caches.
> - */
> - kmalloc_caches[i] = create_kmalloc_cache("kmalloc",
> - cs_size, ARCH_KMALLOC_FLAGS);
> - }
> -
> -#ifdef CONFIG_ZONE_DMA
> - kmalloc_dma_caches[i] = create_kmalloc_cache(
> - "kmalloc-dma", cs_size,
> - SLAB_CACHE_DMA|ARCH_KMALLOC_FLAGS);
> -#endif
> - }
> /* 4) Replace the bootstrap head arrays */
> {
> struct array_cache *ptr;
> @@ -1692,29 +1668,7 @@ void __init kmem_cache_init(void)
> }
> }
>
> - slab_state = UP;
> -
> - /* Create the proper names */
> - for (i = 1; i < PAGE_SHIFT + MAX_ORDER; i++) {
> - char *s;
> - struct kmem_cache *c = kmalloc_caches[i];
> -
> - if (!c)
> - continue;
> -
> - s = kasprintf(GFP_NOWAIT, "kmalloc-%d", kmalloc_size(i));
> -
> - BUG_ON(!s);
> - c->name = s;
> -
> -#ifdef CONFIG_ZONE_DMA
> - c = kmalloc_dma_caches[i];
> - BUG_ON(!c);
> - s = kasprintf(GFP_NOWAIT, "dma-kmalloc-%d", kmalloc_size(i));
> - BUG_ON(!s);
> - c->name = s;
> -#endif
> - }
> + create_kmalloc_caches(ARCH_KMALLOC_FLAGS);
> }
>
> void __init kmem_cache_init_late(void)
> Index: linux/mm/slab.h
> ===================================================================
> --- linux.orig/mm/slab.h 2012-09-20 08:58:38.565734246 -0500
> +++ linux/mm/slab.h 2012-09-20 08:58:38.673736492 -0500
> @@ -35,6 +35,12 @@ extern struct kmem_cache *kmem_cache;
> unsigned long calculate_alignment(unsigned long flags,
> unsigned long align, unsigned long size);
>
> +#ifndef CONFIG_SLOB
> +/* Kmalloc array related functions */
> +void create_kmalloc_caches(gfp_t);
> +#endif
> +
> +
What is the big problem with having the declaration for slob as well?
An ifdef around a single function signature looks very ugly to me.
> /* Functions provided by the slab allocators */
> extern int __kmem_cache_create(struct kmem_cache *, unsigned long flags);
>
> Index: linux/mm/slab_common.c
> ===================================================================
> --- linux.orig/mm/slab_common.c 2012-09-20 08:58:38.657736160 -0500
> +++ linux/mm/slab_common.c 2012-09-20 08:59:38.554983198 -0500
> @@ -260,4 +260,58 @@ struct kmem_cache *kmalloc_dma_caches[KM
> EXPORT_SYMBOL(kmalloc_dma_caches);
> #endif
>
> +/*
> + * Create the kmalloc array. Some of the regular kmalloc arrays
> + * may already have been created because they were needed to
> + * enable allocations for slab creation.
> + */
> +void __init create_kmalloc_caches(gfp_t flags)
> +{
> + int i;
> +
> + /* Caches that are not of the two-to-the-power-of size */
> + if (KMALLOC_MIN_SIZE <= 32 && !kmalloc_caches[1])
> + kmalloc_caches[1] = create_kmalloc_cache(NULL, 96, flags);
> +
> + if (KMALLOC_MIN_SIZE <= 64 && !kmalloc_caches[2])
> + kmalloc_caches[2] = create_kmalloc_cache(NULL, 192, flags);
> +
> + for (i = KMALLOC_SHIFT_LOW; i <= KMALLOC_SHIFT_HIGH; i++)
> + if (!kmalloc_caches[i])
> + kmalloc_caches[i] = create_kmalloc_cache(NULL,
> + 1 << i, flags);
> +
> + /* Kmalloc array is now usable */
> + slab_state = UP;
> +
> + for (i = 0; i <= KMALLOC_SHIFT_HIGH; i++) {
> + struct kmem_cache *s = kmalloc_caches[i];
> + char *n;
> +
> + if (s) {
> + n = kasprintf(GFP_NOWAIT, "kmalloc-%d", kmalloc_size(i));
> +
> + BUG_ON(!n);
> + s->name = n;
> + }
Isn't it better to write:
if (!s)
continue;
s->name = kasprintf(...);
BUG_ON(!s->name);
?
> + }
> +
> +#ifdef CONFIG_ZONE_DMA
> + for (i = 0; i <= KMALLOC_SHIFT_HIGH; i++) {
> + struct kmem_cache *s = kmalloc_caches[i];
> +
> + if (s) {
> + int size = kmalloc_size(i);
> + char *n = kasprintf(GFP_NOWAIT,
> + "dma-kmalloc-%d", size);
> +
> + BUG_ON(!n);
> + kmalloc_dma_caches[i] = create_kmalloc_cache(n,
> + size, SLAB_CACHE_DMA | flags);
> + }
> + }
> +#endif
> +}
> +
> +
> #endif /* !CONFIG_SLOB */
> Index: linux/mm/slub.c
> ===================================================================
> --- linux.orig/mm/slub.c 2012-09-20 08:58:38.661736249 -0500
> +++ linux/mm/slub.c 2012-09-20 08:58:38.673736492 -0500
> @@ -3628,7 +3628,6 @@ static __initdata struct kmem_cache boot
> void __init kmem_cache_init(void)
> {
> int i;
> - int caches = 2;
>
> if (debug_guardpage_minorder())
> slub_max_order = 0;
> @@ -3703,64 +3702,16 @@ void __init kmem_cache_init(void)
> size_index[size_index_elem(i)] = 8;
> }
>
> - /* Caches that are not of the two-to-the-power-of size */
> - if (KMALLOC_MIN_SIZE <= 32) {
> - kmalloc_caches[1] = create_kmalloc_cache("kmalloc-96", 96, 0);
> - caches++;
> - }
> -
> - if (KMALLOC_MIN_SIZE <= 64) {
> - kmalloc_caches[2] = create_kmalloc_cache("kmalloc-192", 192, 0);
> - caches++;
> - }
> -
> - for (i = KMALLOC_SHIFT_LOW; i <= KMALLOC_SHIFT_HIGH; i++) {
> - kmalloc_caches[i] = create_kmalloc_cache("kmalloc", 1 << i, 0);
> - caches++;
> - }
> -
> - slab_state = UP;
> -
> - /* Provide the correct kmalloc names now that the caches are up */
> - if (KMALLOC_MIN_SIZE <= 32) {
> - kmalloc_caches[1]->name = kstrdup(kmalloc_caches[1]->name, GFP_NOWAIT);
> - BUG_ON(!kmalloc_caches[1]->name);
> - }
> -
> - if (KMALLOC_MIN_SIZE <= 64) {
> - kmalloc_caches[2]->name = kstrdup(kmalloc_caches[2]->name, GFP_NOWAIT);
> - BUG_ON(!kmalloc_caches[2]->name);
> - }
> -
> - for (i = KMALLOC_SHIFT_LOW; i <= KMALLOC_SHIFT_HIGH; i++) {
> - char *s = kasprintf(GFP_NOWAIT, "kmalloc-%d", 1 << i);
> -
> - BUG_ON(!s);
> - kmalloc_caches[i]->name = s;
> - }
> + create_kmalloc_caches(0);
>
> #ifdef CONFIG_SMP
> register_cpu_notifier(&slab_notifier);
> #endif
>
> -#ifdef CONFIG_ZONE_DMA
> - for (i = 0; i <= KMALLOC_SHIFT_HIGH; i++) {
> - struct kmem_cache *s = kmalloc_caches[i];
> -
> - if (s && s->size) {
> - char *name = kasprintf(GFP_NOWAIT,
> - "dma-kmalloc-%d", s->object_size);
> -
> - BUG_ON(!name);
> - kmalloc_dma_caches[i] = create_kmalloc_cache(name,
> - s->object_size, SLAB_CACHE_DMA);
> - }
> - }
> -#endif
> printk(KERN_INFO
> - "SLUB: Genslabs=%d, HWalign=%d, Order=%d-%d, MinObjects=%d,"
> + "SLUB: HWalign=%d, Order=%d-%d, MinObjects=%d,"
> " CPUs=%d, Nodes=%d\n",
> - caches, cache_line_size(),
> + cache_line_size(),
> slub_min_order, slub_max_order, slub_min_objects,
> nr_cpu_ids, nr_node_ids);
> }
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
prev parent reply other threads:[~2012-09-28 8:55 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20120926200005.911809821@linux.com>
2012-09-26 20:01 ` CK1 [02/13] create common functions for boot slab creation Christoph Lameter
2012-09-27 13:22 ` Glauber Costa
2012-09-26 20:01 ` CK1 [01/13] slab: Simplify bootstrap Christoph Lameter
2012-09-26 20:07 ` CK1 [09/13] slab: rename nodelists to node Christoph Lameter
2012-09-28 8:42 ` Glauber Costa
2012-09-28 14:45 ` Christoph Lameter
2012-09-26 20:07 ` CK1 [08/13] slab: Common name for the per node structures Christoph Lameter
2012-09-28 8:38 ` Glauber Costa
2012-09-28 14:21 ` Christoph Lameter
2012-09-26 20:18 ` CK1 [10/13] Do not define KMALLOC array definitions for SLOB Christoph Lameter
2012-09-28 8:44 ` Glauber Costa
2012-09-28 17:11 ` Christoph Lameter
2012-09-26 20:18 ` CK1 [07/13] slab: Use common kmalloc_index/kmalloc_size functions Christoph Lameter
2012-09-28 8:36 ` Glauber Costa
2012-09-28 14:20 ` Christoph Lameter
2012-09-26 20:18 ` CK1 [04/13] slab: Use the new create_boot_cache function to simplify bootstrap Christoph Lameter
2012-09-27 13:24 ` Glauber Costa
2012-09-27 14:32 ` Christoph Lameter
2012-09-27 14:33 ` Glauber Costa
2012-09-26 20:20 ` CK1 [11/13] Common constants for kmalloc boundaries Christoph Lameter
2012-09-26 20:20 ` CK1 [12/13] Common names for the array of kmalloc caches Christoph Lameter
2012-09-26 20:20 ` CK1 [06/13] Common kmalloc slab index determination Christoph Lameter
2012-09-28 8:27 ` Glauber Costa
2012-09-28 14:16 ` Christoph Lameter
2012-09-26 20:20 ` CK1 [05/13] Common alignment code Christoph Lameter
2012-09-26 20:20 ` CK1 [03/13] slub: Use a statically allocated kmem_cache boot structure for bootstrap Christoph Lameter
2012-09-27 13:25 ` Glauber Costa
2012-09-26 20:29 ` CK1 [13/13] Common function to create the kmalloc array Christoph Lameter
2012-09-28 8:51 ` Glauber Costa [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50656527.9030705@parallels.com \
--to=glommer@parallels.com \
--cc=cl@linux.com \
--cc=js1304@gmail.com \
--cc=linux-mm@kvack.org \
--cc=penberg@kernel.org \
--cc=rientjes@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.