rt-slab: fix cpu inconsistency case

Signed-off-by Hiroshi Shimamoto <[email protected]>

---
 mm/slab.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

mm/slab.c +0 -0

index 4bf6ca8..7beb378 100644

Hi Hiroshi,

Thanks for looking into it. I'll take a deeper look at it later, but first I have a few nitpicks with the patch.

Quoted 10 lines

Signed-off-by Hiroshi Shimamoto <[email protected]>

---
 mm/slab.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

mm/slab.c +0 -0

index 4bf6ca8..7beb378 100644

mm/slab.c line 142

Quoted 7 lines

mm/slab.c +2 -2

@@ -138,8 +138,8 @@
  *
  * (On PREEMPT_RT, these are NOPs, but we have to drop/get the irq locks.)
  */
-# define slab_irq_disable_nort()	local_irq_disable()
-# define slab_irq_enable_nort()		local_irq_enable()
+# define slab_irq_disable_nort(cpu)	slab_irq_disable(cpu)
+# define slab_irq_enable_nort(cpu)	slab_irq_enable(cpu)

This is the !PREEMPT_RT version. It should stay as is (with the exception of ignoring the "cpu" variable. [ (void)cpu; ? ]

#define slab_irq_disable_nort(cpu) \
	do { \
		(void)cpu;  \
		local_irq_disable(); \
	} while(0)

mm/slab.c DEFINE_PER_CPU_LOCKED(int, slab_irq_locks) = { 0, }; · line 164

Quoted 12 lines

mm/slab.c +2 -2

 # define slab_irq_disable_rt(flags)	do { (void)(flags); } while (0)
 # define slab_irq_enable_rt(flags)	do { (void)(flags); } while (0)
 # define slab_spin_lock_irq(lock, cpu) \

@@ -160,8 +160,8 @@ DEFINE_PER_CPU_LOCKED(int, slab_irq_locks) = { 0, };
 	do { slab_irq_enable(cpu); (void) (flags); } while (0)
 # define slab_irq_disable_rt(cpu)	slab_irq_disable(cpu)
 # define slab_irq_enable_rt(cpu)	slab_irq_enable(cpu)
-# define slab_irq_disable_nort()	do { } while (0)
-# define slab_irq_enable_nort()		do { } while (0)
+# define slab_irq_disable_nort(cpu)	do { } while (0)
+# define slab_irq_enable_nort(cpu)	do { } while (0)

And these are the PREEMPT_RT version. So basically, this patch is a nop for PREEMPT_RT. I doubt it will solve your bug ;-)

And these are the PREEMPT_RT version. So basically, this patch is a nop
for PREEMPT_RT. I doubt it will solve your bug ;-)

mm/slab.c:cache_alloc_refill()

	if (unlikely(!ac->avail)) {
		int x;
		x = cache_grow(cachep, flags | GFP_THISNODE, cpu_to_node(*this_cpu), NULL, this_cpu);

		/* cache_grow can reenable interrupts, then ac could change. */
		ac = cpu_cache_get(cachep, *this_cpu);

The comment says, "ac could change", but it never if *this_cpu is same.

In cache_alloc_refill(), cpu_cache_get() should called with the valid cpu id after cache_grow(). Because on !PREEMPT_RT, the array_cache is protected by disabling irqs, so array_cache of other cpu shouldn't be accessed.

Quoted 16 lines

mm/slab.c DEFINE_PER_CPU_LOCKED(int, slab_irq_locks) = { 0, }; · line 164

Quoted 13 lines

 # define slab_irq_disable_rt(flags)	do { (void)(flags); } while (0)
 # define slab_irq_enable_rt(flags)	do { (void)(flags); } while (0)
 # define slab_spin_lock_irq(lock, cpu) \

mm/slab.c +2 -2

@@ -160,8 +160,8 @@ DEFINE_PER_CPU_LOCKED(int, slab_irq_locks) = { 0, };
 	do { slab_irq_enable(cpu); (void) (flags); } while (0)
 # define slab_irq_disable_rt(cpu)	slab_irq_disable(cpu)
 # define slab_irq_enable_rt(cpu)	slab_irq_enable(cpu)
-# define slab_irq_disable_nort()	do { } while (0)
-# define slab_irq_enable_nort()		do { } while (0)
+# define slab_irq_disable_nort(cpu)	do { } while (0)
+# define slab_irq_enable_nort(cpu)	do { } while (0)

And these are the PREEMPT_RT version. So basically, this patch is a nop for PREEMPT_RT. I doubt it will solve your bug ;-)

yes, it's nop. I'm sorry, I didn't show .config.
My kernel config is !PREEMPT_RT, it's same as the deadlock report. I guess it's a problem only !PREEMPT_RT.

Quoted 8 lines · Hiroshi Shimamoto

mm/slab.c:cache_alloc_refill()

	if (unlikely(!ac->avail)) {
		int x;
		x = cache_grow(cachep, flags | GFP_THISNODE, cpu_to_node(*this_cpu), NULL, this_cpu);

		/* cache_grow can reenable interrupts, then ac could change. */
		ac = cpu_cache_get(cachep, *this_cpu);

Quoted 5 lines

The comment says, "ac could change", but it never if *this_cpu is same.

In cache_alloc_refill(), cpu_cache_get() should called with the valid cpu id after cache_grow(). Because on !PREEMPT_RT, the array_cache is protected by disabling irqs, so array_cache of other cpu shouldn't be accessed.

Ah, sorry I missed the !PREEMPT_RT part.

Quoted 14 lines

Quoted 14 lines

mm/slab.c DEFINE_PER_CPU_LOCKED(int, slab_irq_locks) = { 0, }; · line 164

Quoted 12 lines

 # define slab_irq_disable_rt(flags)	do { (void)(flags); } while (0)
 # define slab_irq_enable_rt(flags)	do { (void)(flags); } while (0)
 # define slab_spin_lock_irq(lock, cpu) \

mm/slab.c +2 -2

@@ -160,8 +160,8 @@ DEFINE_PER_CPU_LOCKED(int, slab_irq_locks) = { 0, };
 	do { slab_irq_enable(cpu); (void) (flags); } while (0)
 # define slab_irq_disable_rt(cpu)	slab_irq_disable(cpu)
 # define slab_irq_enable_rt(cpu)	slab_irq_enable(cpu)
-# define slab_irq_disable_nort()	do { } while (0)
-# define slab_irq_enable_nort()		do { } while (0)
+# define slab_irq_disable_nort(cpu)	do { } while (0)
+# define slab_irq_enable_nort(cpu)	do { } while (0)

And these are the PREEMPT_RT version. So basically, this patch is a nop for PREEMPT_RT. I doubt it will solve your bug ;-)

Quoted 3 lines

yes, it's nop. I'm sorry, I didn't show .config.
My kernel config is !PREEMPT_RT, it's same as the deadlock report. I guess it's a problem only !PREEMPT_RT.

Hmm, I'll have to look deeper into this on Monday (I'm off today - Friday).

Is this a bug in mainline? The !PREEMPT_RT case should be as close to mainline as possible, with no actual changes in object code. If this is not the case, then we need to fix that.

Quoted 5 lines

Is this a bug in mainline? The !PREEMPT_RT case should be as close to mainline as possible, with no actual changes in object code. If this is not the case, then we need to fix that.

No, this issue is in -rt only.

Quoted 8 lines · Hiroshi Shimamoto

Quoted 5 lines

Is this a bug in mainline? The !PREEMPT_RT case should be as close to mainline as possible, with no actual changes in object code. If this is not the case, then we need to fix that.

No, this issue is in -rt only.

Hi Hiroshi,

Thanks again for reporting this. I took a deeper look into this today and came to the conclusion that we need to get rid of both slab_irq_disable_nort and slab_irq_disable_rt, and simply use slab_irq_disable ;-)

This is what you patch indirectly does. I'll write up another patch to fix this.

Quoted 5 lines · Hiroshi Shimamoto

Quoted 3 lines

Is this a bug in mainline? The !PREEMPT_RT case should be as close to mainline as possible, with no actual changes in object code. If this is not the case, then we need to fix that.

No, this issue is in -rt only.

Hi Hiroshi,

Thanks again for reporting this. I took a deeper look into this today and came to the conclusion that we need to get rid of both slab_irq_disable_nort and slab_irq_disable_rt, and simply use slab_irq_disable ;-)

This is what you patch indirectly does. I'll write up another patch to fix this.

Thanks,

-- Steve

P.S. I'm hoping to get -rt4 out today.

Quoted 8 lines · Steven Rostedt

Quoted 5 lines · Hiroshi Shimamoto

Quoted 3 lines

Is this a bug in mainline? The !PREEMPT_RT case should be as close to mainline as possible, with no actual changes in object code. If this is not the case, then we need to fix that.

No, this issue is in -rt only.

Hi Hiroshi,

Quoted 8 lines

Thanks again for reporting this. I took a deeper look into this today and came to the conclusion that we need to get rid of both slab_irq_disable_nort and slab_irq_disable_rt, and simply use slab_irq_disable ;-)

This is what you patch indirectly does. I'll write up another patch to fix this.

Actually, I'm going to take your patch.

Seems that the reason Ingo did the two separate, is that the functions called also call the slab_irq_save, which will retake the locks in PREEMPT_RT. If we don't release the lock, then we can deadlock.