diff mbox series

[v3,09/11] mm/slub: Fold slab_update_freelist()

Message ID 20230515080554.520976397@infradead.org
State New
Headers
Series Introduce cmpxchg128() -- aka. the demise of cmpxchg_double() |

Commit Message

Peter Zijlstra May 15, 2023, 7:57 a.m. UTC 
  The two functions slab_update_freelist() and __slab_update_freelist()
are nearly identical, fold and add a boolean argument and rely on
constant propagation.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 mm/slub.c |   80 +++++++++++++++++++++-----------------------------------------
 1 file changed, 28 insertions(+), 52 deletions(-)

Comments

Vlastimil Babka May 24, 2023, 11:58 a.m. UTC | #1 
On 5/15/23 09:57, Peter Zijlstra wrote:
> The two functions slab_update_freelist() and __slab_update_freelist()
> are nearly identical, fold and add a boolean argument and rely on
> constant propagation.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Something like that has been tried before and the result:
https://lore.kernel.org/all/CAHk-=wiJLqL2cUhJbvpyPQpkbVOu1rVSzgO2=S2jC55hneLtfQ@mail.gmail.com/

Your parameter is not called 'locked' but 'irq_save' which is better, but
that's just one detail.

After your refactoring in 08/11 which puts most of the code into
__update_freelist_fast() and _slow() I'd say the result is not so bad already.

BTW I have some suspicion that some SLUB code is based on assumptions that
are no longer true these days. IIRC I've seen some microbenchmark results a
while ago that showed that disabling/enabling irqs is surprisingly (to me)
very cheap today, so maybe it's not so useful to keep doing the
this_cpu_cmpxchg128 for the struct kmem_cache_cpu operations (less so for
struct slab cmpxchg128 where actually different cpus may be involved). But
it needs a closer look.

> ---
>  mm/slub.c |   80 +++++++++++++++++++++-----------------------------------------
>  1 file changed, 28 insertions(+), 52 deletions(-)
> 
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -559,53 +559,29 @@ __update_freelist_slow(struct slab *slab
>   * allocation/ free operation in hardirq context. Therefore nothing can
>   * interrupt the operation.
>   */
> -static inline bool __slab_update_freelist(struct kmem_cache *s, struct slab *slab,
> -		void *freelist_old, unsigned long counters_old,
> -		void *freelist_new, unsigned long counters_new,
> -		const char *n)
> +static __always_inline
> +bool slab_update_freelist(struct kmem_cache *s, struct slab *slab,
> +			  void *freelist_old, unsigned long counters_old,
> +			  void *freelist_new, unsigned long counters_new,
> +			  bool irq_save, const char *n)
>  {
>  	bool ret;
>  
> -	if (USE_LOCKLESS_FAST_PATH())
> +	if (!irq_save && USE_LOCKLESS_FAST_PATH())
>  		lockdep_assert_irqs_disabled();
>  
>  	if (s->flags & __CMPXCHG_DOUBLE) {
>  		ret = __update_freelist_fast(slab, freelist_old, counters_old,
>  				            freelist_new, counters_new);
>  	} else {
> -		ret = __update_freelist_slow(slab, freelist_old, counters_old,
> -				            freelist_new, counters_new);
> -	}
> -	if (likely(ret))
> -		return true;
> -
> -	cpu_relax();
> -	stat(s, CMPXCHG_DOUBLE_FAIL);
> -
> -#ifdef SLUB_DEBUG_CMPXCHG
> -	pr_info("%s %s: cmpxchg double redo ", n, s->name);
> -#endif
> -
> -	return false;
> -}
> -
> -static inline bool slab_update_freelist(struct kmem_cache *s, struct slab *slab,
> -		void *freelist_old, unsigned long counters_old,
> -		void *freelist_new, unsigned long counters_new,
> -		const char *n)
> -{
> -	bool ret;
> -
> -	if (s->flags & __CMPXCHG_DOUBLE) {
> -		ret = __update_freelist_fast(slab, freelist_old, counters_old,
> -				            freelist_new, counters_new);
> -	} else {
>  		unsigned long flags;
>  
> -		local_irq_save(flags);
> +		if (irq_save)
> +			local_irq_save(flags);
>  		ret = __update_freelist_slow(slab, freelist_old, counters_old,
>  				            freelist_new, counters_new);
> -		local_irq_restore(flags);
> +		if (irq_save)
> +			local_irq_restore(flags);
>  	}
>  	if (likely(ret))
>  		return true;
> @@ -2250,10 +2226,10 @@ static inline void *acquire_slab(struct
>  	VM_BUG_ON(new.frozen);
>  	new.frozen = 1;
>  
> -	if (!__slab_update_freelist(s, slab,
> -			freelist, counters,
> -			new.freelist, new.counters,
> -			"acquire_slab"))
> +	if (!slab_update_freelist(s, slab,
> +				  freelist, counters,
> +				  new.freelist, new.counters,
> +				  false, "acquire_slab"))
>  		return NULL;
>  
>  	remove_partial(n, slab);
> @@ -2577,9 +2553,9 @@ static void deactivate_slab(struct kmem_
>  
>  
>  	if (!slab_update_freelist(s, slab,
> -				old.freelist, old.counters,
> -				new.freelist, new.counters,
> -				"unfreezing slab")) {
> +				  old.freelist, old.counters,
> +				  new.freelist, new.counters,
> +				  true, "unfreezing slab")) {
>  		if (mode == M_PARTIAL)
>  			spin_unlock_irqrestore(&n->list_lock, flags);
>  		goto redo;
> @@ -2633,10 +2609,10 @@ static void __unfreeze_partials(struct k
>  
>  			new.frozen = 0;
>  
> -		} while (!__slab_update_freelist(s, slab,
> -				old.freelist, old.counters,
> -				new.freelist, new.counters,
> -				"unfreezing slab"));
> +		} while (!slab_update_freelist(s, slab,
> +					       old.freelist, old.counters,
> +					       new.freelist, new.counters,
> +					       false, "unfreezing slab"));
>  
>  		if (unlikely(!new.inuse && n->nr_partial >= s->min_partial)) {
>  			slab->next = slab_to_discard;
> @@ -3072,10 +3048,10 @@ static inline void *get_freelist(struct
>  		new.inuse = slab->objects;
>  		new.frozen = freelist != NULL;
>  
> -	} while (!__slab_update_freelist(s, slab,
> -		freelist, counters,
> -		NULL, new.counters,
> -		"get_freelist"));
> +	} while (!slab_update_freelist(s, slab,
> +				       freelist, counters,
> +				       NULL, new.counters,
> +				       false, "get_freelist"));
>  
>  	return freelist;
>  }
> @@ -3666,9 +3642,9 @@ static void __slab_free(struct kmem_cach
>  		}
>  
>  	} while (!slab_update_freelist(s, slab,
> -		prior, counters,
> -		head, new.counters,
> -		"__slab_free"));
> +				       prior, counters,
> +				       head, new.counters,
> +				       true, "__slab_free"));
>  
>  	if (likely(!n)) {
>  
> 
>
diff mbox series

Patch

--- a/mm/slub.c
+++ b/mm/slub.c
@@ -559,53 +559,29 @@  __update_freelist_slow(struct slab *slab
  * allocation/ free operation in hardirq context. Therefore nothing can
  * interrupt the operation.
  */
-static inline bool __slab_update_freelist(struct kmem_cache *s, struct slab *slab,
-		void *freelist_old, unsigned long counters_old,
-		void *freelist_new, unsigned long counters_new,
-		const char *n)
+static __always_inline
+bool slab_update_freelist(struct kmem_cache *s, struct slab *slab,
+			  void *freelist_old, unsigned long counters_old,
+			  void *freelist_new, unsigned long counters_new,
+			  bool irq_save, const char *n)
 {
 	bool ret;
 
-	if (USE_LOCKLESS_FAST_PATH())
+	if (!irq_save && USE_LOCKLESS_FAST_PATH())
 		lockdep_assert_irqs_disabled();
 
 	if (s->flags & __CMPXCHG_DOUBLE) {
 		ret = __update_freelist_fast(slab, freelist_old, counters_old,
 				            freelist_new, counters_new);
 	} else {
-		ret = __update_freelist_slow(slab, freelist_old, counters_old,
-				            freelist_new, counters_new);
-	}
-	if (likely(ret))
-		return true;
-
-	cpu_relax();
-	stat(s, CMPXCHG_DOUBLE_FAIL);
-
-#ifdef SLUB_DEBUG_CMPXCHG
-	pr_info("%s %s: cmpxchg double redo ", n, s->name);
-#endif
-
-	return false;
-}
-
-static inline bool slab_update_freelist(struct kmem_cache *s, struct slab *slab,
-		void *freelist_old, unsigned long counters_old,
-		void *freelist_new, unsigned long counters_new,
-		const char *n)
-{
-	bool ret;
-
-	if (s->flags & __CMPXCHG_DOUBLE) {
-		ret = __update_freelist_fast(slab, freelist_old, counters_old,
-				            freelist_new, counters_new);
-	} else {
 		unsigned long flags;
 
-		local_irq_save(flags);
+		if (irq_save)
+			local_irq_save(flags);
 		ret = __update_freelist_slow(slab, freelist_old, counters_old,
 				            freelist_new, counters_new);
-		local_irq_restore(flags);
+		if (irq_save)
+			local_irq_restore(flags);
 	}
 	if (likely(ret))
 		return true;
@@ -2250,10 +2226,10 @@  static inline void *acquire_slab(struct
 	VM_BUG_ON(new.frozen);
 	new.frozen = 1;
 
-	if (!__slab_update_freelist(s, slab,
-			freelist, counters,
-			new.freelist, new.counters,
-			"acquire_slab"))
+	if (!slab_update_freelist(s, slab,
+				  freelist, counters,
+				  new.freelist, new.counters,
+				  false, "acquire_slab"))
 		return NULL;
 
 	remove_partial(n, slab);
@@ -2577,9 +2553,9 @@  static void deactivate_slab(struct kmem_
 
 
 	if (!slab_update_freelist(s, slab,
-				old.freelist, old.counters,
-				new.freelist, new.counters,
-				"unfreezing slab")) {
+				  old.freelist, old.counters,
+				  new.freelist, new.counters,
+				  true, "unfreezing slab")) {
 		if (mode == M_PARTIAL)
 			spin_unlock_irqrestore(&n->list_lock, flags);
 		goto redo;
@@ -2633,10 +2609,10 @@  static void __unfreeze_partials(struct k
 
 			new.frozen = 0;
 
-		} while (!__slab_update_freelist(s, slab,
-				old.freelist, old.counters,
-				new.freelist, new.counters,
-				"unfreezing slab"));
+		} while (!slab_update_freelist(s, slab,
+					       old.freelist, old.counters,
+					       new.freelist, new.counters,
+					       false, "unfreezing slab"));
 
 		if (unlikely(!new.inuse && n->nr_partial >= s->min_partial)) {
 			slab->next = slab_to_discard;
@@ -3072,10 +3048,10 @@  static inline void *get_freelist(struct
 		new.inuse = slab->objects;
 		new.frozen = freelist != NULL;
 
-	} while (!__slab_update_freelist(s, slab,
-		freelist, counters,
-		NULL, new.counters,
-		"get_freelist"));
+	} while (!slab_update_freelist(s, slab,
+				       freelist, counters,
+				       NULL, new.counters,
+				       false, "get_freelist"));
 
 	return freelist;
 }
@@ -3666,9 +3642,9 @@  static void __slab_free(struct kmem_cach
 		}
 
 	} while (!slab_update_freelist(s, slab,
-		prior, counters,
-		head, new.counters,
-		"__slab_free"));
+				       prior, counters,
+				       head, new.counters,
+				       true, "__slab_free"));
 
 	if (likely(!n)) {