mm, slab: Make kmalloc_info[] contain all types of names
Subsystems (2)
cover mm, slab: Make kmalloc_info[] contain all types of names
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().
Patch1 predefines the names of all types of kmalloc to save the time spent dynamically generating names.
The other 4 patches did some cleanup work.
These changes make sense, and the time spent by new_kmalloc_cache() has been reduced by approximately 36.3%.
Time spent by
new_kmalloc_cache()
5.3-rc7 66264
5.3-rc7+patch 42188
Pengfei Li (5):
mm, slab: Make kmalloc_info[] contain all types of names mm, slab_common: Remove unused kmalloc_cache_name() mm, slab: Remove unused kmalloc_size()
mm, slab_common: Make 'type' is enum kmalloc_cache_type mm, slab_common: Make initializing KMALLOC_DMA start from 1
include/linux/slab.h | 20 --------- mm/slab.c | 7 +-- mm/slab.h | 2 +- mm/slab_common.c | 101 +++++++++++++++++++++++-------------------- 4 files changed, 59 insertions(+), 71 deletions(-)
Signature · 1 line
2.21.0
PATCH 1/5 mm, slab: Make kmalloc_info[] contain all types of names
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.
--- mm/slab.c | 2 +- mm/slab.h | 2 +- mm/slab_common.c | 76 +++++++++++++++++++++++++++++++----------------- 3 files changed, 51 insertions(+), 29 deletions(-)
mm/slab.c +1 -1
@@ -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 ;
mm/slab.h +1 -1
@@ -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[];
mm/slab_common.c +49 -27
104 lines not on this page · raw message
Signature · 1 line
2.21.0
Quoted 9 lines · Pengfei Li
There are three types of kmalloc,
KMALLOC_NORMAL,KMALLOC_RECLAIMandKMALLOC_DMA.The name of
KMALLOC_NORMALis contained in kmalloc_info[].name, but the names ofKMALLOC_RECLAIMandKMALLOC_DMAare dynamically generated bykmalloc_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.
Quoted 1 line
Signed-off-by Pengfei Li <[email protected]>
Quoted 6 lines
/*
* 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 structkmalloc_info_structkmalloc_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?
Quoted 9 lines · Vlastimil Babka
Quoted 7 lines · Pengfei Li
There are three types of kmalloc,
KMALLOC_NORMAL,KMALLOC_RECLAIMandKMALLOC_DMA.The name of
KMALLOC_NORMALis contained in kmalloc_info[].name, but the names ofKMALLOC_RECLAIMandKMALLOC_DMAare dynamically generated bykmalloc_cache_name().
Quoted 6 lines
Quoted 2 lines
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.
Quoted 11 lines
Quoted 1 line
Signed-off-by Pengfei Li <[email protected]>Acked-by Vlastimil Babka <[email protected]>Quoted 6 lines
/*
* 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 structkmalloc_info_structkmalloc_info[]__initconst= {
Quoted 3 lines
BTW should it really be an
__initconst, when references to the names keep on living inkmem_cachestructs? Isn't this for data that's discarded after init?
You are right, I will remove __initconst in v2.
Quoted 9 lines · Pengfei Li
Quoted 7 lines · Vlastimil Babka
Quoted 6 lines
/*
* 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 structkmalloc_info_structkmalloc_info[]__initconst= {
Quoted 5 lines
Quoted 3 lines
BTW should it really be an
__initconst, when references to the names keep on living inkmem_cachestructs? Isn't this for data that's discarded after init?You are right, I will remove
__initconstin 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
Quoted 14 lines · Rasmus Villemoes
Quoted 13 lines · Pengfei Li
Quoted 10 lines · Vlastimil Babka
Quoted 6 lines
/*
* 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 structkmalloc_info_structkmalloc_info[]__initconst= {BTW should it really be an
__initconst, when references to the names keep on living inkmem_cachestructs? Isn't this for data that's discarded after init?
Quoted 8 lines
Quoted 1 line
You are right, I will remove
__initconstin v2.No,
__initconstis 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 structkmalloc_info_structconsisted 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.
Quoted 2 lines
Rasmus
Quoted 15 lines · Rasmus Villemoes
Quoted 13 lines · Pengfei Li
Quoted 10 lines · Vlastimil Babka
Quoted 6 lines
/*
* 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 structkmalloc_info_structkmalloc_info[]__initconst= {BTW should it really be an
__initconst, when references to the names keep on living inkmem_cachestructs? Isn't this for data that's discarded after init?
Quoted 9 lines
Quoted 1 line
You are right, I will remove
__initconstin v2.No,
__initconstis 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 structkmalloc_info_structconsisted of { char name[NN]; unsigned int size; }).
Thank you for your comment. I will keep it in v3.
I did learn :)
Quoted 1 line
Rasmus
PATCH 2/5 mm, slab_common: Remove unused kmalloc_cache_name()
In reply to Pengfei LiSince the name of kmalloc can be obtained from kmalloc_info[], remove the kmalloc_cache_name() that is no longer used.
--- mm/slab_common.c | 15 --------------- 1 file changed, 15 deletions(-)
mm/slab_common.c +0 -15
@@ -1191,21 +1191,6 @@ void __init setup_kmalloc_cache_index_table(void) } } -static const char * -kmalloc_cache_name (const char *prefix,unsigned int size) -{ - -static const char units[3 ] ="\0kM" ; -int idx =0 ; - -while (size >=1024 && (size %1024 ==0 )) { - size /=1024 ; - idx++; - } - -return kasprintf (GFP_NOWAIT ,"%s-%u%c" , prefix, size, units[idx]); -} -static void __init new_kmalloc_cache (int idx,int type,slab_flags_t flags) {
Signature · 1 line
2.21.0
Re: [PATCH 2/5] mm, slab_common: Remove unused kmalloc_cache_name()
Quoted 2 lines · Pengfei Li
Since the name of kmalloc can be obtained from kmalloc_info[], remove the
kmalloc_cache_name()that is no longer used.
That could simply be part of patch 1/5 really.
Quoted 1 line
Signed-off-by Pengfei Li <[email protected]>
Ack
Quoted 3 lines
--- mm/slab_common.c | 15 --------------- 1 file changed, 15 deletions(-)
Re: [PATCH 2/5] mm, slab_common: Remove unused kmalloc_cache_name()
Quoted 7 lines · Vlastimil Babka
Quoted 2 lines · Pengfei Li
Since the name of kmalloc can be obtained from kmalloc_info[], remove the
kmalloc_cache_name()that is no longer used.That could simply be part of patch 1/5 really.
Ok, thanks.
Quoted 7 lines
Quoted 1 line
Signed-off-by Pengfei Li <[email protected]>Ack
Quoted 3 lines
--- mm/slab_common.c | 15 --------------- 1 file changed, 15 deletions(-)
PATCH 3/5 mm, slab: Remove unused kmalloc_size()
In reply to Pengfei LiThe size of kmalloc can be obtained from kmalloc_info[], so remove kmalloc_size() that will not be used anymore.
--- include/linux/slab.h | 20 -------------------- mm/slab.c | 5 +++-- mm/slab_common.c | 5 ++--- 3 files changed, 5 insertions(+), 25 deletions(-)
include/linux/slab.h +0 -20
@@ -557,26 +557,6 @@ static __always_inline void *kmalloc(size_t size, gfp_t flags)return __kmalloc (size, flags); } -/* -* Determine size used for the nth kmalloc cache. -* return size or 0 if a kmalloc cache for that -* size does not exist -*/ -static __always_inline unsigned int kmalloc_size(unsigned int n) -{ -#ifndef CONFIG_SLOB -if (n >2 ) -return 1U << n; - -if (n ==1 &&KMALLOC_MIN_SIZE <=32 ) -return 96 ; - -if (n ==2 &&KMALLOC_MIN_SIZE <=64 ) -return 192 ; -#endif -return 0 ; -} - static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node) {#ifndef CONFIG_SLOB
mm/slab.c +3 -2
@@ -1248,8 +1248,9 @@ void __init kmem_cache_init(void)*/ kmalloc_caches[KMALLOC_NORMAL ][INDEX_NODE ] =create_kmalloc_cache ( kmalloc_info[INDEX_NODE ].name[KMALLOC_NORMAL ], -kmalloc_size (INDEX_NODE ),ARCH_KMALLOC_FLAGS , -0 ,kmalloc_size (INDEX_NODE )); + kmalloc_info[INDEX_NODE ].size, +ARCH_KMALLOC_FLAGS ,0 , + kmalloc_info[INDEX_NODE ].size); slab_state =PARTIAL_NODE ;setup_kmalloc_cache_index_table ();
mm/slab_common.c +2 -3
@@ -1239,11 +1239,10 @@ void __init create_kmalloc_caches(slab_flags_t flags)struct kmem_cache *s = kmalloc_caches[KMALLOC_NORMAL ][i];if (s) { -unsigned int size =kmalloc_size (i); - kmalloc_caches[KMALLOC_DMA ][i] =create_kmalloc_cache ( kmalloc_info[i].name[KMALLOC_DMA ], - size,SLAB_CACHE_DMA | flags,0 ,0 ); + kmalloc_info[i].size, +SLAB_CACHE_DMA | flags,0 ,0 ); } }#endif
Signature · 1 line
2.21.0
Re: [PATCH 3/5] mm, slab: Remove unused kmalloc_size()
Quoted 4 lines · Pengfei Li
The size of kmalloc can be obtained from kmalloc_info[], so remove
kmalloc_size()that will not be used anymore.Signed-off-by Pengfei Li <[email protected]>
Quoted 72 lines
--- include/linux/slab.h | 20 -------------------- mm/slab.c | 5 +++-- mm/slab_common.c | 5 ++--- 3 files changed, 5 insertions(+), 25 deletions(-)include/linux/slab.h +0
-20@@ -557,26 +557,6 @@ static __always_inline void *kmalloc(size_t size, gfp_t flags)return __kmalloc (size, flags); } -/* -* Determine size used for the nth kmalloc cache. -* return size or 0 if a kmalloc cache for that -* size does not exist -*/ -static __always_inline unsigned int kmalloc_size(unsigned int n) -{ -#ifndef CONFIG_SLOB -if (n >2 ) -return 1U << n; - -if (n ==1 &&KMALLOC_MIN_SIZE <=32 ) -return 96 ; - -if (n ==2 &&KMALLOC_MIN_SIZE <=64 ) -return 192 ; -#endif -return 0 ; -} - static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node) {#ifndef CONFIG_SLOB mm/slab.c +3
-2@@ -1248,8 +1248,9 @@ void __init kmem_cache_init(void)*/ kmalloc_caches[KMALLOC_NORMAL ][INDEX_NODE ] =create_kmalloc_cache ( kmalloc_info[INDEX_NODE ].name[KMALLOC_NORMAL ], -kmalloc_size (INDEX_NODE ),ARCH_KMALLOC_FLAGS , -0 ,kmalloc_size (INDEX_NODE )); + kmalloc_info[INDEX_NODE ].size, +ARCH_KMALLOC_FLAGS ,0 , + kmalloc_info[INDEX_NODE ].size); slab_state =PARTIAL_NODE ;setup_kmalloc_cache_index_table ();mm/slab_common.c +2
-3@@ -1239,11 +1239,10 @@ void __init create_kmalloc_caches(slab_flags_t flags)struct kmem_cache *s = kmalloc_caches[KMALLOC_NORMAL ][i];if (s) { -unsigned int size =kmalloc_size (i); - kmalloc_caches[KMALLOC_DMA ][i] =create_kmalloc_cache ( kmalloc_info[i].name[KMALLOC_DMA ], - size,SLAB_CACHE_DMA | flags,0 ,0 ); + kmalloc_info[i].size, +SLAB_CACHE_DMA | flags,0 ,0 ); } }#endif
PATCH 4/5 mm, slab_common: Make 'type' is enum kmalloc_cache_type
In reply to Pengfei LiThe 'type' of the function new_kmalloc_cache should be enum kmalloc_cache_type instead of int, so correct it.
--- mm/slab_common.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
mm/slab_common.c +3 -2
@@ -1192,7 +1192,7 @@ void __init setup_kmalloc_cache_index_table(void) }static void __init -new_kmalloc_cache (int idx,int type,slab_flags_t flags) +new_kmalloc_cache (int idx,enum kmalloc_cache_type type,slab_flags_t flags) {if (type ==KMALLOC_RECLAIM ) flags |=SLAB_RECLAIM_ACCOUNT ;
@@ -1210,7 +1210,8 @@ new_kmalloc_cache(int idx, int type, slab_flags_t flags)*/ void __init create_kmalloc_caches (slab_flags_t flags) { -int i, type; +int i; +enum kmalloc_cache_type type;for (type =KMALLOC_NORMAL ; type <=KMALLOC_RECLAIM ; type++) {for (i =KMALLOC_SHIFT_LOW ; i <=KMALLOC_SHIFT_HIGH ; i++) {
Signature · 1 line
2.21.0
Re: [PATCH 4/5] mm, slab_common: Make 'type' is enum kmalloc_cache_type
Quoted 2 lines · Pengfei Li
The 'type' of the function
new_kmalloc_cacheshould be enumkmalloc_cache_typeinstead of int, so correct it.
OK
Quoted 1 line
Signed-off-by Pengfei Li <[email protected]>
Quoted 28 lines
--- mm/slab_common.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)mm/slab_common.c +3
-2@@ -1192,7 +1192,7 @@ void __init setup_kmalloc_cache_index_table(void) }static void __init -new_kmalloc_cache (int idx,int type,slab_flags_t flags) +new_kmalloc_cache (int idx,enum kmalloc_cache_type type,slab_flags_t flags) {if (type ==KMALLOC_RECLAIM ) flags |=SLAB_RECLAIM_ACCOUNT ;@@ -1210,7 +1210,8 @@ new_kmalloc_cache(int idx, int type, slab_flags_t flags)*/ void __init create_kmalloc_caches (slab_flags_t flags) { -int i, type; +int i; +enum kmalloc_cache_type type;for (type =KMALLOC_NORMAL ; type <=KMALLOC_RECLAIM ; type++) {for (i =KMALLOC_SHIFT_LOW ; i <=KMALLOC_SHIFT_HIGH ; i++) {
PATCH 5/5 mm, slab_common: Make initializing KMALLOC_DMA start from 1
In reply to Pengfei Likmalloc_caches[KMALLOC_NORMAL][0] will never be initialized, so the loop should start at 1 instead of 0
--- mm/slab_common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
mm/slab_common.c +1 -1
@@ -1236,7 +1236,7 @@ void __init create_kmalloc_caches(slab_flags_t flags) slab_state =UP ;#ifdef CONFIG_ZONE_DMA -for (i =0 ; i <=KMALLOC_SHIFT_HIGH ; i++) { +for (i =1 ; i <=KMALLOC_SHIFT_HIGH ; i++) {struct kmem_cache *s = kmalloc_caches[KMALLOC_NORMAL ][i];if (s) {
Signature · 1 line
2.21.0
Quoted 2 lines · Pengfei Li
There are three types of kmalloc,
KMALLOC_NORMAL,KMALLOC_RECLAIMandKMALLOC_DMA.
I only got a few patches of this set. Can I see the complete patchset somewhere?
Quoted 8 lines · Christopher Lameter
Quoted 2 lines · Pengfei Li
There are three types of kmalloc,
KMALLOC_NORMAL,KMALLOC_RECLAIMandKMALLOC_DMA.I only got a few patches of this set. Can I see the complete patchset somewhere?
Yes, you can get the patches from
https://patchwork.kernel.org/cover/11128325/ or
https://lore.kernel.org/patchwork/cover/1123412/
Hope you like it :)
Quoted 12 lines · Pengfei Li
There are three types of kmalloc,
KMALLOC_NORMAL,KMALLOC_RECLAIMandKMALLOC_DMA.The name of
KMALLOC_NORMALis contained in kmalloc_info[].name, but the names ofKMALLOC_RECLAIMandKMALLOC_DMAare dynamically generated bykmalloc_cache_name().Patch1 predefines the names of all types of kmalloc to save the time spent dynamically generating names.
The other 4 patches did some cleanup work.
Quoted 7 lines
These changes make sense, and the time spent by
new_kmalloc_cache()has been reduced by approximately 36.3%.Time spent by
new_kmalloc_cache()
5.3-rc7 66264
5.3-rc7+patch 42188
Note that the caches are created only once upon boot, so I doubt that these time savings (is it in CPU cycles?) will be noticeable at all. But diffstat looks ok, and it avoids using kmalloc() (via kasprintf()) to allocate names for kmalloc(), so in that sense I think it's worthwhile to consider. Thanks.
Quoted 13 lines
Pengfei Li (5):
mm, slab: Make kmalloc_info[] contain all types of names mm,slab_common: Remove unusedkmalloc_cache_name()mm, slab: Remove unusedkmalloc_size()
mm,slab_common: Make 'type' is enumkmalloc_cache_typemm,slab_common: Make initializingKMALLOC_DMAstart from 1include/linux/slab.h | 20 --------- mm/slab.c | 7 +-- mm/slab.h | 2 +- mm/slab_common.c | 101 +++++++++++++++++++++++-------------------- 4 files changed, 59 insertions(+), 71 deletions(-)
Quoted 17 lines · Vlastimil Babka
Quoted 15 lines · Pengfei Li
There are three types of kmalloc,
KMALLOC_NORMAL,KMALLOC_RECLAIMandKMALLOC_DMA.The name of
KMALLOC_NORMALis contained in kmalloc_info[].name, but the names ofKMALLOC_RECLAIMandKMALLOC_DMAare dynamically generated bykmalloc_cache_name().Patch1 predefines the names of all types of kmalloc to save the time spent dynamically generating names.
The other 4 patches did some cleanup work.
These changes make sense, and the time spent by
new_kmalloc_cache()has been reduced by approximately 36.3%.
Quoted 6 lines
Quoted 4 lines
Time spent by
new_kmalloc_cache()
5.3-rc7 66264
5.3-rc7+patch 42188Note that the caches are created only once upon boot, so I doubt that
Thank you for your comments.
Yes, kmalloc-xxx are only created at boot time.
Quoted 1 line
these time savings (is it in CPU cycles?) will be noticeable at all.
Yes, it is CPU cycles.
Quoted 4 lines
But diffstat looks ok, and it avoids using
kmalloc()(viakasprintf()) to allocate names forkmalloc(), so in that sense I think it's worthwhile to consider. Thanks.
Thanks.
Quoted 14 lines
Quoted 13 lines
Pengfei Li (5):
mm, slab: Make kmalloc_info[] contain all types of names mm,slab_common: Remove unusedkmalloc_cache_name()mm, slab: Remove unusedkmalloc_size()
mm,slab_common: Make 'type' is enumkmalloc_cache_typemm,slab_common: Make initializingKMALLOC_DMAstart from 1include/linux/slab.h | 20 --------- mm/slab.c | 7 +-- mm/slab.h | 2 +- mm/slab_common.c | 101 +++++++++++++++++++++++-------------------- 4 files changed, 59 insertions(+), 71 deletions(-)