| Message ID | 20220825015722.1697209-1-42.hyeyoo@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/slub: fix comments about fastpath limitation on PREEMPT_RT | expand |
On 8/25/22 03:57, Hyeonggon Yoo wrote: > With PREEMPT_RT disabling interrupt is unnecessary as there is > no user of slab in hardirq context on PREEMPT_RT. > > The limitation of lockless fastpath on PREEMPT_RT comes from the fact > that local_lock does not disable preemption on PREEMPT_RT. > > Fix comments accordingly. > > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> Just FTR, as "slub: Make PREEMPT_RT support less convoluted" patch dealt with these comments already, there's now nothing left to apply from below. > --- > mm/slub.c | 23 ++++++++++++++--------- > 1 file changed, 14 insertions(+), 9 deletions(-) > > diff --git a/mm/slub.c b/mm/slub.c > index 30c2ee9e8a29..aa42ac6013b8 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -100,7 +100,7 @@ > * 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 > + * On PREEMPT_RT, the local lock does not actually disable preemption (and thus > * prevent the lockless operations), so fastpath operations also need to take > * the lock and are no longer lockless. > * > @@ -3185,10 +3185,12 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s, struct list_l > 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. > + * slowpath has taken the local_lock which does not disable preemption > + * on PREEMPT_RT, it is not protected against a fast path operation in > + * another thread that does not take the local_lock. > + * > + * 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) || > unlikely(!object || !slab || !node_match(slab, node))) { > @@ -3457,10 +3459,13 @@ static __always_inline void do_slab_free(struct kmem_cache *s, > #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. > + * a slowpath has taken the local_lock which does not disable > + * preemption on PREEMPT_RT, it is not protected against a > + * fast path operation in another thread that does not take > + * the local_lock. > + * > + * 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; >
diff --git a/mm/slub.c b/mm/slub.c index 30c2ee9e8a29..aa42ac6013b8 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -100,7 +100,7 @@ * 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 + * On PREEMPT_RT, the local lock does not actually disable preemption (and thus * prevent the lockless operations), so fastpath operations also need to take * the lock and are no longer lockless. * @@ -3185,10 +3185,12 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s, struct list_l 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. + * slowpath has taken the local_lock which does not disable preemption + * on PREEMPT_RT, it is not protected against a fast path operation in + * another thread that does not take the local_lock. + * + * 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) || unlikely(!object || !slab || !node_match(slab, node))) { @@ -3457,10 +3459,13 @@ static __always_inline void do_slab_free(struct kmem_cache *s, #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. + * a slowpath has taken the local_lock which does not disable + * preemption on PREEMPT_RT, it is not protected against a + * fast path operation in another thread that does not take + * the local_lock. + * + * 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;
With PREEMPT_RT disabling interrupt is unnecessary as there is no user of slab in hardirq context on PREEMPT_RT. The limitation of lockless fastpath on PREEMPT_RT comes from the fact that local_lock does not disable preemption on PREEMPT_RT. Fix comments accordingly. Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> --- mm/slub.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)