Skip to contentSkip to conversation

mm, slab: Make kmalloc_info[] contain all types of names

PATCH Superseded Pengfei Li 4 participants 19 messages Sep 3, 2019 – Sep 10, 2019

Subsystems (2)

v1 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Pengfei Li

author
#0 lore raw

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
author
#1 lore raw

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
index 9df370558e5d..c42b6211f42e 100644
@@ -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
index 9057b8056b07..2fc8f956906a 100644
@@ -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
Acked-by on v1 1/5
#2 lore raw
Quoted 9 lines · Pengfei Li

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.

Quoted 1 line
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 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?

author
#3 lore raw
Quoted 9 lines · Vlastimil Babka
Quoted 7 lines · Pengfei Li

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().

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
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 struct kmalloc_info_struct kmalloc_info[] __initconst = {

Quoted 3 lines

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.

#4 lore raw
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 struct kmalloc_info_struct kmalloc_info[] __initconst = {

Quoted 5 lines
Quoted 3 lines

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

#5 lore raw
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 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?

Quoted 8 lines
Quoted 1 line

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.

Quoted 2 lines

Rasmus

author
#6 lore raw
In reply to Rasmus Villemoes
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 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?

Quoted 9 lines
Quoted 1 line

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 :)

Quoted 1 line

Rasmus

author
#7 lore raw

PATCH 2/5 mm, slab_common: Remove unused kmalloc_cache_name()

In reply to Pengfei Li

Since 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
index 7bd88cc09987..002e16673581 100644
@@ -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
#8 lore raw

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

Ack

Quoted 3 lines
---
  mm/slab_common.c | 15 ---------------
  1 file changed, 15 deletions(-)
author
#9 lore raw

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

Ack

Quoted 3 lines
---
  mm/slab_common.c | 15 ---------------
  1 file changed, 15 deletions(-)
author
#10 lore raw

PATCH 3/5 mm, slab: Remove unused kmalloc_size()

In reply to Pengfei Li

The 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
index 56c9c7eed34e..e773e5764b7b 100644
@@ -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
index c42b6211f42e..7bc4e90e1147 100644
@@ -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
index 002e16673581..8b542cfcc4f2 100644
@@ -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
Acked-by on v1 3/5
#11 lore raw

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.

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
index 56c9c7eed34e..e773e5764b7b 100644
@@ -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
index c42b6211f42e..7bc4e90e1147 100644
@@ -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
index 002e16673581..8b542cfcc4f2 100644
@@ -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
author
#12 lore raw

PATCH 4/5 mm, slab_common: Make 'type' is enum kmalloc_cache_type

In reply to Pengfei Li

The '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
index 8b542cfcc4f2..af45b5278fdc 100644
@@ -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
Acked-by on v1 4/5
#13 lore raw

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_cache should be enum kmalloc_cache_type instead of int, so correct it.

OK

Quoted 1 line
Quoted 28 lines
---
  mm/slab_common.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
mm/slab_common.c +3 -2
index 8b542cfcc4f2..af45b5278fdc 100644
@@ -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++) {
author
#14 lore raw

PATCH 5/5 mm, slab_common: Make initializing KMALLOC_DMA start from 1

In reply to Pengfei Li

kmalloc_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
index af45b5278fdc..c81fc7dc2946 100644
@@ -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
#15 lore raw
In reply to Pengfei Li
Quoted 2 lines · Pengfei Li

There are three types of kmalloc, KMALLOC_NORMAL, KMALLOC_RECLAIM and KMALLOC_DMA.

I only got a few patches of this set. Can I see the complete patchset somewhere?

#17 lore raw
In reply to Pengfei Li
Quoted 12 lines · Pengfei Li

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.

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 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(-)
author
#18 lore raw
Quoted 17 lines · Vlastimil Babka
Quoted 15 lines · Pengfei Li

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%.

Quoted 6 lines
Quoted 4 lines

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

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() (via kasprintf()) to allocate names for kmalloc(), 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 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(-)