| Message ID | 20220817162703.728679-2-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/9] slub: Make PREEMPT_RT support less convoluted | expand |
On Wed, 17 Aug 2022, Sebastian Andrzej Siewior wrote: > + * On PREEMPT_RT, the local lock neither disables interrupts nor preemption > + * which means the lockless fastpath cannot be used as it might interfere with > + * an in-progress slow path operations. In this case the local lock is always > + * taken but it still utilizes the freelist for the common operations. The slub fastpath does not interfere with slow path operations and the fastpath does not require disabling preemption or interrupts if the processor supports local rmv operations. So you should be able to use the fastpath on PREEMPT_RT. If the fastpath is not possible then you need to disable preemption and eventually take locks etc and then things may get a bit more complicated.
On 8/18/22 11:42, Christoph Lameter wrote: > On Wed, 17 Aug 2022, Sebastian Andrzej Siewior wrote: > >> + * On PREEMPT_RT, the local lock neither disables interrupts nor preemption >> + * which means the lockless fastpath cannot be used as it might interfere with >> + * an in-progress slow path operations. In this case the local lock is always >> + * taken but it still utilizes the freelist for the common operations. > > The slub fastpath does not interfere with slow path operations and the That's true on !PREEMPT_RT because a slowpath operation under local_lock_irqsave() will disable interrupts, so there can't be a fastpath operation in an interrupt handler appearing in the middle of a slowpath operation. On PREEMPT_RT local_lock_irqsave() doesn't actually disable interrupts, so that can happen. IIRC we learned that the hard way when Mike Galbraith was testing early versions of my PREEMPT_RT changes for SLUB. > fastpath does not require disabling preemption or interrupts if the > processor supports local rmv operations. So you should be able to use the > fastpath on PREEMPT_RT. > > If the fastpath is not possible then you need to disable preemption and > eventually take locks etc and then things may get a bit more complicated. Yeah that's basically the solution for PREEMPT_RT, take the local_lock.
On 2022-08-18 16:37:06 [+0200], Vlastimil Babka wrote: > > The slub fastpath does not interfere with slow path operations and the > > That's true on !PREEMPT_RT because a slowpath operation under > local_lock_irqsave() will disable interrupts, so there can't be a > fastpath operation in an interrupt handler appearing in the middle of a > slowpath operation. > > On PREEMPT_RT local_lock_irqsave() doesn't actually disable interrupts, > so that can happen. IIRC we learned that the hard way when Mike > Galbraith was testing early versions of my PREEMPT_RT changes for SLUB. I think the point is that local_lock_irqsave() does not disable preemption. So the lock owner (within the local_lock_irqsave() section) can be preempted and another task can use the fast path which does not involve the lock and things go boom from here. Sebastian
On Wed, Aug 17, 2022 at 9:27 AM Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote: > > The slub code already has a few helpers depending on PREEMPT_RT. Add a few > more and get rid of the CONFIG_PREEMPT_RT conditionals all over the place. > > No functional change. Looks like a fine cleanup, but I'd prefer that #define use_lockless_fast_path() {(true)/(false)} to look much less like it's some function call. The first read-through it looked like some very dynamic thing to me. Just doing #define USE_LOCKLESS_FAST_PATH .. would make it much more visually obvious that it's not some kind of complex run-time decision. Linus
On Thu, 18 Aug 2022, Vlastimil Babka wrote: > On 8/18/22 11:42, Christoph Lameter wrote: > > On Wed, 17 Aug 2022, Sebastian Andrzej Siewior wrote: > > > >> + * On PREEMPT_RT, the local lock neither disables interrupts nor preemption > >> + * which means the lockless fastpath cannot be used as it might interfere with > >> + * an in-progress slow path operations. In this case the local lock is always > >> + * taken but it still utilizes the freelist for the common operations. > > > > The slub fastpath does not interfere with slow path operations and the > > That's true on !PREEMPT_RT because a slowpath operation under > local_lock_irqsave() will disable interrupts, so there can't be a > fastpath operation in an interrupt handler appearing in the middle of a > slowpath operation. > > On PREEMPT_RT local_lock_irqsave() doesn't actually disable interrupts, > so that can happen. IIRC we learned that the hard way when Mike > Galbraith was testing early versions of my PREEMPT_RT changes for SLUB. Well yes if you enable interrupts during the slowpath then interrupts may use the fastpath. That is a basic design change to the way concurrency is handled in the allocators. There needs to be some fix here to restore the exclusion of the fastpath during slow path processing. This could be A) Exclude the fastpath during slowpath operations This can be accomplished by setting things up like in the debug mode that also excludes the fastpath. or B) Force interrupt allocations to the slowpath. Check some flag that indicates an interrupt allocation is occurring and then bypass the fastpath.
On 8/17/22 18:26, Sebastian Andrzej Siewior wrote: > From: Thomas Gleixner <tglx@linutronix.de> > > The slub code already has a few helpers depending on PREEMPT_RT. Add a few > more and get rid of the CONFIG_PREEMPT_RT conditionals all over the place. > > No functional change. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Christoph Lameter <cl@linux.com> > Cc: David Rientjes <rientjes@google.com> > Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com> > Cc: Pekka Enberg <penberg@kernel.org> > Cc: Vlastimil Babka <vbabka@suse.cz> > Cc: linux-mm@kvack.org > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > mm/slub.c | 66 +++++++++++++++++++++++++------------------------------ > 1 file changed, 30 insertions(+), 36 deletions(-) > > diff --git a/mm/slub.c b/mm/slub.c > index 862dbd9af4f52..5f7c5b5bd49f9 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -100,9 +100,11 @@ > * except the stat counters. This is a percpu structure manipulated only by > * the local cpu, so the lock protects against being preempted or interrupted > * by an irq. Fast path operations rely on lockless operations instead. > - * On PREEMPT_RT, the local lock does not actually disable irqs (and thus > - * prevent the lockless operations), so fastpath operations also need to take > - * the lock and are no longer lockless. > + * > + * On PREEMPT_RT, the local lock neither disables interrupts nor preemption > + * which means the lockless fastpath cannot be used as it might interfere with > + * an in-progress slow path operations. In this case the local lock is always > + * taken but it still utilizes the freelist for the common operations. > * > * lockless fastpaths > * > @@ -163,8 +165,11 @@ > * function call even on !PREEMPT_RT, use inline preempt_disable() there. > */ > #ifndef CONFIG_PREEMPT_RT > -#define slub_get_cpu_ptr(var) get_cpu_ptr(var) > -#define slub_put_cpu_ptr(var) put_cpu_ptr(var) > +#define slub_get_cpu_ptr(var) get_cpu_ptr(var) > +#define slub_put_cpu_ptr(var) put_cpu_ptr(var) > +#define use_lockless_fast_path() (true) > +#define slub_local_irq_save(flags) local_irq_save(flags) > +#define slub_local_irq_restore(flags) local_irq_restore(flags) Note these won't be neccessary anymore after https://lore.kernel.org/linux-mm/20220823170400.26546-6-vbabka@suse.cz/T/#u > #else > #define slub_get_cpu_ptr(var) \ > ({ \ > @@ -176,6 +181,9 @@ do { \ > (void)(var); \ > migrate_enable(); \ > } while (0) > +#define use_lockless_fast_path() (false) > +#define slub_local_irq_save(flags) do { } while (0) > +#define slub_local_irq_restore(flags) do { } while (0) > #endif > > #ifdef CONFIG_SLUB_DEBUG > @@ -460,16 +468,14 @@ static __always_inline void __slab_unlock(struct slab *slab) > > static __always_inline void slab_lock(struct slab *slab, unsigned long *flags) > { > - if (IS_ENABLED(CONFIG_PREEMPT_RT)) > - local_irq_save(*flags); > + slub_local_irq_save(*flags); > __slab_lock(slab); > } > > static __always_inline void slab_unlock(struct slab *slab, unsigned long *flags) > { > __slab_unlock(slab); > - if (IS_ENABLED(CONFIG_PREEMPT_RT)) > - local_irq_restore(*flags); > + slub_local_irq_restore(*flags); > } Ditto. > /* > @@ -482,7 +488,7 @@ static inline bool __cmpxchg_double_slab(struct kmem_cache *s, struct slab *slab > void *freelist_new, unsigned long counters_new, > const char *n) > { > - if (!IS_ENABLED(CONFIG_PREEMPT_RT)) > + if (use_lockless_fast_path()) > lockdep_assert_irqs_disabled(); This test would stay after the patch I referenced above. But while this change will keep testing the technically correct thing, the name would be IMHO misleading here, as this is semantically not about the lockless fast path, but whether we need to have irqs disabled to avoid a deadlock due to irq incoming when we hold the bit_spin_lock() and its handler trying to acquire it as well.
On 2022-08-23 19:15:43 [+0200], Vlastimil Babka wrote: > > +#define slub_local_irq_save(flags) local_irq_save(flags) > > +#define slub_local_irq_restore(flags) local_irq_restore(flags) > > Note these won't be neccessary anymore after > https://lore.kernel.org/linux-mm/20220823170400.26546-6-vbabka@suse.cz/T/#u Okay, let me postpone that one and rebase what is left on top. > > @@ -482,7 +488,7 @@ static inline bool __cmpxchg_double_slab(struct kmem_cache *s, struct slab *slab > > void *freelist_new, unsigned long counters_new, > > const char *n) > > { > > - if (!IS_ENABLED(CONFIG_PREEMPT_RT)) > > + if (use_lockless_fast_path()) > > lockdep_assert_irqs_disabled(); > > This test would stay after the patch I referenced above. But while this > change will keep testing the technically correct thing, the name would be > IMHO misleading here, as this is semantically not about the lockless fast > path, but whether we need to have irqs disabled to avoid a deadlock due to > irq incoming when we hold the bit_spin_lock() and its handler trying to > acquire it as well. Color me confused. Memory is never allocated in-IRQ context on PREEMPT_RT. Therefore I don't understand why interrupts must be disabled for the fast path (unless that comment only applied to !RT). It could be about preemption since spinlock, local_lock don't disable preemption and so another allocation on the same CPU is possible. But then you say "we hold the bit_spin_lock()" and this one disables preemption. This means nothing can stop the bit_spin_lock() owner from making progress and since there is no memory allocation in-IRQ, we can't block on the same bit_spin_lock() on the same CPU. Sebastian
On 8/24/22 15:25, Sebastian Andrzej Siewior wrote: > On 2022-08-23 19:15:43 [+0200], Vlastimil Babka wrote: >>> +#define slub_local_irq_save(flags) local_irq_save(flags) >>> +#define slub_local_irq_restore(flags) local_irq_restore(flags) >> >> Note these won't be neccessary anymore after >> https://lore.kernel.org/linux-mm/20220823170400.26546-6-vbabka@suse.cz/T/#u > > Okay, let me postpone that one and rebase what is left on top. > >>> @@ -482,7 +488,7 @@ static inline bool __cmpxchg_double_slab(struct kmem_cache *s, struct slab *slab >>> void *freelist_new, unsigned long counters_new, >>> const char *n) >>> { >>> - if (!IS_ENABLED(CONFIG_PREEMPT_RT)) >>> + if (use_lockless_fast_path()) >>> lockdep_assert_irqs_disabled(); >> >> This test would stay after the patch I referenced above. But while this >> change will keep testing the technically correct thing, the name would be >> IMHO misleading here, as this is semantically not about the lockless fast >> path, but whether we need to have irqs disabled to avoid a deadlock due to >> irq incoming when we hold the bit_spin_lock() and its handler trying to >> acquire it as well. > > Color me confused. Memory is never allocated in-IRQ context on > PREEMPT_RT. Therefore I don't understand why interrupts must be disabled > for the fast path (unless that comment only applied to !RT). Yes that only applied to !RT. Hence why the assert is there only for !RT. > It could be about preemption since spinlock, local_lock don't disable > preemption and so another allocation on the same CPU is possible. But > then you say "we hold the bit_spin_lock()" and this one disables > preemption. This means nothing can stop the bit_spin_lock() owner from > making progress and since there is no memory allocation in-IRQ, we can't > block on the same bit_spin_lock() on the same CPU. Yeah, realizing that this is true on RT led to the recent patch I referenced. Initially when converting SLUB to RT last year I didn't realize this detail, and instead replaced the irq disabling done (only on !RT) by e.g. local_lock_irqsave with the manual local_irq_save(). > Sebastian
On 2022-08-24 15:54:54 [+0200], Vlastimil Babka wrote: > Yeah, realizing that this is true on RT led to the recent patch I > referenced. Initially when converting SLUB to RT last year I didn't realize > this detail, and instead replaced the irq disabling done (only on !RT) by > e.g. local_lock_irqsave with the manual local_irq_save(). Ah, okay. Let me get to the other series then ;) Sebastian
On Fri, Aug 19, 2022 at 05:04:31PM +0200, Christoph Lameter wrote: > On Thu, 18 Aug 2022, Vlastimil Babka wrote: > > > On 8/18/22 11:42, Christoph Lameter wrote: > > > On Wed, 17 Aug 2022, Sebastian Andrzej Siewior wrote: > > > > > >> + * On PREEMPT_RT, the local lock neither disables interrupts nor preemption > > >> + * which means the lockless fastpath cannot be used as it might interfere with > > >> + * an in-progress slow path operations. In this case the local lock is always > > >> + * taken but it still utilizes the freelist for the common operations. > > > > > > The slub fastpath does not interfere with slow path operations and the > > > > That's true on !PREEMPT_RT because a slowpath operation under > > local_lock_irqsave() will disable interrupts, so there can't be a > > fastpath operation in an interrupt handler appearing in the middle of a > > slowpath operation. > > > > On PREEMPT_RT local_lock_irqsave() doesn't actually disable interrupts, > > so that can happen. IIRC we learned that the hard way when Mike > > Galbraith was testing early versions of my PREEMPT_RT changes for SLUB. > > Well yes if you enable interrupts during the slowpath then interrupts may > use the fastpath. That is a basic design change to the way concurrency is > handled in the allocators. > > There needs to be some fix here to restore the exclusion of the fastpath > during slow path processing. This could be > > A) Exclude the fastpath during slowpath operations > > This can be accomplished by setting things up like in the debug mode > that also excludes the fastpath. I think we can do that by disabling preemption (for a short period, I think) in slowpath on RT (like disabling irq in non-RT) But I wonder if RT guys will prefer that? > B) Force interrupt allocations to the slowpath. > > Check some flag that indicates an interrupt allocation is occurring and > then bypass the fastpath. There is nothing special about interrupt allocation on RT. All users of SLUB on RT must not be in hardirq context. So I don't think it is possible to distingush between a thread being preempted and another thread that preempts it.
diff --git a/mm/slub.c b/mm/slub.c index 862dbd9af4f52..5f7c5b5bd49f9 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -100,9 +100,11 @@ * except the stat counters. This is a percpu structure manipulated only by * the local cpu, so the lock protects against being preempted or interrupted * by an irq. Fast path operations rely on lockless operations instead. - * On PREEMPT_RT, the local lock does not actually disable irqs (and thus - * prevent the lockless operations), so fastpath operations also need to take - * the lock and are no longer lockless. + * + * On PREEMPT_RT, the local lock neither disables interrupts nor preemption + * which means the lockless fastpath cannot be used as it might interfere with + * an in-progress slow path operations. In this case the local lock is always + * taken but it still utilizes the freelist for the common operations. * * lockless fastpaths * @@ -163,8 +165,11 @@ * function call even on !PREEMPT_RT, use inline preempt_disable() there. */ #ifndef CONFIG_PREEMPT_RT -#define slub_get_cpu_ptr(var) get_cpu_ptr(var) -#define slub_put_cpu_ptr(var) put_cpu_ptr(var) +#define slub_get_cpu_ptr(var) get_cpu_ptr(var) +#define slub_put_cpu_ptr(var) put_cpu_ptr(var) +#define use_lockless_fast_path() (true) +#define slub_local_irq_save(flags) local_irq_save(flags) +#define slub_local_irq_restore(flags) local_irq_restore(flags) #else #define slub_get_cpu_ptr(var) \ ({ \ @@ -176,6 +181,9 @@ do { \ (void)(var); \ migrate_enable(); \ } while (0) +#define use_lockless_fast_path() (false) +#define slub_local_irq_save(flags) do { } while (0) +#define slub_local_irq_restore(flags) do { } while (0) #endif #ifdef CONFIG_SLUB_DEBUG @@ -460,16 +468,14 @@ static __always_inline void __slab_unlock(struct slab *slab) static __always_inline void slab_lock(struct slab *slab, unsigned long *flags) { - if (IS_ENABLED(CONFIG_PREEMPT_RT)) - local_irq_save(*flags); + slub_local_irq_save(*flags); __slab_lock(slab); } static __always_inline void slab_unlock(struct slab *slab, unsigned long *flags) { __slab_unlock(slab); - if (IS_ENABLED(CONFIG_PREEMPT_RT)) - local_irq_restore(*flags); + slub_local_irq_restore(*flags); } /* @@ -482,7 +488,7 @@ static inline bool __cmpxchg_double_slab(struct kmem_cache *s, struct slab *slab void *freelist_new, unsigned long counters_new, const char *n) { - if (!IS_ENABLED(CONFIG_PREEMPT_RT)) + if (use_lockless_fast_path()) lockdep_assert_irqs_disabled(); #if defined(CONFIG_HAVE_CMPXCHG_DOUBLE) && \ defined(CONFIG_HAVE_ALIGNED_STRUCT_PAGE) @@ -3197,14 +3203,8 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s, struct list_l object = c->freelist; slab = c->slab; - /* - * We cannot use the lockless fastpath on PREEMPT_RT because if a - * slowpath has taken the local_lock_irqsave(), it is not protected - * against a fast path operation in an irq handler. So we need to take - * the slow path which uses local_lock. It is still relatively fast if - * there is a suitable cpu freelist. - */ - if (IS_ENABLED(CONFIG_PREEMPT_RT) || + + if (!use_lockless_fast_path() || unlikely(!object || !slab || !node_match(slab, node))) { object = __slab_alloc(s, gfpflags, node, addr, c); } else { @@ -3463,6 +3463,7 @@ static __always_inline void do_slab_free(struct kmem_cache *s, void *tail_obj = tail ? : head; struct kmem_cache_cpu *c; unsigned long tid; + void **freelist; redo: /* @@ -3477,9 +3478,13 @@ static __always_inline void do_slab_free(struct kmem_cache *s, /* Same with comment on barrier() in slab_alloc_node() */ barrier(); - if (likely(slab == c->slab)) { -#ifndef CONFIG_PREEMPT_RT - void **freelist = READ_ONCE(c->freelist); + if (unlikely(slab != c->slab)) { + __slab_free(s, slab, head, tail_obj, cnt, addr); + return; + } + + if (use_lockless_fast_path()) { + freelist = READ_ONCE(c->freelist); set_freepointer(s, tail_obj, freelist); @@ -3491,16 +3496,8 @@ static __always_inline void do_slab_free(struct kmem_cache *s, note_cmpxchg_failure("slab_free", s, tid); goto redo; } -#else /* CONFIG_PREEMPT_RT */ - /* - * We cannot use the lockless fastpath on PREEMPT_RT because if - * a slowpath has taken the local_lock_irqsave(), it is not - * protected against a fast path operation in an irq handler. So - * we need to take the local_lock. We shouldn't simply defer to - * __slab_free() as that wouldn't use the cpu freelist at all. - */ - void **freelist; - + } else { + /* Update the free list under the local lock */ local_lock(&s->cpu_slab->lock); c = this_cpu_ptr(s->cpu_slab); if (unlikely(slab != c->slab)) { @@ -3515,11 +3512,8 @@ static __always_inline void do_slab_free(struct kmem_cache *s, c->tid = next_tid(tid); local_unlock(&s->cpu_slab->lock); -#endif - stat(s, FREE_FASTPATH); - } else - __slab_free(s, slab, head, tail_obj, cnt, addr); - + } + stat(s, FREE_FASTPATH); } static __always_inline void slab_free(struct kmem_cache *s, struct slab *slab,