[PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Michael Schmitz]

Comparing m68k with mips, we are missing the tests for
disabled interrupts present in the low level kernel
preemption code for mips.

Adding that test results in no more 'table already freed'
messages when running stress-ng --zombie (on ARAnyM).

Needs testing on qemu / Mac as this test result may be
Atari specific due to the hsync interrupt disable scheme
there.

Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
Link: https://lore.kernel.org/all/7858a184cda66e0991fd295c711dfed7e4d1248c.1696603287.git.geert@linux-m68k.org/
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Finn Thain <fthain@linux-m68k.org>
---
 arch/m68k/kernel/entry.S | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/m68k/kernel/entry.S b/arch/m68k/kernel/entry.S
index 9933679ea28b..f2cdea3cbc93 100644
--- a/arch/m68k/kernel/entry.S
+++ b/arch/m68k/kernel/entry.S
@@ -249,6 +249,10 @@ ENTRY(ret_from_exception)
 	jpl	1f
 
 	movel	%a0@(TINFO_PREEMPT),%d1
+	jne	1f
+
+					| check if interrupts disabled
+	bfextu	%sp@(PT_OFF_SR){#0,#3},%d1
 	jne	1f
 
 	pea	1f
-- 
2.17.1

Re: [PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Michael Schmitz]

Hi Finn,

Am 23.03.2024 um 17:35 schrieb Finn Thain:
> On Fri, 22 Mar 2024, Michael Schmitz wrote:
>
>> Am 22.03.2024 um 17:53 schrieb Michael Schmitz:
>>> Am 22.03.2024 um 17:39 schrieb Finn Thain:
>>>
>>>> I find that patch description to be a bit confusing, since
>>>> preempt_schedule_irq() requires that interrupts were disabled:
>>>>
>>>>         BUG_ON(preempt_count() || !irqs_disabled());
>>>>
>>>> Looking more closely I see that you're testing the IPL bits from the
>>>> stack not the status register...
>>>
>>> Yes - the problem appears to be that if we enter preemption when about
>>> to return to kernel code that had interrupts disabled, bad things may
>>> happen [...]
>>> That's independent from having interrupts disabled in the currently
>>> active exception.
>>>
>
> Is it? If interrupts were disabled by the code we're returning to, surely
> they must be disabled still (in the Status reg, for the BUG_ON above).

They would still be disabled, but by calling preempt_schedule_irq() they 
will be reenabled before calling schedule(). At that time, another 
interrupt may come in, and change things from under the piece of code 
that had interrupts disabled.

That won't happen without preemption.

What's more, I suspect schedule() may cause another process to exit that 
would otherwise first take a signal, and signal delivery then repeats 
the vma teardown on process exit. At least that is how I read those 
'table already freed' stack traces.

>
>>>>
>>>> It's not clear to me which pre-emption opportunities remain in
>>>> effect. E.g. is full preemption (in effect) the same as voluntary
>>>> preemption?
>>>
>>> Good question - I'll have to instrument preempt_schedule_irq() and see
>>> if it gets called at all with my patch.
>>
>> Can't see preempt_schedule_irq() entered with my patch ...
>>
>
> Neither can I: A quick "stress-ng --zombie -1 -t 100" test passed without
> ever calling preempt_schedule_irq() in QEMU.

Right. Whenever I allow preemption with IPL (in the saved frame's SR) > 
0, I eventually get the 'table already free' panic. Trying to save the 
whole frame for later printing in free_pointer_table() hasn't been too 
helpful though. The saved frame's PC does not appear anywhere in the trace.

>
>> In general, full preemption is not the same as voluntary preemption.
>> Full preemption attempts to preempt on every return from interrupt or
>> syscall (given certain constraints are fulfilled such as we're not
>> currently in any interrupt or already preempting, and there's other work
>> that needs scheduling).
>>
>> The constraint that interrupts must not be disabled in the saved stack
>> frame apparently is one constraint too many...
>>
>
> I dropped the mailing list from the recipients in my previous message.
> This might a good time to follow up on the list in case Geert can shed
> some light.

Adding Geert and list back in now ...

Cheers,

	Michael


>

Re: [PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Michael Schmitz]

Hi Finn,

> What's more, I suspect schedule() may cause another process to exit that
> would otherwise first take a signal, and signal delivery then repeats
> the vma teardown on process exit. At least that is how I read those
> 'table already freed' stack traces.

In the backtrace below, I do actually see free_pointer_table() reentered 
after preemption. (Ignore the 2200, 2200, 6480 in the call trace - 
that's just the arguments to a pr_err() that prints the first line in 
the excerpt below.)

To get this snapshot of what exception context preempt_schedule_irq() 
was last called from, I save the stack pointer, the status register in 
the exception stack frame, and the vector number before calling 
preempt_schedule_irq(). The saved values are printed before calling 
panic() in free_pointer_table().

Unfortunately, when I add preempt_disable()/ 
sched_preempt_enable_noresched() in free_pointer_table(), I don't get 
that particular panic() anymore, but the bad kernel buserror with the 
warning from set_fc() that you reported. So no real improvement ...

Cheers,

	Michael

saved from last preempt: nr 2d sr 2200 2200 pc 6480 sp dcddc4
Kernel panic - not syncing: table already free!
CPU: 0 PID: 1629 Comm: udevd Not tainted 
6.8.0-rc7-atari-fpuemu-spinlocktest+ #1759
Stack from 00f7fd9c:
         00f7fd9c 003e75ee 003e75ee 00000000 00000001 003354cc 003e75ee 
0032f208
         00000001 00000001 00e14000 004f0000 00f8b40c 0000643c 00f7ff0e 
000064d4
         003d9128 003d90ee 0000002d 00002200 00002200 00006480 00dcddc4 
c0000000
         c017c000 efd00000 c00c0000 01e14800 ee000000 00f8b184 000cdee8 
00e14800
         00000001 ffffffff c0000000 000d367c 0031d67e 000d9916 000d367c 
00dc00cc
         000cdd12 0031d67e 000d9916 c0080000 00f8b180 c017bfff c017bfff 
edffffff
Call Trace: [<003354cc>] dump_stack+0xc/0x10
  [<0032f208>] panic+0xd8/0x258
  [<0000643c>] free_pointer_table+0x0/0x160
  [<000064d4>] free_pointer_table+0x98/0x160
  [<00002200>] do_one_initcall+0xa4/0x188
  [<00002200>] do_one_initcall+0xa4/0x188
  [<00006480>] free_pointer_table+0x44/0x160
  [<000cdee8>] free_pgd_range+0x1d6/0x1f2
  [<000d367c>] unlink_file_vma+0x0/0x40
  [<0031d67e>] mas_find+0x0/0x102
  [<000d9916>] unlink_anon_vmas+0x0/0x106
  [<000d367c>] unlink_file_vma+0x0/0x40
  [<000cdd12>] free_pgd_range+0x0/0x1f2
  [<0031d67e>] mas_find+0x0/0x102
  [<000d9916>] unlink_anon_vmas+0x0/0x106
  [<000cdfbe>] free_pgtables+0xba/0xcc
  [<0031d67e>] mas_find+0x0/0x102
  [<000d2950>] mas_set+0x0/0x22
  [<00039f02>] up_read+0x0/0x64
  [<000d41d0>] exit_mmap+0x13c/0x1de
  [<0000efd3>] NODD+0xab/0x104
  [<001900f9>] ext2_get_folio.constprop.11+0x1a7/0x274
  [<0000ffff>] EXPTBL+0x3bf/0x400
  [<00016782>] __mmput+0x16/0x94
  [<0001a158>] do_exit+0x250/0x714
  [<00339f12>] _raw_spin_unlock_irq+0x10/0x38
  [<0001a79a>] sys_exit_group+0x0/0x14
  [<0001a7ae>] pid_child_should_wake+0x0/0x52
  [<0000269a>] syscall+0x8/0xc
  [<0008c00b>] do_check_common+0x325/0x2946

Re: [PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Finn Thain]

On Mon, 25 Mar 2024, Michael Schmitz wrote:

> Unfortunately, when I add preempt_disable()/ 
> sched_preempt_enable_noresched() in free_pointer_table(), I don't get 
> that particular panic() anymore,

I'd like to try that patch. Would you send it please?

> but the bad kernel buserror with the warning from set_fc() that you 
> reported...

I think those set_fc() calls are confined to 68040 fault handling. Maybe 
we should try running stress tests on a 68030?

Re: [PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Michael Schmitz]

Hi Finn,

see below (hopefully not whitespace-damaged).

The set_fc warning is specific to 040 but the bad kernel buserror (null 
pointer) causing it isn't. But testing on 030 would be a good idea if 
you have one with enough RAM. Haven't tried to boot my Falcon with a 
preempt enabled kernel yet.

Cheers,

     Michael


diff --git a/arch/m68k/mm/motorola.c b/arch/m68k/mm/motorola.c
index 4d4d390a2df2..96cdcacb94ea 100644
--- a/arch/m68k/mm/motorola.c
+++ b/arch/m68k/mm/motorola.c
@@ -15,6 +15,7 @@
  #include <linux/mm.h>
  #include <linux/swap.h>
  #include <linux/kernel.h>
+#include <linux/preempt.h>
  #include <linux/string.h>
  #include <linux/types.h>
  #include <linux/init.h>
@@ -195,6 +196,8 @@ int free_pointer_table(void *table, int type)
         unsigned long page = ptable & PAGE_MASK;
         unsigned int mask = 1U << ((ptable - page)/ptable_size(type));

+       preempt_disable();
+
         dp = PD_PTABLE(page);
         if (PD_MARKBITS (dp) & mask) {
                 pr_err("saved from last preempt: nr %x sr %lx %x pc %lx 
sp %lx\n",
@@ -213,6 +216,7 @@ int free_pointer_table(void *table, int type)
                 if (type == TABLE_PTE)
                         pagetable_pte_dtor(virt_to_ptdesc((void *)page));
                 free_page (page);
+               sched_preempt_enable_no_resched();
                 return 1;
         } else if (ptable_list[type].next != dp) {
                 /*
@@ -221,6 +225,7 @@ int free_pointer_table(void *table, int type)
                  */
                 list_move(dp, &ptable_list[type]);
         }
+       sched_preempt_enable_no_resched();
         return 0;
  }


On 26/03/24 11:28, Finn Thain wrote:
> On Mon, 25 Mar 2024, Michael Schmitz wrote:
>
>> Unfortunately, when I add preempt_disable()/
>> sched_preempt_enable_noresched() in free_pointer_table(), I don't get
>> that particular panic() anymore,
> I'd like to try that patch. Would you send it please?
>
>> but the bad kernel buserror with the warning from set_fc() that you
>> reported...
> I think those set_fc() calls are confined to 68040 fault handling. Maybe
> we should try running stress tests on a 68030?

Re: [PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Finn Thain]

On Tue, 26 Mar 2024, Michael Schmitz wrote:

> The set_fc warning is specific to 040 but the bad kernel buserror (null 
> pointer) causing it isn't. 

I'm curious as to what happens if we just ignore the error and continue 
stress testing. You're right though -- we don't need an '030 for that 
(it's a non-fatal error).

> But testing on 030 would be a good idea if you have one with enough RAM.

My LC III has 36 MB which is sufficient for stress-ng. I will try it.

Re: [PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Finn Thain]

On Tue, 26 Mar 2024, Michael Schmitz wrote:

> see below (hopefully not whitespace-damaged).
> 

Following your approach, I added mutual exclusion to the other candidate 
in the same file, namely get_pointer_table(). Together with your patch, 
that seems to be sufficient to make stress-ng happy. I don't know the real 
extent of the data race here so I've probably used too much locking in 
this experiment (?)

diff --git a/arch/m68k/mm/motorola.c b/arch/m68k/mm/motorola.c
index 068ad7c9bb5c..d1ad55ee1b60 100644
--- a/arch/m68k/mm/motorola.c
+++ b/arch/m68k/mm/motorola.c
@@ -140,10 +140,14 @@ void __init init_pointer_table(void *table, int type)
 
 void *get_pointer_table(int type)
 {
-	ptable_desc *dp = ptable_list[type].next;
-	unsigned int mask = list_empty(&ptable_list[type]) ? 0 : PD_MARKBITS(dp);
+	ptable_desc *dp;
+	unsigned int mask;
 	unsigned int tmp, off;
 
+	preempt_disable();
+
+	dp = ptable_list[type].next;
+	mask = list_empty(&ptable_list[type]) ? 0 : PD_MARKBITS(dp);
 	/*
 	 * For a pointer table for a user process address space, a
 	 * table is taken from a page allocated for the purpose.  Each
@@ -154,8 +158,13 @@ void *get_pointer_table(int type)
 		void *page;
 		ptable_desc *new;
 
-		if (!(page = (void *)get_zeroed_page(GFP_KERNEL)))
+		sched_preempt_enable_no_resched();
+
+		if (!(page = (void *)get_zeroed_page(GFP_KERNEL))) {
 			return NULL;
+		}
+
+		preempt_disable();
 
 		if (type == TABLE_PTE) {
 			/*
@@ -171,6 +180,7 @@ void *get_pointer_table(int type)
 		PD_MARKBITS(new) = ptable_mask(type) - 1;
 		list_add_tail(new, dp);
 
+		sched_preempt_enable_no_resched();
 		return (pmd_t *)page;
 	}
 
@@ -181,6 +191,7 @@ void *get_pointer_table(int type)
 		/* move to end of list */
 		list_move_tail(dp, &ptable_list[type]);
 	}
+	sched_preempt_enable_no_resched();
 	return page_address(PD_PAGE(dp)) + off;
 }
 

Re: [PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Michael Schmitz]

Hi Finn,

thanks - I'm pretty sure tried that early on but botched it by excessive 
locking (i.e., keeping preemption disabled when calling 
get_zeroed_page()!).

So I don't think you do use too much locking here.

I'm running stress tests for a while now, without any trouble so far. 
Need to add a few other stressors back in, and repeat all that on a 
slower ARAnyM instance but I'm quite confident you found the solution.

Geert: with this data race fixed, it does appear my RFC patch is no 
longer needed. Finn or I probably ought to prepare a new RFC patch to go 
on top of your preemption patch. There is no commit ID to use in a 
Fixes: tag for that one, correct?

Cheers,

     Michael

On 26/03/24 22:58, Finn Thain wrote:
> On Tue, 26 Mar 2024, Michael Schmitz wrote:
>
>> see below (hopefully not whitespace-damaged).
>>
> Following your approach, I added mutual exclusion to the other candidate
> in the same file, namely get_pointer_table(). Together with your patch,
> that seems to be sufficient to make stress-ng happy. I don't know the real
> extent of the data race here so I've probably used too much locking in
> this experiment (?)
>
> diff --git a/arch/m68k/mm/motorola.c b/arch/m68k/mm/motorola.c
> index 068ad7c9bb5c..d1ad55ee1b60 100644
> --- a/arch/m68k/mm/motorola.c
> +++ b/arch/m68k/mm/motorola.c
> @@ -140,10 +140,14 @@ void __init init_pointer_table(void *table, int type)
>   
>   void *get_pointer_table(int type)
>   {
> -	ptable_desc *dp = ptable_list[type].next;
> -	unsigned int mask = list_empty(&ptable_list[type]) ? 0 : PD_MARKBITS(dp);
> +	ptable_desc *dp;
> +	unsigned int mask;
>   	unsigned int tmp, off;
>   
> +	preempt_disable();
> +
> +	dp = ptable_list[type].next;
> +	mask = list_empty(&ptable_list[type]) ? 0 : PD_MARKBITS(dp);
>   	/*
>   	 * For a pointer table for a user process address space, a
>   	 * table is taken from a page allocated for the purpose.  Each
> @@ -154,8 +158,13 @@ void *get_pointer_table(int type)
>   		void *page;
>   		ptable_desc *new;
>   
> -		if (!(page = (void *)get_zeroed_page(GFP_KERNEL)))
> +		sched_preempt_enable_no_resched();
> +
> +		if (!(page = (void *)get_zeroed_page(GFP_KERNEL))) {
>   			return NULL;
> +		}
> +
> +		preempt_disable();
>   
>   		if (type == TABLE_PTE) {
>   			/*
> @@ -171,6 +180,7 @@ void *get_pointer_table(int type)
>   		PD_MARKBITS(new) = ptable_mask(type) - 1;
>   		list_add_tail(new, dp);
>   
> +		sched_preempt_enable_no_resched();
>   		return (pmd_t *)page;
>   	}
>   
> @@ -181,6 +191,7 @@ void *get_pointer_table(int type)
>   		/* move to end of list */
>   		list_move_tail(dp, &ptable_list[type]);
>   	}
> +	sched_preempt_enable_no_resched();
>   	return page_address(PD_PAGE(dp)) + off;
>   }
>   

Re: [PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Finn Thain]

On Wed, 27 Mar 2024, Michael Schmitz wrote:

> thanks - I'm pretty sure tried that early on but botched it by excessive 
> locking (i.e., keeping preemption disabled when calling 
> get_zeroed_page()!).
> 
> So I don't think you do use too much locking here.
> 
> I'm running stress tests for a while now, without any trouble so far. 
> Need to add a few other stressors back in, and repeat all that on a 
> slower ARAnyM instance but I'm quite confident you found the solution.
> 
> Geert: with this data race fixed, it does appear my RFC patch is no 
> longer needed. Finn or I probably ought to prepare a new RFC patch to go 
> on top of your preemption patch. There is no commit ID to use in a 
> Fixes: tag for that one, correct?
> 

I don't think we need a formal fix. I'd be content for Geert to simply 
roll your free_pointer_table() changes and my get_pointer_table() changes 
into his own commit. That way, bisection won't produce an unstable build.

Re: [PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Michael Schmitz]

Hi Finn,

yes, that would be the end goal, but it might be easier for Geert to 
merge the two patches (or a single one) on top of his preempt branch if 
they come in the usual git format.

Cheers,

     Michael

On 27/03/24 11:03, Finn Thain wrote:
> On Wed, 27 Mar 2024, Michael Schmitz wrote:
>
>> thanks - I'm pretty sure tried that early on but botched it by excessive
>> locking (i.e., keeping preemption disabled when calling
>> get_zeroed_page()!).
>>
>> So I don't think you do use too much locking here.
>>
>> I'm running stress tests for a while now, without any trouble so far.
>> Need to add a few other stressors back in, and repeat all that on a
>> slower ARAnyM instance but I'm quite confident you found the solution.
>>
>> Geert: with this data race fixed, it does appear my RFC patch is no
>> longer needed. Finn or I probably ought to prepare a new RFC patch to go
>> on top of your preemption patch. There is no commit ID to use in a
>> Fixes: tag for that one, correct?
>>
> I don't think we need a formal fix. I'd be content for Geert to simply
> roll your free_pointer_table() changes and my get_pointer_table() changes
> into his own commit. That way, bisection won't produce an unstable build.

Re: [PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Finn Thain]

On Wed, 27 Mar 2024, Michael Schmitz wrote:

> it might be easier for Geert to merge the two patches (or a single one) 
> on top of his preempt branch if they come in the usual git format.

I suppose that would be easier, assuming it doesn't need a lot of rework. 
Anyway, here's the diff.

diff --git a/arch/m68k/mm/motorola.c b/arch/m68k/mm/motorola.c
index c1761d309fc6..d0092a3127c0 100644
--- a/arch/m68k/mm/motorola.c
+++ b/arch/m68k/mm/motorola.c
@@ -15,6 +15,7 @@
 #include <linux/mm.h>
 #include <linux/swap.h>
 #include <linux/kernel.h>
+#include <linux/preempt.h>
 #include <linux/string.h>
 #include <linux/types.h>
 #include <linux/init.h>
@@ -139,10 +140,14 @@ void __init init_pointer_table(void *table, int type)
 
 void *get_pointer_table(int type)
 {
-	ptable_desc *dp = ptable_list[type].next;
-	unsigned int mask = list_empty(&ptable_list[type]) ? 0 : PD_MARKBITS(dp);
+	ptable_desc *dp;
+	unsigned int mask;
 	unsigned int tmp, off;
 
+	preempt_disable();
+
+	dp = ptable_list[type].next;
+	mask = list_empty(&ptable_list[type]) ? 0 : PD_MARKBITS(dp);
 	/*
 	 * For a pointer table for a user process address space, a
 	 * table is taken from a page allocated for the purpose.  Each
@@ -153,9 +158,13 @@ void *get_pointer_table(int type)
 		void *page;
 		ptable_desc *new;
 
+		sched_preempt_enable_no_resched();
+
 		if (!(page = (void *)get_zeroed_page(GFP_KERNEL)))
 			return NULL;
 
+		preempt_disable();
+
 		if (type == TABLE_PTE) {
 			/*
 			 * m68k doesn't have SPLIT_PTE_PTLOCKS for not having
@@ -170,6 +179,7 @@ void *get_pointer_table(int type)
 		PD_MARKBITS(new) = ptable_mask(type) - 1;
 		list_add_tail(new, dp);
 
+		sched_preempt_enable_no_resched();
 		return (pmd_t *)page;
 	}
 
@@ -180,6 +190,7 @@ void *get_pointer_table(int type)
 		/* move to end of list */
 		list_move_tail(dp, &ptable_list[type]);
 	}
+	sched_preempt_enable_no_resched();
 	return page_address(PD_PAGE(dp)) + off;
 }
 
@@ -190,6 +201,8 @@ int free_pointer_table(void *table, int type)
 	unsigned long page = ptable & PAGE_MASK;
 	unsigned int mask = 1U << ((ptable - page)/ptable_size(type));
 
+	preempt_disable();
+
 	dp = PD_PTABLE(page);
 	if (PD_MARKBITS (dp) & mask)
 		panic ("table already free!");
@@ -203,6 +216,7 @@ int free_pointer_table(void *table, int type)
 		if (type == TABLE_PTE)
 			pagetable_pte_dtor(virt_to_ptdesc((void *)page));
 		free_page (page);
+		sched_preempt_enable_no_resched();
 		return 1;
 	} else if (ptable_list[type].next != dp) {
 		/*
@@ -211,6 +225,7 @@ int free_pointer_table(void *table, int type)
 		 */
 		list_move(dp, &ptable_list[type]);
 	}
+	sched_preempt_enable_no_resched();
 	return 0;
 }
 

Re: [PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Michael Schmitz]

Hi Finn,

thanks - that should suffice.

Tests on the slower ARAnyM instance also passed OK. Leaves test on 030 - 
I'll see whether I can find an earlier kernel version with a smaller 
memory footprint to test these patches on.

Now none of this has got us any closer to a solution for the spinlock 
recursion bug...

Cheers,

	Michael

Am 27.03.2024 um 16:50 schrieb Finn Thain:
>
> On Wed, 27 Mar 2024, Michael Schmitz wrote:
>
>> it might be easier for Geert to merge the two patches (or a single one)
>> on top of his preempt branch if they come in the usual git format.
>
> I suppose that would be easier, assuming it doesn't need a lot of rework.
> Anyway, here's the diff.
>
> diff --git a/arch/m68k/mm/motorola.c b/arch/m68k/mm/motorola.c
> index c1761d309fc6..d0092a3127c0 100644
> --- a/arch/m68k/mm/motorola.c
> +++ b/arch/m68k/mm/motorola.c
> @@ -15,6 +15,7 @@
>  #include <linux/mm.h>
>  #include <linux/swap.h>
>  #include <linux/kernel.h>
> +#include <linux/preempt.h>
>  #include <linux/string.h>
>  #include <linux/types.h>
>  #include <linux/init.h>
> @@ -139,10 +140,14 @@ void __init init_pointer_table(void *table, int type)
>
>  void *get_pointer_table(int type)
>  {
> -	ptable_desc *dp = ptable_list[type].next;
> -	unsigned int mask = list_empty(&ptable_list[type]) ? 0 : PD_MARKBITS(dp);
> +	ptable_desc *dp;
> +	unsigned int mask;
>  	unsigned int tmp, off;
>
> +	preempt_disable();
> +
> +	dp = ptable_list[type].next;
> +	mask = list_empty(&ptable_list[type]) ? 0 : PD_MARKBITS(dp);
>  	/*
>  	 * For a pointer table for a user process address space, a
>  	 * table is taken from a page allocated for the purpose.  Each
> @@ -153,9 +158,13 @@ void *get_pointer_table(int type)
>  		void *page;
>  		ptable_desc *new;
>
> +		sched_preempt_enable_no_resched();
> +
>  		if (!(page = (void *)get_zeroed_page(GFP_KERNEL)))
>  			return NULL;
>
> +		preempt_disable();
> +
>  		if (type == TABLE_PTE) {
>  			/*
>  			 * m68k doesn't have SPLIT_PTE_PTLOCKS for not having
> @@ -170,6 +179,7 @@ void *get_pointer_table(int type)
>  		PD_MARKBITS(new) = ptable_mask(type) - 1;
>  		list_add_tail(new, dp);
>
> +		sched_preempt_enable_no_resched();
>  		return (pmd_t *)page;
>  	}
>
> @@ -180,6 +190,7 @@ void *get_pointer_table(int type)
>  		/* move to end of list */
>  		list_move_tail(dp, &ptable_list[type]);
>  	}
> +	sched_preempt_enable_no_resched();
>  	return page_address(PD_PAGE(dp)) + off;
>  }
>
> @@ -190,6 +201,8 @@ int free_pointer_table(void *table, int type)
>  	unsigned long page = ptable & PAGE_MASK;
>  	unsigned int mask = 1U << ((ptable - page)/ptable_size(type));
>
> +	preempt_disable();
> +
>  	dp = PD_PTABLE(page);
>  	if (PD_MARKBITS (dp) & mask)
>  		panic ("table already free!");
> @@ -203,6 +216,7 @@ int free_pointer_table(void *table, int type)
>  		if (type == TABLE_PTE)
>  			pagetable_pte_dtor(virt_to_ptdesc((void *)page));
>  		free_page (page);
> +		sched_preempt_enable_no_resched();
>  		return 1;
>  	} else if (ptable_list[type].next != dp) {
>  		/*
> @@ -211,6 +225,7 @@ int free_pointer_table(void *table, int type)
>  		 */
>  		list_move(dp, &ptable_list[type]);
>  	}
> +	sched_preempt_enable_no_resched();
>  	return 0;
>  }
>
>

Spinlock debug, was Re: [PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Finn Thain]

On Wed, 27 Mar 2024, Michael Schmitz wrote:

> Now none of this has got us any closer to a solution for the spinlock 
> recursion bug...
> 

It appears that the spinlock recursion check really works: I am able to 
reproduce the following failure every time I paste too much text into the 
qemu console.

This looks like a straight-forward driver bug, whereby the "irq flood" 
printk() results in pmz_console_write() attempting to lock the spinlock 
already locked by pmz_interrupt(). The spinlock at 0x62e140 is the one in 
struct uart_port.

I'll see if I can come up with a driver patch. 

[   14.560000] ttyPZ0: pmz: rx irq flood !
[   14.560000] BUG: spinlock recursion on CPU#0, swapper/0
[   14.560000]  lock: 0x62e140, .magic: dead4ead, .owner: swapper/0, .owner_cpu: 0
[   14.560000] CPU: 0 PID: 0 Comm: swapper Not tainted 6.8.0-mac-dbg-preempt-00004-g4143b7b9144a #1
[   14.560000] Stack from 0059bcc4:
[   14.560000]         0059bcc4 0056316f 0056316f 00002700 004b6444 0059bce4 004ad8c6 0056316f
[   14.560000]         0059bd10 004a6546 00556759 0062e140 dead4ead 0059f892 00000000 00000000
[   14.560000]         0062e140 0059bde8 005c03d0 0059bd24 0004daf6 0062e140 005567bf 0062e140
[   14.560000]         0059bd34 004b64c2 0062e140 00000001 0059bd50 002e15ea 0062e140 00000001
[   14.560000]         0059bde7 0059bde8 005c03d0 0059bdac 0005124e 005c03d0 005cdc00 0000002b
[   14.560000]         005a3caa 005a3caa 00000000 0059bde8 0004ff00 0059be8b 00038200 000529ba
[   14.560000] Call Trace: [<00002700>] ret_from_kernel_thread+0xc/0x14
[   14.560000]  [<004b6444>] _raw_spin_lock+0x0/0x28
[   14.560000]  [<004ad8c6>] dump_stack+0x10/0x16
[   14.560000]  [<004a6546>] spin_dump+0x6e/0x7c
[   14.560000]  [<0004daf6>] do_raw_spin_lock+0x9c/0xa6
[   14.560000]  [<004b64c2>] _raw_spin_lock_irqsave+0x2a/0x34
[   14.560000]  [<002e15ea>] pmz_console_write+0x32/0x9a
[   14.560000]  [<0005124e>] console_flush_all+0x112/0x3a2
[   14.560000]  [<0004ff00>] console_trylock+0x0/0x7a
[   14.560000]  [<00038200>] parameq+0x48/0x6e
[   14.560000]  [<000529ba>] __printk_safe_enter+0x0/0x36
[   14.560000]  [<0005113c>] console_flush_all+0x0/0x3a2
[   14.560000]  [<000542c4>] prb_read_valid+0x0/0x1a
[   14.560000]  [<004b65a4>] _raw_spin_unlock+0x0/0x38
[   14.560000]  [<0005151e>] console_unlock+0x40/0xb8
[   14.560000]  [<00038200>] parameq+0x48/0x6e
[   14.560000]  [<002c778c>] __tty_insert_flip_string_flags+0x0/0x14e
[   14.560000]  [<00051798>] vprintk_emit+0x156/0x238
[   14.560000]  [<00051894>] vprintk_default+0x1a/0x1e
[   14.560000]  [<000529a8>] vprintk+0x74/0x86
[   14.560000]  [<004a6596>] _printk+0x12/0x16
[   14.560000]  [<002e23be>] pmz_receive_chars+0x1cc/0x394
[   14.560000]  [<004b6444>] _raw_spin_lock+0x0/0x28
[   14.560000]  [<00038226>] parse_args+0x0/0x3a6
[   14.560000]  [<004b6466>] _raw_spin_lock+0x22/0x28
[   14.560000]  [<002e26b4>] pmz_interrupt+0x12e/0x1e0
[   14.560000]  [<00048680>] arch_cpu_idle_enter+0x0/0x8
[   14.560000]  [<00054ebc>] __handle_irq_event_percpu+0x24/0x106
[   14.560000]  [<004ae576>] default_idle_call+0x0/0x46
[   14.560000]  [<00055020>] handle_irq_event+0x30/0x90
[   14.560000]  [<00058320>] handle_simple_irq+0x5e/0xc0
[   14.560000]  [<00048688>] arch_cpu_idle_exit+0x0/0x8
[   14.560000]  [<00054800>] generic_handle_irq+0x3c/0x4a
[   14.560000]  [<00002978>] do_IRQ+0x24/0x3a
[   14.560000]  [<004ae508>] cpu_idle_poll.isra.0+0x0/0x6e
[   14.560000]  [<00002874>] auto_irqhandler_fixup+0x4/0xc
[   14.560000]  [<004ae508>] cpu_idle_poll.isra.0+0x0/0x6e
[   14.560000]  [<004ae576>] default_idle_call+0x0/0x46
[   14.560000]  [<004ae598>] default_idle_call+0x22/0x46
[   14.560000]  [<00048710>] do_idle+0x6a/0xf0
[   14.560000]  [<000486a6>] do_idle+0x0/0xf0
[   14.560000]  [<000367d2>] find_task_by_pid_ns+0x0/0x2a
[   14.560000]  [<0005d064>] __rcu_read_lock+0x0/0x12
[   14.560000]  [<00048a5a>] cpu_startup_entry+0x18/0x1c
[   14.560000]  [<00063a06>] __rcu_read_unlock+0x0/0x26
[   14.560000]  [<004ae65a>] kernel_init+0x0/0xfa
[   14.560000]  [<0049c5a8>] strcpy+0x0/0x1e
[   14.560000]  [<004a6584>] _printk+0x0/0x16
[   14.560000]  [<0049c72a>] strlen+0x0/0x22
[   14.560000]  [<006452d4>] memblock_alloc_try_nid+0x0/0x82
[   14.560000]  [<0063939a>] arch_post_acpi_subsys_init+0x0/0x8
[   14.560000]  [<0063991e>] console_on_rootfs+0x0/0x60
[   14.560000]  [<00638410>] _sinittext+0x410/0xadc
[   14.560000] 

Re: Spinlock debug, was Re: [PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Finn Thain]

On Thu, 28 Mar 2024, Michael Schmitz wrote:

> On 28/03/24 12:29, Finn Thain wrote:
> > On Wed, 27 Mar 2024, Michael Schmitz wrote:
> >
> >> Now none of this has got us any closer to a solution for the spinlock 
> >> recursion bug...
> >>
> > It appears that the spinlock recursion check really works: I am able 
> > to reproduce the following failure every time I paste too much text 
> > into the qemu console.
> >
> > This looks like a straight-forward driver bug, whereby the "irq flood" 
> > printk() results in pmz_console_write() attempting to lock the 
> > spinlock already locked by pmz_interrupt(). The spinlock at 0x62e140 
> > is the one in struct uart_port.
> 
> That's a legitimate recursion warning then. pmz_console_write() should 
> not retake the lock already held when called from interrupt context.
> 
> I agree the recursion checks did work correctly in this case.
> 
> Spinlock recursion in scheduler_tick() doesn't look possible from 
> reentering scheduler_tick() through code in its call path, so the 
> mechanism there would be different.
> 
> We did rule out the VIA timer interrupt (or MFP timer A interrupt) 
> getting reentered, right?
> 

I never added an assertion to prove that because I don't have a way to 
reproduce the "spinlock recursion" BUG reported by Guenter.

Re: Spinlock debug, was Re: [PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Michael Schmitz]

Hi Finn,

Am 28.03.2024 um 15:48 schrieb Finn Thain:
>> Spinlock recursion in scheduler_tick() doesn't look possible from
>> reentering scheduler_tick() through code in its call path, so the
>> mechanism there would be different.
>>
>> We did rule out the VIA timer interrupt (or MFP timer A interrupt)
>> getting reentered, right?
>>
>
> I never added an assertion to prove that because I don't have a way to
> reproduce the "spinlock recursion" BUG reported by Guenter.

Right. I can reproduce the bug in ARAnyM, so I'll check that the timer 
interrupt isn't reentered there. Doesn't quite amount to the same thing 
though.

Cheers,

	Michael

Re: Spinlock debug, was Re: [PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Michael Schmitz]

Hi Finn,

Am 29.03.2024 um 08:54 schrieb Michael Schmitz:
> Hi Finn,
>
> Am 28.03.2024 um 15:48 schrieb Finn Thain:
>>> Spinlock recursion in scheduler_tick() doesn't look possible from
>>> reentering scheduler_tick() through code in its call path, so the
>>> mechanism there would be different.
>>>
>>> We did rule out the VIA timer interrupt (or MFP timer A interrupt)
>>> getting reentered, right?
>>>
>>
>> I never added an assertion to prove that because I don't have a way to
>> reproduce the "spinlock recursion" BUG reported by Guenter.
>
> Right. I can reproduce the bug in ARAnyM, so I'll check that the timer
> interrupt isn't reentered there. Doesn't quite amount to the same thing
> though.

The MFP timer C is not reentered in 66 boots even though I've seen it 
appear in the stack trace of 8 recursion warnings.

This was with full preempt - no different from earlier when I got a 
similar fraction of warnings at boot.

Cheers,

	Michael

Re: Spinlock debug, was Re: [PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Finn Thain]

On Sat, 30 Mar 2024, Michael Schmitz wrote:

> 
> The MFP timer C is not reentered in 66 boots even though I've seen it 
> appear in the stack trace of 8 recursion warnings.
> 

The spinlock recursion warning gives the address of the spinlock. Did you 
figure out which spinlock it is? Also, did you capture all 8 backtraces?

Re: Spinlock debug, was Re: [PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Michael Schmitz]

[-- Attachment #1: Type: text/plain, Size: 747 bytes --]

Hi Finn,

Am 30.03.2024 um 19:23 schrieb Finn Thain:
> On Sat, 30 Mar 2024, Michael Schmitz wrote:
>
>>
>> The MFP timer C is not reentered in 66 boots even though I've seen it
>> appear in the stack trace of 8 recursion warnings.
>>
>
> The spinlock recursion warning gives the address of the spinlock. Did you
> figure out which spinlock it is? Also, did you capture all 8 backtraces?

The lock is that in 'runqueues', the single struct rq in use on a single 
processor system.

That much was clear from the backtrace and looking at the code in 
scheduler_tick() before.

I've attached all eight backtraces - the stack contents only varies in a 
few positions, mostly explainable by differences in the initial stack 
pointer.

Cheers,

	Michael

[-- Attachment #2: spinlockdebug-74-traces.txt --]
[-- Type: text/plain, Size: 13671 bytes --]

BUG: spinlock recursion on CPU#0, swapper/1
 lock: 0x447600, .magic: dead4ead, .owner: swapper/1, .owner_cpu: 0
CPU: 0 PID: 1 Comm: swapper Not tainted 6.8.0-rc7-atari-fpuemu-spinlocktest+ #1775
Stack from 00831e98:
        00831e98 003e766e 003e766e 00000000 0000000d 00335630 003e766e 0003aac6
        00447600 003dc464 00830000 000321a6 00447600 0083c000 00831ee0 000321cc
        00447600 00000000 00831eec 000335be 00814080 0003fdea 00055464 00002604
        00006e16 00000000 00000000 0003fe5e 0000000d 00000000 00000000 00000000
        00443048 0032c472 0047aa4c 0047aaa8 00000000 00000000 0003fef6 0047aa4c
        00000000 0047aa4c 0003ff4e 0047aa4c 0047aaa8 0047aa4c 0047aaa8 0047aa4c
Call Trace: [<00335630>] dump_stack+0xc/0x10
 [<0003aac6>] do_raw_spin_lock+0x34/0x58
 [<000321a6>] raw_spin_rq_lock_nested+0x24/0x38
 [<000321cc>] raw_spin_rq_lock.constprop.135+0x12/0x18
 [<000335be>] scheduler_tick+0x10/0x66
 [<0003fdea>] __irq_wake_thread+0x0/0x40
 [<00055464>] update_process_times+0x5e/0x66
 [<00002604>] ret_from_kernel_thread+0x4/0x14
 [<00006e16>] mfp_timer_c_handler+0x5a/0x70
 [<0003fe5e>] __handle_irq_event_percpu+0x34/0xbc
 [<0032c472>] sprintf+0x0/0x24
 [<0003fef6>] handle_irq_event_percpu+0x10/0x3a
 [<0003ff4e>] handle_irq_event+0x2e/0x54
 [<000424f6>] handle_simple_irq+0x56/0x66
 [<00335976>] kernel_init+0x0/0xf2
 [<0003fa00>] handle_irq_desc+0x1e/0x28
 [<000028c0>] do_IRQ+0x20/0x32
 [<000027f4>] user_irqvec_fixup+0xc/0x14
 [<0032c472>] sprintf+0x0/0x24
 [<00002600>] ret_from_kernel_thread+0x0/0x14

RCU Tasks: Setting shift to 0 and lim to 1 rcu_task_cb_adjust=1.
RCU Tasks Trace: Setting shift to 0 and lim to 1 rcu_task_cb_adjust=1.
--
BUG: spinlock recursion on CPU#0, pool_workqueue_/3
 lock: 0x447600, .magic: dead4ead, .owner: pool_workqueue_/3, .owner_cpu: 0
CPU: 0 PID: 3 Comm: pool_workqueue_ Not tainted 6.8.0-rc7-atari-fpuemu-spinlocktest+ #1775
Stack from 00837e98:
        00837e98 003e766e 003e766e 00000000 0000000d 00335630 003e766e 0003aac6
        00447600 003dc464 00836000 000321a6 00447600 0083cc20 00837ee0 000321cc
        00447600 00000000 00837eec 000335be 00814080 0003fdea 00055464 00002604
        00006e16 00000000 00000000 0003fe5e 0000000d 00000000 00000000 00339ee0
        0033a016 000183f2 0047aa4c 0047aaa8 00000000 00000000 0003fef6 0047aa4c
        00000000 0047aa4c 0003ff4e 0047aa4c 0047aaa8 0047aa4c 0047aaa8 0047aa4c
Call Trace: [<00335630>] dump_stack+0xc/0x10
 [<0003aac6>] do_raw_spin_lock+0x34/0x58
 [<000321a6>] raw_spin_rq_lock_nested+0x24/0x38
 [<000321cc>] raw_spin_rq_lock.constprop.135+0x12/0x18
 [<000335be>] scheduler_tick+0x10/0x66
 [<0003fdea>] __irq_wake_thread+0x0/0x40
 [<00055464>] update_process_times+0x5e/0x66
 [<00002604>] ret_from_kernel_thread+0x4/0x14
 [<00006e16>] mfp_timer_c_handler+0x5a/0x70
 [<0003fe5e>] __handle_irq_event_percpu+0x34/0xbc
 [<00339ee0>] _raw_spin_lock+0x0/0x22
 [<0033a016>] _raw_spin_unlock+0x0/0x32
 [<000183f2>] kernel_thread+0x0/0x6c
 [<0003fef6>] handle_irq_event_percpu+0x10/0x3a
 [<0003ff4e>] handle_irq_event+0x2e/0x54
 [<000424f6>] handle_simple_irq+0x56/0x66
 [<0002de6e>] kthread+0x0/0xd4
 [<0003fa00>] handle_irq_desc+0x1e/0x28
 [<000028c0>] do_IRQ+0x20/0x32
 [<000027f4>] user_irqvec_fixup+0xc/0x14
 [<00339ee0>] _raw_spin_lock+0x0/0x22
 [<0033a016>] _raw_spin_unlock+0x0/0x32
 [<000183f2>] kernel_thread+0x0/0x6c
--
BUG: spinlock recursion on CPU#0, swapper/2
 lock: 0x447600, .magic: dead4ead, .owner: swapper/2, .owner_cpu: 0
CPU: 0 PID: 2 Comm: swapper Not tainted 6.8.0-rc7-atari-fpuemu-spinlocktest+ #1775
Stack from 00835e98:
        00835e98 003e766e 003e766e 00000000 0000000d 00335630 003e766e 0003aac6
        00447600 003dc464 00834000 000321a6 00447600 0083c610 00835ee0 000321cc
        00447600 00000000 00835eec 000335be 00814080 0003fdea 00055464 00002604
        00006e16 00000000 00000000 0003fe5e 0000000d 00000000 00000000 00831f10
        00000102 0032c472 0047aa4c 0047aaa8 00000000 00000000 0003fef6 0047aa4c
        00000000 0047aa4c 0003ff4e 0047aa4c 0047aaa8 0047aa4c 0047aaa8 0047aa4c
Call Trace: [<00335630>] dump_stack+0xc/0x10
 [<0003aac6>] do_raw_spin_lock+0x34/0x58
 [<000321a6>] raw_spin_rq_lock_nested+0x24/0x38
 [<000321cc>] raw_spin_rq_lock.constprop.135+0x12/0x18
 [<000335be>] scheduler_tick+0x10/0x66
 [<0003fdea>] __irq_wake_thread+0x0/0x40
 [<00055464>] update_process_times+0x5e/0x66
 [<00002604>] ret_from_kernel_thread+0x4/0x14
 [<00006e16>] mfp_timer_c_handler+0x5a/0x70
 [<0003fe5e>] __handle_irq_event_percpu+0x34/0xbc
 [<0032c472>] sprintf+0x0/0x24
 [<0003fef6>] handle_irq_event_percpu+0x10/0x3a
 [<0003ff4e>] handle_irq_event+0x2e/0x54
 [<000424f6>] handle_simple_irq+0x56/0x66
 [<0002dfd2>] kthreadd+0x0/0x12a
 [<0003fa00>] handle_irq_desc+0x1e/0x28
 [<000028c0>] do_IRQ+0x20/0x32
 [<000027f4>] user_irqvec_fixup+0xc/0x14
 [<0032c472>] sprintf+0x0/0x24
 [<00002600>] ret_from_kernel_thread+0x0/0x14

RCU Tasks: Setting shift to 0 and lim to 1 rcu_task_cb_adjust=1.
RCU Tasks Trace: Setting shift to 0 and lim to 1 rcu_task_cb_adjust=1.
--
BUG: spinlock recursion on CPU#0, swapper/2
 lock: 0x447600, .magic: dead4ead, .owner: swapper/2, .owner_cpu: 0
CPU: 0 PID: 2 Comm: swapper Not tainted 6.8.0-rc7-atari-fpuemu-spinlocktest+ #1775
Stack from 00835e98:
        00835e98 003e766e 003e766e 00000000 0000000d 00335630 003e766e 0003aac6
        00447600 003dc464 00834000 000321a6 00447600 0083c610 00835ee0 000321cc
        00447600 00000000 00835eec 000335be 00814080 0003fdea 00055464 00002604
        00006e16 00000000 00000000 0003fe5e 0000000d 00000000 00000000 00831f10
        00000102 0032c472 0047aa4c 0047aaa8 00000000 00000000 0003fef6 0047aa4c
        00000000 0047aa4c 0003ff4e 0047aa4c 0047aaa8 0047aa4c 0047aaa8 0047aa4c
Call Trace: [<00335630>] dump_stack+0xc/0x10
 [<0003aac6>] do_raw_spin_lock+0x34/0x58
 [<000321a6>] raw_spin_rq_lock_nested+0x24/0x38
 [<000321cc>] raw_spin_rq_lock.constprop.135+0x12/0x18
 [<000335be>] scheduler_tick+0x10/0x66
 [<0003fdea>] __irq_wake_thread+0x0/0x40
 [<00055464>] update_process_times+0x5e/0x66
 [<00002604>] ret_from_kernel_thread+0x4/0x14
 [<00006e16>] mfp_timer_c_handler+0x5a/0x70
 [<0003fe5e>] __handle_irq_event_percpu+0x34/0xbc
 [<0032c472>] sprintf+0x0/0x24
 [<0003fef6>] handle_irq_event_percpu+0x10/0x3a
 [<0003ff4e>] handle_irq_event+0x2e/0x54
 [<000424f6>] handle_simple_irq+0x56/0x66
 [<0002dfd2>] kthreadd+0x0/0x12a
 [<0003fa00>] handle_irq_desc+0x1e/0x28
 [<000028c0>] do_IRQ+0x20/0x32
 [<000027f4>] user_irqvec_fixup+0xc/0x14
 [<0032c472>] sprintf+0x0/0x24
 [<00002600>] ret_from_kernel_thread+0x0/0x14

RCU Tasks: Setting shift to 0 and lim to 1 rcu_task_cb_adjust=1.
RCU Tasks Trace: Setting shift to 0 and lim to 1 rcu_task_cb_adjust=1.
--
BUG: spinlock recursion on CPU#0, swapper/2
 lock: 0x447600, .magic: dead4ead, .owner: swapper/2, .owner_cpu: 0
CPU: 0 PID: 2 Comm: swapper Not tainted 6.8.0-rc7-atari-fpuemu-spinlocktest+ #1775
Stack from 00835e98:
        00835e98 003e766e 003e766e 00000000 0000000d 00335630 003e766e 0003aac6
        00447600 003dc464 00834000 000321a6 00447600 0083c610 00835ee0 000321cc
        00447600 00000000 00835eec 000335be 00814080 0003fdea 00055464 00002604
        00006e16 00000000 00000000 0003fe5e 0000000d 00000000 00000000 00831f10
        00000102 0032c472 0047aa4c 0047aaa8 00000000 00000000 0003fef6 0047aa4c
        00000000 0047aa4c 0003ff4e 0047aa4c 0047aaa8 0047aa4c 0047aaa8 0047aa4c
Call Trace: [<00335630>] dump_stack+0xc/0x10
 [<0003aac6>] do_raw_spin_lock+0x34/0x58
 [<000321a6>] raw_spin_rq_lock_nested+0x24/0x38
 [<000321cc>] raw_spin_rq_lock.constprop.135+0x12/0x18
 [<000335be>] scheduler_tick+0x10/0x66
 [<0003fdea>] __irq_wake_thread+0x0/0x40
 [<00055464>] update_process_times+0x5e/0x66
 [<00002604>] ret_from_kernel_thread+0x4/0x14
 [<00006e16>] mfp_timer_c_handler+0x5a/0x70
 [<0003fe5e>] __handle_irq_event_percpu+0x34/0xbc
 [<0032c472>] sprintf+0x0/0x24
 [<0003fef6>] handle_irq_event_percpu+0x10/0x3a
 [<0003ff4e>] handle_irq_event+0x2e/0x54
 [<000424f6>] handle_simple_irq+0x56/0x66
 [<0002dfd2>] kthreadd+0x0/0x12a
 [<0003fa00>] handle_irq_desc+0x1e/0x28
 [<000028c0>] do_IRQ+0x20/0x32
 [<000027f4>] user_irqvec_fixup+0xc/0x14
 [<0032c472>] sprintf+0x0/0x24
 [<00002600>] ret_from_kernel_thread+0x0/0x14

RCU Tasks: Setting shift to 0 and lim to 1 rcu_task_cb_adjust=1.
RCU Tasks Trace: Setting shift to 0 and lim to 1 rcu_task_cb_adjust=1.
--
BUG: spinlock recursion on CPU#0, pool_workqueue_/3
 lock: 0x447600, .magic: dead4ead, .owner: pool_workqueue_/3, .owner_cpu: 0
CPU: 0 PID: 3 Comm: pool_workqueue_ Not tainted 6.8.0-rc7-atari-fpuemu-spinlocktest+ #1775
Stack from 00837e78:
        00837e78 003e766e 003e766e 00000000 0000000d 00335630 003e766e 0003aac6
        00447600 003dc464 00836000 000321a6 00447600 0083cc20 00837ec0 000321cc
        00447600 00000000 00837ecc 000335be 00814080 0003fdea 00055464 00002604
        00006e16 00000000 00000000 0003fe5e 0000000d 00000000 00000000 00339ee0
        0033a016 000183f2 0047aa4c 0047aaa8 00000000 00837fb4 0003fef6 0047aa4c
        00000000 0047aa4c 0003ff4e 0047aa4c 0047aaa8 0047aa4c 0047aaa8 0047aa4c
Call Trace: [<00335630>] dump_stack+0xc/0x10
 [<0003aac6>] do_raw_spin_lock+0x34/0x58
 [<000321a6>] raw_spin_rq_lock_nested+0x24/0x38
 [<000321cc>] raw_spin_rq_lock.constprop.135+0x12/0x18
 [<000335be>] scheduler_tick+0x10/0x66
 [<0003fdea>] __irq_wake_thread+0x0/0x40
 [<00055464>] update_process_times+0x5e/0x66
 [<00002604>] ret_from_kernel_thread+0x4/0x14
 [<00006e16>] mfp_timer_c_handler+0x5a/0x70
 [<0003fe5e>] __handle_irq_event_percpu+0x34/0xbc
 [<00339ee0>] _raw_spin_lock+0x0/0x22
 [<0033a016>] _raw_spin_unlock+0x0/0x32
 [<000183f2>] kernel_thread+0x0/0x6c
 [<0003fef6>] handle_irq_event_percpu+0x10/0x3a
 [<0003ff4e>] handle_irq_event+0x2e/0x54
 [<000424f6>] handle_simple_irq+0x56/0x66
 [<0003fa00>] handle_irq_desc+0x1e/0x28
 [<000028c0>] do_IRQ+0x20/0x32
 [<000027f4>] user_irqvec_fixup+0xc/0x14
 [<00339ee0>] _raw_spin_lock+0x0/0x22
 [<0033a016>] _raw_spin_unlock+0x0/0x32
 [<000183f2>] kernel_thread+0x0/0x6c
 [<0002de6e>] kthread+0x0/0xd4
--
BUG: spinlock recursion on CPU#0, swapper/1
 lock: 0x447600, .magic: dead4ead, .owner: swapper/1, .owner_cpu: 0
CPU: 0 PID: 1 Comm: swapper Not tainted 6.8.0-rc7-atari-fpuemu-spinlocktest+ #1775
Stack from 00831e98:
        00831e98 003e766e 003e766e 00000000 0000000d 00335630 003e766e 0003aac6
        00447600 003dc464 00830000 000321a6 00447600 0083c000 00831ee0 000321cc
        00447600 00000000 00831eec 000335be 00814080 0003fdea 00055464 00002604
        00006e16 00000000 00000000 0003fe5e 0000000d 00000000 00000000 00000000
        00443048 0032c472 0047aa4c 0047aaa8 00000000 00000000 0003fef6 0047aa4c
        00000000 0047aa4c 0003ff4e 0047aa4c 0047aaa8 0047aa4c 0047aaa8 0047aa4c
Call Trace: [<00335630>] dump_stack+0xc/0x10
 [<0003aac6>] do_raw_spin_lock+0x34/0x58
 [<000321a6>] raw_spin_rq_lock_nested+0x24/0x38
 [<000321cc>] raw_spin_rq_lock.constprop.135+0x12/0x18
 [<000335be>] scheduler_tick+0x10/0x66
 [<0003fdea>] __irq_wake_thread+0x0/0x40
 [<00055464>] update_process_times+0x5e/0x66
 [<00002604>] ret_from_kernel_thread+0x4/0x14
 [<00006e16>] mfp_timer_c_handler+0x5a/0x70
 [<0003fe5e>] __handle_irq_event_percpu+0x34/0xbc
 [<0032c472>] sprintf+0x0/0x24
 [<0003fef6>] handle_irq_event_percpu+0x10/0x3a
 [<0003ff4e>] handle_irq_event+0x2e/0x54
 [<000424f6>] handle_simple_irq+0x56/0x66
 [<00335976>] kernel_init+0x0/0xf2
 [<0003fa00>] handle_irq_desc+0x1e/0x28
 [<000028c0>] do_IRQ+0x20/0x32
 [<000027f4>] user_irqvec_fixup+0xc/0x14
 [<0032c472>] sprintf+0x0/0x24
 [<00002600>] ret_from_kernel_thread+0x0/0x14

RCU Tasks: Setting shift to 0 and lim to 1 rcu_task_cb_adjust=1.
RCU Tasks Trace: Setting shift to 0 and lim to 1 rcu_task_cb_adjust=1.
--
BUG: spinlock recursion on CPU#0, swapper/2
 lock: 0x447600, .magic: dead4ead, .owner: swapper/2, .owner_cpu: 0
CPU: 0 PID: 2 Comm: swapper Not tainted 6.8.0-rc7-atari-fpuemu-spinlocktest+ #1775
Stack from 00835e98:
        00835e98 003e766e 003e766e 00000000 0000000d 00335630 003e766e 0003aac6
        00447600 003dc464 00834000 000321a6 00447600 0083c610 00835ee0 000321cc
        00447600 00000000 00835eec 000335be 00814080 0003fdea 00055464 00002604
        00006e16 00000000 00000000 0003fe5e 0000000d 00000000 00000000 00831f10
        00000102 0032c472 0047aa4c 0047aaa8 00000000 00000000 0003fef6 0047aa4c
        00000000 0047aa4c 0003ff4e 0047aa4c 0047aaa8 0047aa4c 0047aaa8 0047aa4c
Call Trace: [<00335630>] dump_stack+0xc/0x10
 [<0003aac6>] do_raw_spin_lock+0x34/0x58
 [<000321a6>] raw_spin_rq_lock_nested+0x24/0x38
 [<000321cc>] raw_spin_rq_lock.constprop.135+0x12/0x18
 [<000335be>] scheduler_tick+0x10/0x66
 [<0003fdea>] __irq_wake_thread+0x0/0x40
 [<00055464>] update_process_times+0x5e/0x66
 [<00002604>] ret_from_kernel_thread+0x4/0x14
 [<00006e16>] mfp_timer_c_handler+0x5a/0x70
 [<0003fe5e>] __handle_irq_event_percpu+0x34/0xbc
 [<0032c472>] sprintf+0x0/0x24
 [<0003fef6>] handle_irq_event_percpu+0x10/0x3a
 [<0003ff4e>] handle_irq_event+0x2e/0x54
 [<000424f6>] handle_simple_irq+0x56/0x66
 [<0002dfd2>] kthreadd+0x0/0x12a
 [<0003fa00>] handle_irq_desc+0x1e/0x28
 [<000028c0>] do_IRQ+0x20/0x32
 [<000027f4>] user_irqvec_fixup+0xc/0x14
 [<0032c472>] sprintf+0x0/0x24
 [<00002600>] ret_from_kernel_thread+0x0/0x14

RCU Tasks: Setting shift to 0 and lim to 1 rcu_task_cb_adjust=1.
RCU Tasks Trace: Setting shift to 0 and lim to 1 rcu_task_cb_adjust=1.

Re: [PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Finn Thain]

On Wed, 27 Mar 2024, Finn Thain wrote:

> 
> On Wed, 27 Mar 2024, Michael Schmitz wrote:
> 
> > it might be easier for Geert to merge the two patches (or a single one) 
> > on top of his preempt branch if they come in the usual git format.
> 
> I suppose that would be easier, assuming it doesn't need a lot of rework. 
> Anyway, here's the diff.
> 
> diff --git a/arch/m68k/mm/motorola.c b/arch/m68k/mm/motorola.c
> index c1761d309fc6..d0092a3127c0 100644
> --- a/arch/m68k/mm/motorola.c
> +++ b/arch/m68k/mm/motorola.c
> @@ -15,6 +15,7 @@
>  #include <linux/mm.h>
>  #include <linux/swap.h>
>  #include <linux/kernel.h>
> +#include <linux/preempt.h>
>  #include <linux/string.h>
>  #include <linux/types.h>
>  #include <linux/init.h>
> @@ -139,10 +140,14 @@ void __init init_pointer_table(void *table, int type)
>  
>  void *get_pointer_table(int type)
>  {
> -	ptable_desc *dp = ptable_list[type].next;
> -	unsigned int mask = list_empty(&ptable_list[type]) ? 0 : PD_MARKBITS(dp);
> +	ptable_desc *dp;
> +	unsigned int mask;
>  	unsigned int tmp, off;
>  
> +	preempt_disable();
> +
> +	dp = ptable_list[type].next;
> +	mask = list_empty(&ptable_list[type]) ? 0 : PD_MARKBITS(dp);
>  	/*
>  	 * For a pointer table for a user process address space, a
>  	 * table is taken from a page allocated for the purpose.  Each
> @@ -153,9 +158,13 @@ void *get_pointer_table(int type)
>  		void *page;
>  		ptable_desc *new;
>  
> +		sched_preempt_enable_no_resched();
> +
>  		if (!(page = (void *)get_zeroed_page(GFP_KERNEL)))
>  			return NULL;
>  
> +		preempt_disable();
> +
>  		if (type == TABLE_PTE) {
>  			/*
>  			 * m68k doesn't have SPLIT_PTE_PTLOCKS for not having
> @@ -170,6 +179,7 @@ void *get_pointer_table(int type)
>  		PD_MARKBITS(new) = ptable_mask(type) - 1;
>  		list_add_tail(new, dp);
>  
> +		sched_preempt_enable_no_resched();
>  		return (pmd_t *)page;
>  	}
>  
> @@ -180,6 +190,7 @@ void *get_pointer_table(int type)
>  		/* move to end of list */
>  		list_move_tail(dp, &ptable_list[type]);
>  	}
> +	sched_preempt_enable_no_resched();
>  	return page_address(PD_PAGE(dp)) + off;
>  }
>  
> @@ -190,6 +201,8 @@ int free_pointer_table(void *table, int type)
>  	unsigned long page = ptable & PAGE_MASK;
>  	unsigned int mask = 1U << ((ptable - page)/ptable_size(type));
>  
> +	preempt_disable();
> +
>  	dp = PD_PTABLE(page);
>  	if (PD_MARKBITS (dp) & mask)
>  		panic ("table already free!");
> @@ -203,6 +216,7 @@ int free_pointer_table(void *table, int type)
>  		if (type == TABLE_PTE)
>  			pagetable_pte_dtor(virt_to_ptdesc((void *)page));
>  		free_page (page);
> +		sched_preempt_enable_no_resched();
>  		return 1;
>  	} else if (ptable_list[type].next != dp) {
>  		/*
> @@ -211,6 +225,7 @@ int free_pointer_table(void *table, int type)
>  		 */
>  		list_move(dp, &ptable_list[type]);
>  	}
> +	sched_preempt_enable_no_resched();
>  	return 0;
>  }
>  
> 

For my part,

Signed-off-by: Finn Thain <fthain@linux-m68k.org>

Re: [PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Michael Schmitz]

Hi Geert,

On 27/03/24 21:38, Finn Thain wrote:
> On Wed, 27 Mar 2024, Finn Thain wrote:
>
>> On Wed, 27 Mar 2024, Michael Schmitz wrote:
>>
>>> it might be easier for Geert to merge the two patches (or a single one)
>>> on top of his preempt branch if they come in the usual git format.
>> I suppose that would be easier, assuming it doesn't need a lot of rework.
>> Anyway, here's the diff.
>>
>> diff --git a/arch/m68k/mm/motorola.c b/arch/m68k/mm/motorola.c
>> index c1761d309fc6..d0092a3127c0 100644
>> --- a/arch/m68k/mm/motorola.c
>> +++ b/arch/m68k/mm/motorola.c
>> @@ -15,6 +15,7 @@
>>   #include <linux/mm.h>
>>   #include <linux/swap.h>
>>   #include <linux/kernel.h>
>> +#include <linux/preempt.h>
>>   #include <linux/string.h>
>>   #include <linux/types.h>
>>   #include <linux/init.h>
>> @@ -139,10 +140,14 @@ void __init init_pointer_table(void *table, int type)
>>   
>>   void *get_pointer_table(int type)
>>   {
>> -	ptable_desc *dp = ptable_list[type].next;
>> -	unsigned int mask = list_empty(&ptable_list[type]) ? 0 : PD_MARKBITS(dp);
>> +	ptable_desc *dp;
>> +	unsigned int mask;
>>   	unsigned int tmp, off;
>>   
>> +	preempt_disable();
>> +
>> +	dp = ptable_list[type].next;
>> +	mask = list_empty(&ptable_list[type]) ? 0 : PD_MARKBITS(dp);
>>   	/*
>>   	 * For a pointer table for a user process address space, a
>>   	 * table is taken from a page allocated for the purpose.  Each
>> @@ -153,9 +158,13 @@ void *get_pointer_table(int type)
>>   		void *page;
>>   		ptable_desc *new;
>>   
>> +		sched_preempt_enable_no_resched();
>> +
>>   		if (!(page = (void *)get_zeroed_page(GFP_KERNEL)))
>>   			return NULL;
>>   
>> +		preempt_disable();
>> +
>>   		if (type == TABLE_PTE) {
>>   			/*
>>   			 * m68k doesn't have SPLIT_PTE_PTLOCKS for not having
>> @@ -170,6 +179,7 @@ void *get_pointer_table(int type)
>>   		PD_MARKBITS(new) = ptable_mask(type) - 1;
>>   		list_add_tail(new, dp);
>>   
>> +		sched_preempt_enable_no_resched();
>>   		return (pmd_t *)page;
>>   	}
>>   
>> @@ -180,6 +190,7 @@ void *get_pointer_table(int type)
>>   		/* move to end of list */
>>   		list_move_tail(dp, &ptable_list[type]);
>>   	}
>> +	sched_preempt_enable_no_resched();
>>   	return page_address(PD_PAGE(dp)) + off;
>>   }
>>   
>> @@ -190,6 +201,8 @@ int free_pointer_table(void *table, int type)
>>   	unsigned long page = ptable & PAGE_MASK;
>>   	unsigned int mask = 1U << ((ptable - page)/ptable_size(type));
>>   
>> +	preempt_disable();
>> +
>>   	dp = PD_PTABLE(page);
>>   	if (PD_MARKBITS (dp) & mask)
>>   		panic ("table already free!");
>> @@ -203,6 +216,7 @@ int free_pointer_table(void *table, int type)
>>   		if (type == TABLE_PTE)
>>   			pagetable_pte_dtor(virt_to_ptdesc((void *)page));
>>   		free_page (page);
>> +		sched_preempt_enable_no_resched();
>>   		return 1;
>>   	} else if (ptable_list[type].next != dp) {
>>   		/*
>> @@ -211,6 +225,7 @@ int free_pointer_table(void *table, int type)
>>   		 */
>>   		list_move(dp, &ptable_list[type]);
>>   	}
>> +	sched_preempt_enable_no_resched();
>>   	return 0;
>>   }
>>   
>>
> For my part,
>
> Signed-off-by: Finn Thain <fthain@linux-m68k.org>

Same for me:

Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

Re: [PATCH RFC] m68k: skip kernel premption if interrupts were disabled

[Geert Uytterhoeven]

Hi Michael,

On Tue, Mar 26, 2024 at 8:59 PM Michael Schmitz <schmitzmic@gmail.com> wrote:
> thanks - I'm pretty sure tried that early on but botched it by excessive
> locking (i.e., keeping preemption disabled when calling
> get_zeroed_page()!).
>
> So I don't think you do use too much locking here.
>
> I'm running stress tests for a while now, without any trouble so far.
> Need to add a few other stressors back in, and repeat all that on a
> slower ARAnyM instance but I'm quite confident you found the solution.
>
> Geert: with this data race fixed, it does appear my RFC patch is no
> longer needed. Finn or I probably ought to prepare a new RFC patch to go
> on top of your preemption patch. There is no commit ID to use in a
> Fixes: tag for that one, correct?

As my preemption patch was not applied to any tree yet, I'll just
fold in any fixes.  I think this does deserve Co-developed-by tags,
so please make sure to include your SoB in any fixes.

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds