Skip to contentSkip to conversation

mm, slab: remove unnecessary unlikely()

PATCH In review Yangtao Li 4 participants 5 messages Nov 4, 2018 – Nov 7, 2018

Subsystems (2)
Memory management Slab allocator
lore
Share & tools
lore

Yangtao Li

author
#0 lore raw

PATCH 1/1 mm, slab: remove unnecessary unlikely()

WARN_ON() already contains an unlikely(), so it's not necessary to use unlikely.

Signed-off-by Yangtao Li <[email protected]>
---
 mm/slab_common.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
mm/slab_common.c +1 -3
index 7eb8dc136c1c..4f54684f5435 100644
@@ -1029,10 +1029,8 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
 
 		index = size_index[size_index_elem(size)];
 	} else {
-		if (unlikely(size > KMALLOC_MAX_CACHE_SIZE)) {
-			WARN_ON(1);
+		if (WARN_ON(size > KMALLOC_MAX_CACHE_SIZE))
 			return NULL;
-		}
 		index = fls(size - 1);
 	}
Signature · 1 line
2.17.0
Acked-by on v1 1/1
#1 lore raw

+CC Dmitry

Quoted 4 lines · Yangtao Li

WARN_ON() already contains an unlikely(), so it's not necessary to use unlikely.

Signed-off-by Yangtao Li <[email protected]>
Acked-by Vlastimil Babka <[email protected]>

Maybe also change it back to WARN_ON_ONCE? I already considered it while reviewing Dmitry's patch and wasn't sure. Now I think that what can happen is that either a kernel bug is introduced that _ONCE is enough to catch (two separate bugs introduced to both hit this would be rare, and in that case the second one will be reported after the first one is fixed), or this gets called with a user-supplied value, and then we want to avoid spamming dmesg with multiple warnings that the user could trigger at will.

Quoted 21 lines
---
 mm/slab_common.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
mm/slab_common.c +1 -3
index 7eb8dc136c1c..4f54684f5435 100644
@@ -1029,10 +1029,8 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
 
 		index = size_index[size_index_elem(size)];
 	} else {
-		if (unlikely(size > KMALLOC_MAX_CACHE_SIZE)) {
-			WARN_ON(1);
+		if (WARN_ON(size > KMALLOC_MAX_CACHE_SIZE))
 			return NULL;
-		}
 		index = fls(size - 1);
 	}
#2 lore raw
Quoted 10 lines · Vlastimil Babka

+CC Dmitry

Quoted 4 lines · Yangtao Li

WARN_ON() already contains an unlikely(), so it's not necessary to use unlikely.

Signed-off-by Yangtao Li <[email protected]>
Acked-by Vlastimil Babka <[email protected]>
Quoted 8 lines

Maybe also change it back to WARN_ON_ONCE? I already considered it while reviewing Dmitry's patch and wasn't sure. Now I think that what can happen is that either a kernel bug is introduced that _ONCE is enough to catch (two separate bugs introduced to both hit this would be rare, and in that case the second one will be reported after the first one is fixed), or this gets called with a user-supplied value, and then we want to avoid spamming dmesg with multiple warnings that the user could trigger at will.

If you asking me, I am fine both changes.
I was mainly interested in removing the bogus warnings that actually fire.

Quoted 22 lines
Quoted 21 lines
---
 mm/slab_common.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
mm/slab_common.c +1 -3
index 7eb8dc136c1c..4f54684f5435 100644
@@ -1029,10 +1029,8 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
 
              index = size_index[size_index_elem(size)];
      } else {
-             if (unlikely(size > KMALLOC_MAX_CACHE_SIZE)) {
-                     WARN_ON(1);
+             if (WARN_ON(size > KMALLOC_MAX_CACHE_SIZE))
                      return NULL;
-             }
              index = fls(size - 1);
      }
#3 lore raw
Quoted 21 lines · Dmitry Vyukov
Quoted 10 lines · Vlastimil Babka

+CC Dmitry

Quoted 4 lines · Yangtao Li

WARN_ON() already contains an unlikely(), so it's not necessary to use unlikely.

Signed-off-by Yangtao Li <[email protected]>
Acked-by Vlastimil Babka <[email protected]>
Quoted 8 lines

Maybe also change it back to WARN_ON_ONCE? I already considered it while reviewing Dmitry's patch and wasn't sure. Now I think that what can happen is that either a kernel bug is introduced that _ONCE is enough to catch (two separate bugs introduced to both hit this would be rare, and in that case the second one will be reported after the first one is fixed), or this gets called with a user-supplied value, and then we want to avoid spamming dmesg with multiple warnings that the user could trigger at will.

Quoted 2 lines

If you asking me, I am fine both changes.
I was mainly interested in removing the bogus warnings that actually fire.

OK thanks. Andrew can you update the patch to WARN_ON_ONCE?

Changelog addition:
Also change WARN_ON() back to WARN_ON_ONCE() to avoid potentially spamming dmesg with user-triggerable large allocations.

Quoted 23 lines
Quoted 23 lines
Quoted 22 lines
---
 mm/slab_common.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
mm/slab_common.c +1 -3
index 7eb8dc136c1c..4f54684f5435 100644
@@ -1029,10 +1029,8 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
 
              index = size_index[size_index_elem(size)];
      } else {
-             if (unlikely(size > KMALLOC_MAX_CACHE_SIZE)) {
-                     WARN_ON(1);
+             if (WARN_ON(size > KMALLOC_MAX_CACHE_SIZE))
                      return NULL;
-             }
              index = fls(size - 1);
      }