Re: [PATCH v2 5/5] mm/slub: simplify __cmpxchg_double_slab() and slab_[un]lock()

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2022-08-23 19:04:00 [+0200], Vlastimil Babka wrote:
> The PREEMPT_RT specific disabling of irqs in __cmpxchg_double_slab()
> (through slab_[un]lock()) is unnecessary as bit_spin_lock() disables
> preemption and that's sufficient on RT where interrupts are threaded.

maybe something like 
"… sufficient on PREEMPT_RT where no allocation/ free operation is
 performed in hardirq context and so interrupt the current operation."

> That means we no longer need the slab_[un]lock() wrappers, so delete
> them and rename the current __slab_[un]lock() to slab_[un]lock().
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Acked-by: David Rientjes <rientjes@google.com>

Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -454,7 +454,7 @@ static __always_inline void __slab_lock(struct slab *slab)
…
>  /*
>   * Interrupts must be disabled (for the fallback code to work right), typically
> - * by an _irqsave() lock variant. Except on PREEMPT_RT where locks are different
> - * so we disable interrupts as part of slab_[un]lock().
> + * by an _irqsave() lock variant. Except on PREEMPT_RT where these variants do
> + * not actually disable interrupts. On the other hand the migrate_disable()
> + * done by bit_spin_lock() is sufficient on PREEMPT_RT thanks to its threaded
> + * interrupts.

 "                                     On PREEMPT_RT the
 preempt_disable(), which is part of bit_spin_lock(), is sufficient
 because the policy is not to allow any allocation/ free operation in
 hardirq context. Therefore nothing can interrupt the operation."

This also includes SMP function calls (IPI).

>   */
>  static inline bool __cmpxchg_double_slab(struct kmem_cache *s, struct slab *slab,
>  		void *freelist_old, unsigned long counters_old,

Sebastian