| Message ID | 20190903160430.1368-2-lpf.vector@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm, slab: Make kmalloc_info[] contain all types of names | expand |
On 9/3/19 6:04 PM, Pengfei Li wrote: > There are three types of kmalloc, KMALLOC_NORMAL, KMALLOC_RECLAIM > and KMALLOC_DMA. > > The name of KMALLOC_NORMAL is contained in kmalloc_info[].name, > but the names of KMALLOC_RECLAIM and KMALLOC_DMA are dynamically > generated by kmalloc_cache_name(). > > This patch predefines the names of all types of kmalloc to save > the time spent dynamically generating names. As I said, IMHO it's more useful that we don't need to allocate the names dynamically anymore, and it's simpler overall. > Signed-off-by: Pengfei Li <lpf.vector@gmail.com> Acked-by: Vlastimil Babka <vbabka@suse.cz> > /* > * kmalloc_info[] is to make slub_debug=,kmalloc-xx option work at boot time. > * kmalloc_index() supports up to 2^26=64MB, so the final entry of the table is > * kmalloc-67108864. > */ > const struct kmalloc_info_struct kmalloc_info[] __initconst = { BTW should it really be an __initconst, when references to the names keep on living in kmem_cache structs? Isn't this for data that's discarded after init?
On Mon, Sep 9, 2019 at 10:59 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 9/3/19 6:04 PM, Pengfei Li wrote: > > There are three types of kmalloc, KMALLOC_NORMAL, KMALLOC_RECLAIM > > and KMALLOC_DMA. > > > > The name of KMALLOC_NORMAL is contained in kmalloc_info[].name, > > but the names of KMALLOC_RECLAIM and KMALLOC_DMA are dynamically > > generated by kmalloc_cache_name(). > > > > This patch predefines the names of all types of kmalloc to save > > the time spent dynamically generating names. > > As I said, IMHO it's more useful that we don't need to allocate the > names dynamically anymore, and it's simpler overall. > Thank you very much for your review. > > Signed-off-by: Pengfei Li <lpf.vector@gmail.com> > > Acked-by: Vlastimil Babka <vbabka@suse.cz> > > > /* > > * kmalloc_info[] is to make slub_debug=,kmalloc-xx option work at boot time. > > * kmalloc_index() supports up to 2^26=64MB, so the final entry of the table is > > * kmalloc-67108864. > > */ > > const struct kmalloc_info_struct kmalloc_info[] __initconst = { > > BTW should it really be an __initconst, when references to the names > keep on living in kmem_cache structs? Isn't this for data that's > discarded after init? You are right, I will remove __initconst in v2.
On 09/09/2019 18.53, Pengfei Li wrote: > On Mon, Sep 9, 2019 at 10:59 PM Vlastimil Babka <vbabka@suse.cz> wrote: >>> /* >>> * kmalloc_info[] is to make slub_debug=,kmalloc-xx option work at boot time. >>> * kmalloc_index() supports up to 2^26=64MB, so the final entry of the table is >>> * kmalloc-67108864. >>> */ >>> const struct kmalloc_info_struct kmalloc_info[] __initconst = { >> >> BTW should it really be an __initconst, when references to the names >> keep on living in kmem_cache structs? Isn't this for data that's >> discarded after init? > > You are right, I will remove __initconst in v2. No, __initconst is correct, and should be kept. The string literals which the .name pointers point to live in .rodata, and we're copying the values of these .name pointers. Nothing refers to something inside kmalloc_info[] after init. (It would be a whole different matter if struct kmalloc_info_struct consisted of { char name[NN]; unsigned int size; }). Rasmus
On 9/9/19 8:30 PM, Rasmus Villemoes wrote: > On 09/09/2019 18.53, Pengfei Li wrote: >> On Mon, Sep 9, 2019 at 10:59 PM Vlastimil Babka <vbabka@suse.cz> wrote: > >>>> /* >>>> * kmalloc_info[] is to make slub_debug=,kmalloc-xx option work at boot time. >>>> * kmalloc_index() supports up to 2^26=64MB, so the final entry of the table is >>>> * kmalloc-67108864. >>>> */ >>>> const struct kmalloc_info_struct kmalloc_info[] __initconst = { >>> >>> BTW should it really be an __initconst, when references to the names >>> keep on living in kmem_cache structs? Isn't this for data that's >>> discarded after init? >> >> You are right, I will remove __initconst in v2. > > No, __initconst is correct, and should be kept. The string literals > which the .name pointers point to live in .rodata, and we're copying the > values of these .name pointers. Nothing refers to something inside > kmalloc_info[] after init. (It would be a whole different matter if > struct kmalloc_info_struct consisted of { char name[NN]; unsigned int > size; }). *slaps forehead* ah, of course, string literals themselves are not affected by the __initconst, thanks! Sorry for the wrong suggestion Pengfei. > Rasmus >
On Tue, Sep 10, 2019 at 2:30 AM Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > > On 09/09/2019 18.53, Pengfei Li wrote: > > On Mon, Sep 9, 2019 at 10:59 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > >>> /* > >>> * kmalloc_info[] is to make slub_debug=,kmalloc-xx option work at boot time. > >>> * kmalloc_index() supports up to 2^26=64MB, so the final entry of the table is > >>> * kmalloc-67108864. > >>> */ > >>> const struct kmalloc_info_struct kmalloc_info[] __initconst = { > >> > >> BTW should it really be an __initconst, when references to the names > >> keep on living in kmem_cache structs? Isn't this for data that's > >> discarded after init? > > > > You are right, I will remove __initconst in v2. > > No, __initconst is correct, and should be kept. The string literals > which the .name pointers point to live in .rodata, and we're copying the > values of these .name pointers. Nothing refers to something inside > kmalloc_info[] after init. (It would be a whole different matter if > struct kmalloc_info_struct consisted of { char name[NN]; unsigned int > size; }). > Thank you for your comment. I will keep it in v3. I did learn :) > Rasmus
diff --git a/mm/slab.c b/mm/slab.c index 9df370558e5d..c42b6211f42e 100644 --- a/mm/slab.c +++ b/mm/slab.c @@ -1247,7 +1247,7 @@ void __init kmem_cache_init(void) * structures first. Without this, further allocations will bug. */ kmalloc_caches[KMALLOC_NORMAL][INDEX_NODE] = create_kmalloc_cache( - kmalloc_info[INDEX_NODE].name, + kmalloc_info[INDEX_NODE].name[KMALLOC_NORMAL], kmalloc_size(INDEX_NODE), ARCH_KMALLOC_FLAGS, 0, kmalloc_size(INDEX_NODE)); slab_state = PARTIAL_NODE; diff --git a/mm/slab.h b/mm/slab.h index 9057b8056b07..2fc8f956906a 100644 --- a/mm/slab.h +++ b/mm/slab.h @@ -76,7 +76,7 @@ extern struct kmem_cache *kmem_cache; /* A table of kmalloc cache names and sizes */ extern const struct kmalloc_info_struct { - const char *name; + const char *name[NR_KMALLOC_TYPES]; unsigned int size; } kmalloc_info[]; diff --git a/mm/slab_common.c b/mm/slab_common.c index 807490fe217a..7bd88cc09987 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -1092,26 +1092,56 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags) return kmalloc_caches[kmalloc_type(flags)][index]; } +#ifdef CONFIG_ZONE_DMA +#define SET_KMALLOC_SIZE(__size, __short_size) \ +{ \ + .name[KMALLOC_NORMAL] = "kmalloc-" #__short_size, \ + .name[KMALLOC_RECLAIM] = "kmalloc-rcl-" #__short_size, \ + .name[KMALLOC_DMA] = "dma-kmalloc-" #__short_size, \ + .size = __size, \ +} +#else +#define SET_KMALLOC_SIZE(__size, __short_size) \ +{ \ + .name[KMALLOC_NORMAL] = "kmalloc-" #__short_size, \ + .name[KMALLOC_RECLAIM] = "kmalloc-rcl-" #__short_size, \ + .size = __size, \ +} +#endif + /* * kmalloc_info[] is to make slub_debug=,kmalloc-xx option work at boot time. * kmalloc_index() supports up to 2^26=64MB, so the final entry of the table is * kmalloc-67108864. */ const struct kmalloc_info_struct kmalloc_info[] __initconst = { - {NULL, 0}, {"kmalloc-96", 96}, - {"kmalloc-192", 192}, {"kmalloc-8", 8}, - {"kmalloc-16", 16}, {"kmalloc-32", 32}, - {"kmalloc-64", 64}, {"kmalloc-128", 128}, - {"kmalloc-256", 256}, {"kmalloc-512", 512}, - {"kmalloc-1k", 1024}, {"kmalloc-2k", 2048}, - {"kmalloc-4k", 4096}, {"kmalloc-8k", 8192}, - {"kmalloc-16k", 16384}, {"kmalloc-32k", 32768}, - {"kmalloc-64k", 65536}, {"kmalloc-128k", 131072}, - {"kmalloc-256k", 262144}, {"kmalloc-512k", 524288}, - {"kmalloc-1M", 1048576}, {"kmalloc-2M", 2097152}, - {"kmalloc-4M", 4194304}, {"kmalloc-8M", 8388608}, - {"kmalloc-16M", 16777216}, {"kmalloc-32M", 33554432}, - {"kmalloc-64M", 67108864} + SET_KMALLOC_SIZE(0, 0), + SET_KMALLOC_SIZE(96, 96), + SET_KMALLOC_SIZE(192, 192), + SET_KMALLOC_SIZE(8, 8), + SET_KMALLOC_SIZE(16, 16), + SET_KMALLOC_SIZE(32, 32), + SET_KMALLOC_SIZE(64, 64), + SET_KMALLOC_SIZE(128, 128), + SET_KMALLOC_SIZE(256, 256), + SET_KMALLOC_SIZE(512, 512), + SET_KMALLOC_SIZE(1024, 1k), + SET_KMALLOC_SIZE(2048, 2k), + SET_KMALLOC_SIZE(4096, 4k), + SET_KMALLOC_SIZE(8192, 8k), + SET_KMALLOC_SIZE(16384, 16k), + SET_KMALLOC_SIZE(32768, 32k), + SET_KMALLOC_SIZE(65536, 64k), + SET_KMALLOC_SIZE(131072, 128k), + SET_KMALLOC_SIZE(262144, 256k), + SET_KMALLOC_SIZE(524288, 512k), + SET_KMALLOC_SIZE(1048576, 1M), + SET_KMALLOC_SIZE(2097152, 2M), + SET_KMALLOC_SIZE(4194304, 4M), + SET_KMALLOC_SIZE(8388608, 8M), + SET_KMALLOC_SIZE(16777216, 16M), + SET_KMALLOC_SIZE(33554432, 32M), + SET_KMALLOC_SIZE(67108864, 64M) }; /* @@ -1179,18 +1209,11 @@ kmalloc_cache_name(const char *prefix, unsigned int size) static void __init new_kmalloc_cache(int idx, int type, slab_flags_t flags) { - const char *name; - - if (type == KMALLOC_RECLAIM) { + if (type == KMALLOC_RECLAIM) flags |= SLAB_RECLAIM_ACCOUNT; - name = kmalloc_cache_name("kmalloc-rcl", - kmalloc_info[idx].size); - BUG_ON(!name); - } else { - name = kmalloc_info[idx].name; - } - kmalloc_caches[type][idx] = create_kmalloc_cache(name, + kmalloc_caches[type][idx] = create_kmalloc_cache( + kmalloc_info[idx].name[type], kmalloc_info[idx].size, flags, 0, kmalloc_info[idx].size); } @@ -1232,11 +1255,10 @@ void __init create_kmalloc_caches(slab_flags_t flags) if (s) { unsigned int size = kmalloc_size(i); - const char *n = kmalloc_cache_name("dma-kmalloc", size); - BUG_ON(!n); kmalloc_caches[KMALLOC_DMA][i] = create_kmalloc_cache( - n, size, SLAB_CACHE_DMA | flags, 0, 0); + kmalloc_info[i].name[KMALLOC_DMA], + size, SLAB_CACHE_DMA | flags, 0, 0); } } #endif
There are three types of kmalloc, KMALLOC_NORMAL, KMALLOC_RECLAIM and KMALLOC_DMA. The name of KMALLOC_NORMAL is contained in kmalloc_info[].name, but the names of KMALLOC_RECLAIM and KMALLOC_DMA are dynamically generated by kmalloc_cache_name(). This patch predefines the names of all types of kmalloc to save the time spent dynamically generating names. Signed-off-by: Pengfei Li <lpf.vector@gmail.com> --- mm/slab.c | 2 +- mm/slab.h | 2 +- mm/slab_common.c | 76 +++++++++++++++++++++++++++++++----------------- 3 files changed, 51 insertions(+), 29 deletions(-)