* spinlock recursion in aio_complete()
@ 2023-05-21 5:43 Helge Deller
2023-05-22 19:28 ` Bart Van Assche
0 siblings, 1 reply; 11+ messages in thread
From: Helge Deller @ 2023-05-21 5:43 UTC (permalink / raw)
To: Linux SCSI List, linux-aio, linux-parisc
Hi,
On a single-CPU parisc64 machine I face the spinlock recursion below.
Happens reproduceably directly at bootup since kernel 6.2 (and ~ 6.1.5).
Kernel is built for SMP. Same kernel binary works nicely on machines with more than
one CPU, but stops on UP machines.
Any idea or patch I could try?
Helge
[ 26.116568] fuse: init (API version 7.38)
[ 26.338231] loop: module loaded
[ 26.357390] BUG: spinlock recursion on CPU#0, systemd/1
[ 26.357504] lock: 0x4ad42618, .magic: dead4ead, .owner: systemd/1, .owner_cpu: 0
[ 26.357884] CPU: 0 PID: 1 Comm: systemd Not tainted 6.3.3+ #180
[ 26.357976] Hardware name: 9000/800/rp3410
[ 26.361055] Backtrace:
[ 26.361055] [<000000004030c4d0>] show_stack+0x74/0xb0
[ 26.361055] [<00000000412ec758>] dump_stack_lvl+0xd8/0x128
[ 26.361055] [<00000000412ec7dc>] dump_stack+0x34/0x48
[ 26.361055] [<00000000412d6c8c>] spin_dump+0xe0/0x120
[ 26.361055] [<00000000403f026c>] do_raw_spin_lock+0x138/0x1a0
[ 26.361055] [<00000000412efcb0>] _raw_spin_lock_irqsave+0x3c/0x78
[ 26.361055] [<0000000040808b9c>] aio_complete+0x68/0x470
[ 26.361055] [<000000004080bd54>] aio_complete_rw+0x200/0x400
[ 26.361055] [<0000000040aac314>] blkdev_bio_end_io_async+0x60/0x140
[ 26.361055] [<0000000040ab0fb0>] bio_endio+0x274/0x318
[ 26.361055] [<0000000040ad0b38>] blk_update_request+0x2bc/0x600
[ 26.361055] [<0000000040e6daa4>] scsi_end_request+0x60/0x370
[ 26.361055] [<0000000040e6ef68>] scsi_io_completion+0x9c/0x8e8
[ 26.361055] [<0000000040e5d5c8>] scsi_finish_command+0x10c/0x290
[ 26.361055] [<0000000040e6e7c4>] scsi_complete+0x118/0x308
[ 26.361055] [<0000000040acd3fc>] blk_mq_complete_request+0x60/0x98
[ 26.361055] [<0000000040e6eaf0>] scsi_done_internal+0x13c/0x1c8
[ 26.361055] [<0000000040e6ebac>] scsi_done+0x30/0x60
[ 26.361055] [<0000000040e8b140>] sym_xpt_done+0x94/0xc8
[ 26.361055] [<0000000040e91588>] sym_interrupt+0x5bc/0x28e8
[ 26.361055] [<0000000040e88c1c>] sym53c8xx_intr+0x98/0x170
[ 26.361055] [<00000000403fe380>] __handle_irq_event_percpu+0xdc/0x338
[ 26.361055] [<00000000403fe618>] handle_irq_event_percpu+0x3c/0xf8
[ 26.361055] [<000000004040925c>] handle_percpu_irq+0xb8/0x110
[ 26.361055] [<00000000403fcc74>] generic_handle_irq+0x60/0xb0
[ 26.361055] [<000000004030f930>] do_cpu_irq_mask+0x304/0x508
[ 26.361055] [<0000000040303070>] intr_return+0x0/0xc
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: spinlock recursion in aio_complete()
2023-05-21 5:43 spinlock recursion in aio_complete() Helge Deller
@ 2023-05-22 19:28 ` Bart Van Assche
2023-05-22 20:51 ` Helge Deller
0 siblings, 1 reply; 11+ messages in thread
From: Bart Van Assche @ 2023-05-22 19:28 UTC (permalink / raw)
To: Helge Deller, Linux SCSI List, linux-aio, linux-parisc
On 5/20/23 22:43, Helge Deller wrote:
> On a single-CPU parisc64 machine I face the spinlock recursion below.
> Happens reproduceably directly at bootup since kernel 6.2 (and ~ 6.1.5).
> Kernel is built for SMP. Same kernel binary works nicely on machines with more than
> one CPU, but stops on UP machines.
> Any idea or patch I could try?
How about performing one or more of the following actions?
* Translating aio_complete+0x68 into a line number.
* Repeating the test with lockdep enabled.
* Bisecting this issue.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: spinlock recursion in aio_complete()
2023-05-22 19:28 ` Bart Van Assche
@ 2023-05-22 20:51 ` Helge Deller
2023-05-22 20:58 ` Bart Van Assche
0 siblings, 1 reply; 11+ messages in thread
From: Helge Deller @ 2023-05-22 20:51 UTC (permalink / raw)
To: Bart Van Assche, Linux SCSI List, linux-aio, linux-parisc
On 5/22/23 21:28, Bart Van Assche wrote:
> On 5/20/23 22:43, Helge Deller wrote:
>> On a single-CPU parisc64 machine I face the spinlock recursion below.
>> Happens reproduceably directly at bootup since kernel 6.2 (and ~ 6.1.5).
>> Kernel is built for SMP. Same kernel binary works nicely on machines with more than
>> one CPU, but stops on UP machines.
>> Any idea or patch I could try?
>
> How about performing one or more of the following actions?
> * Translating aio_complete+0x68 into a line number.
It hangs in fs/aio.c:1128, function aio_complete(), in this call:
spin_lock_irqsave(&ctx->completion_lock, flags);
> * Repeating the test with lockdep enabled.
Hmm... We don't have LOCKDEP yet on parisc :-(
> * Bisecting this issue.
Will try, but this process is *very* slow as it happens on one machine only,
it's only accessible remotely, uses endless time to reboot and I have no
easy way to netboot kernels...
Helge
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: spinlock recursion in aio_complete()
2023-05-22 20:51 ` Helge Deller
@ 2023-05-22 20:58 ` Bart Van Assche
2023-05-22 21:22 ` Helge Deller
0 siblings, 1 reply; 11+ messages in thread
From: Bart Van Assche @ 2023-05-22 20:58 UTC (permalink / raw)
To: Helge Deller, Linux SCSI List, linux-aio, linux-parisc
On 5/22/23 13:51, Helge Deller wrote:
> On 5/22/23 21:28, Bart Van Assche wrote:
>> On 5/20/23 22:43, Helge Deller wrote:
>>> On a single-CPU parisc64 machine I face the spinlock recursion below.
>>> Happens reproduceably directly at bootup since kernel 6.2 (and ~ 6.1.5).
>>> Kernel is built for SMP. Same kernel binary works nicely on machines with more than
>>> one CPU, but stops on UP machines.
>>> Any idea or patch I could try?
>>
>> How about performing one or more of the following actions?
>> * Translating aio_complete+0x68 into a line number.
>
> It hangs in fs/aio.c:1128, function aio_complete(), in this call:
> spin_lock_irqsave(&ctx->completion_lock, flags);
All code that I found and that obtains ctx->completion_lock disables IRQs.
It is not clear to me how this spinlock can be locked recursively? Is it
sure that the "spinlock recursion" report is correct?
Thanks,
Bart.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: spinlock recursion in aio_complete()
2023-05-22 20:58 ` Bart Van Assche
@ 2023-05-22 21:22 ` Helge Deller
2023-05-23 10:24 ` Helge Deller
0 siblings, 1 reply; 11+ messages in thread
From: Helge Deller @ 2023-05-22 21:22 UTC (permalink / raw)
To: Bart Van Assche, Linux SCSI List, linux-aio, linux-parisc
On 5/22/23 22:58, Bart Van Assche wrote:
> On 5/22/23 13:51, Helge Deller wrote:
>> On 5/22/23 21:28, Bart Van Assche wrote:
>>> On 5/20/23 22:43, Helge Deller wrote:
>>>> On a single-CPU parisc64 machine I face the spinlock recursion below.
>>>> Happens reproduceably directly at bootup since kernel 6.2 (and ~ 6.1.5).
>>>> Kernel is built for SMP. Same kernel binary works nicely on machines with more than
>>>> one CPU, but stops on UP machines.
>>>> Any idea or patch I could try?
>>>
>>> How about performing one or more of the following actions?
>>> * Translating aio_complete+0x68 into a line number.
>>
>> It hangs in fs/aio.c:1128, function aio_complete(), in this call:
>> spin_lock_irqsave(&ctx->completion_lock, flags);
>
> All code that I found and that obtains ctx->completion_lock disables IRQs.
> It is not clear to me how this spinlock can be locked recursively? Is it
> sure that the "spinlock recursion" report is correct?
Yes, it seems correct.
I can see the machine hanging in exactly the same lock when I build without
the CONFIG_DEBUG_SPINLOCK option and check the processor registers with the TOC
command (TOC on parisc means: stop CPU instruction processing, save all CPU registers
to an area which can be analyzed at a later point in time, then reboot the machine).
So, the spinlock recursion from kernel and TOC both agree.
Again, the strange thing is, that I can only reproduce it on that specific machine,
which has only one CPU but runs the SMP kernel.
Helge
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: spinlock recursion in aio_complete()
2023-05-22 21:22 ` Helge Deller
@ 2023-05-23 10:24 ` Helge Deller
2023-05-23 10:51 ` Russell King (Oracle)
0 siblings, 1 reply; 11+ messages in thread
From: Helge Deller @ 2023-05-23 10:24 UTC (permalink / raw)
To: Bart Van Assche, Linux SCSI List, linux-aio, linux-parisc,
Russell King, linux-arm-kernel@lists.infradead.org
On 5/22/23 23:22, Helge Deller wrote:
>>> It hangs in fs/aio.c:1128, function aio_complete(), in this call:
>>> spin_lock_irqsave(&ctx->completion_lock, flags);
>>
>> All code that I found and that obtains ctx->completion_lock disables IRQs.
>> It is not clear to me how this spinlock can be locked recursively? Is it
>> sure that the "spinlock recursion" report is correct?
>
> Yes, it seems correct.
> [...]
Bart, thanks to your suggestions I was able to narrow down the problem!
I got LOCKDEP working on parisc, which then reports:
raw_local_irq_restore() called with IRQs enabled
for the spin_unlock_irqrestore() in function aio_complete(), which shouldn't happen.
Finally, I found that parisc's flush_dcache_page() re-enables the IRQs
which leads to the spinlock hang in aio_complete().
So, this is NOT a bug in aio or scsci, but we need fix in the the arch code.
While checking flush_dcache_page() re-enables IRQs, I see on parisc and ARM(32):
flush_dcache_page() calls:
-> flush_dcache_mmap_lock() / flush_dcache_mmap_unlock()
which uses: xa_lock_irq() / xa_unlock_irq()
So, the call to xa_unlock_irq() re-enables the IRQs unconditionally
and triggers the hang in aio_complete().
I temporarily #defined flush_dcache_mmap_lock() to NOP and the kernel booted nicely.
Not sure yet what the best fix is...
Helge
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: spinlock recursion in aio_complete()
2023-05-23 10:24 ` Helge Deller
@ 2023-05-23 10:51 ` Russell King (Oracle)
2023-05-23 20:01 ` Helge Deller
0 siblings, 1 reply; 11+ messages in thread
From: Russell King (Oracle) @ 2023-05-23 10:51 UTC (permalink / raw)
To: Helge Deller
Cc: Bart Van Assche, Linux SCSI List, linux-aio, linux-parisc,
linux-arm-kernel@lists.infradead.org
On Tue, May 23, 2023 at 12:24:04PM +0200, Helge Deller wrote:
> On 5/22/23 23:22, Helge Deller wrote:
> > > > It hangs in fs/aio.c:1128, function aio_complete(), in this call:
> > > > spin_lock_irqsave(&ctx->completion_lock, flags);
> > >
> > > All code that I found and that obtains ctx->completion_lock disables IRQs.
> > > It is not clear to me how this spinlock can be locked recursively? Is it
> > > sure that the "spinlock recursion" report is correct?
> >
> > Yes, it seems correct.
> > [...]
>
> Bart, thanks to your suggestions I was able to narrow down the problem!
>
> I got LOCKDEP working on parisc, which then reports:
> raw_local_irq_restore() called with IRQs enabled
> for the spin_unlock_irqrestore() in function aio_complete(), which shouldn't happen.
>
> Finally, I found that parisc's flush_dcache_page() re-enables the IRQs
> which leads to the spinlock hang in aio_complete().
>
> So, this is NOT a bug in aio or scsci, but we need fix in the the arch code.
You can find some of the background to this at:
https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=16ceff2d5dc9f0347ab5a08abff3f4647c2fee04
which introduced flush_dcache_mmap_lock(). It looks like Hugh had
questions over whether this should be _irqsave() rather than _irq()
but I guess at the time all callers had interrupts enabled, and
it's only recently that someone came up with the idea of calling
flush_dcache_page() with interrupts disabled.
Adding another arg to flush_dcache_mmap_lock() to save the flags
may be doable, but requires a patch that touches not only architectures
that have a private implementation, but also various code in mm/.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: spinlock recursion in aio_complete()
2023-05-23 10:51 ` Russell King (Oracle)
@ 2023-05-23 20:01 ` Helge Deller
2023-05-23 20:06 ` Bart Van Assche
2023-05-23 20:24 ` Helge Deller
0 siblings, 2 replies; 11+ messages in thread
From: Helge Deller @ 2023-05-23 20:01 UTC (permalink / raw)
To: Russell King (Oracle), Dinh Nguyen
Cc: Helge Deller, Bart Van Assche, Linux SCSI List, linux-aio,
linux-parisc, linux-arm-kernel@lists.infradead.org
* Russell King (Oracle) <linux@armlinux.org.uk>:
> On Tue, May 23, 2023 at 12:24:04PM +0200, Helge Deller wrote:
> > On 5/22/23 23:22, Helge Deller wrote:
> > > > > It hangs in fs/aio.c:1128, function aio_complete(), in this call:
> > > > > spin_lock_irqsave(&ctx->completion_lock, flags);
> > > >
> > > > All code that I found and that obtains ctx->completion_lock disables IRQs.
> > > > It is not clear to me how this spinlock can be locked recursively? Is it
> > > > sure that the "spinlock recursion" report is correct?
> > >
> > > Yes, it seems correct.
> > > [...]
> >
> > Bart, thanks to your suggestions I was able to narrow down the problem!
> >
> > I got LOCKDEP working on parisc, which then reports:
> > raw_local_irq_restore() called with IRQs enabled
> > for the spin_unlock_irqrestore() in function aio_complete(), which shouldn't happen.
> >
> > Finally, I found that parisc's flush_dcache_page() re-enables the IRQs
> > which leads to the spinlock hang in aio_complete().
> >
> > So, this is NOT a bug in aio or scsci, but we need fix in the the arch code.
>
> You can find some of the background to this at:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=16ceff2d5dc9f0347ab5a08abff3f4647c2fee04
>
> which introduced flush_dcache_mmap_lock(). It looks like Hugh had
> questions over whether this should be _irqsave() rather than _irq()
> but I guess at the time all callers had interrupts enabled, and
> it's only recently that someone came up with the idea of calling
> flush_dcache_page() with interrupts disabled.
>
> Adding another arg to flush_dcache_mmap_lock() to save the flags
> may be doable, but requires a patch that touches not only architectures
> that have a private implementation, but also various code in mm/.
I've tested the attached patch on parisc, and it solves the issue.
I've not compile-tested it on arm and nios2, both seem to be
the only other affected platforms.
Thoughts?
Helge
From 25a96a4211975d46e6f4dac06e144d0fb9f5ed53 Mon Sep 17 00:00:00 2001
From: Helge Deller <deller@gmx.de>
Date: Tue, 23 May 2023 21:48:33 +0200
Subject: [PATCH] Fix flush_dcache_page() for usage in irq context
flush_dcache_page() can be called with IRQs disabled, e.g. from
aio_complete().
Fix flush_dcache_page() on the arm, parisc and nios2 architectures
to not unintentionally re-enable IRQs by using xa_lock_irqsave() instead
of xa_lock_irq() for the flush_dcache_mmap_*lock() functions.
Cc: Russell King (Oracle) <linux@armlinux.org.uk>
Cc: Dinh Nguyen <dinguyen@kernel.org>
Signed-off-by: Helge Deller <deller@gmx.de>
diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
index a094f964c869..5b8a1ef0dc50 100644
--- a/arch/arm/include/asm/cacheflush.h
+++ b/arch/arm/include/asm/cacheflush.h
@@ -315,6 +315,10 @@ static inline void flush_anon_page(struct vm_area_struct *vma,
#define flush_dcache_mmap_lock(mapping) xa_lock_irq(&mapping->i_pages)
#define flush_dcache_mmap_unlock(mapping) xa_unlock_irq(&mapping->i_pages)
+#define flush_dcache_mmap_lock_irqsave(mapping, flags) \
+ xa_lock_irqsave(&mapping->i_pages, flags)
+#define flush_dcache_mmap_unlock_irqrestore(mapping, flags) \
+ xa_unlock_irqrestore(&mapping->i_pages, flags)
/*
* We don't appear to need to do anything here. In fact, if we did, we'd
diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
index 7ff9feea13a6..d57ec9165520 100644
--- a/arch/arm/mm/flush.c
+++ b/arch/arm/mm/flush.c
@@ -238,6 +238,7 @@ static void __flush_dcache_aliases(struct address_space *mapping, struct page *p
{
struct mm_struct *mm = current->active_mm;
struct vm_area_struct *mpnt;
+ unsigned long flags;
pgoff_t pgoff;
/*
@@ -248,7 +249,7 @@ static void __flush_dcache_aliases(struct address_space *mapping, struct page *p
*/
pgoff = page->index;
- flush_dcache_mmap_lock(mapping);
+ flush_dcache_mmap_lock_irqsave(mapping, flags);
vma_interval_tree_foreach(mpnt, &mapping->i_mmap, pgoff, pgoff) {
unsigned long offset;
@@ -262,7 +263,7 @@ static void __flush_dcache_aliases(struct address_space *mapping, struct page *p
offset = (pgoff - mpnt->vm_pgoff) << PAGE_SHIFT;
flush_cache_page(mpnt, mpnt->vm_start + offset, page_to_pfn(page));
}
- flush_dcache_mmap_unlock(mapping);
+ flush_dcache_mmap_unlock_irqrestore(mapping, flags);
}
#if __LINUX_ARM_ARCH__ >= 6
diff --git a/arch/nios2/include/asm/cacheflush.h b/arch/nios2/include/asm/cacheflush.h
index d0b71dd71287..a37242662809 100644
--- a/arch/nios2/include/asm/cacheflush.h
+++ b/arch/nios2/include/asm/cacheflush.h
@@ -48,5 +48,9 @@ extern void invalidate_dcache_range(unsigned long start, unsigned long end);
#define flush_dcache_mmap_lock(mapping) xa_lock_irq(&mapping->i_pages)
#define flush_dcache_mmap_unlock(mapping) xa_unlock_irq(&mapping->i_pages)
+#define flush_dcache_mmap_lock_irqsave(mapping, flags) \
+ xa_lock_irqsave(&mapping->i_pages, flags)
+#define flush_dcache_mmap_unlock_irqrestore(mapping, flags) \
+ xa_unlock_irqrestore(&mapping->i_pages, flags)
#endif /* _ASM_NIOS2_CACHEFLUSH_H */
diff --git a/arch/nios2/mm/cacheflush.c b/arch/nios2/mm/cacheflush.c
index 6aa9257c3ede..35f3b599187f 100644
--- a/arch/nios2/mm/cacheflush.c
+++ b/arch/nios2/mm/cacheflush.c
@@ -75,11 +75,12 @@ static void flush_aliases(struct address_space *mapping, struct page *page)
{
struct mm_struct *mm = current->active_mm;
struct vm_area_struct *mpnt;
+ unsigned long flags;
pgoff_t pgoff;
pgoff = page->index;
- flush_dcache_mmap_lock(mapping);
+ flush_dcache_mmap_lock_irqsave(mapping, flags);
vma_interval_tree_foreach(mpnt, &mapping->i_mmap, pgoff, pgoff) {
unsigned long offset;
@@ -92,7 +93,7 @@ static void flush_aliases(struct address_space *mapping, struct page *page)
flush_cache_page(mpnt, mpnt->vm_start + offset,
page_to_pfn(page));
}
- flush_dcache_mmap_unlock(mapping);
+ flush_dcache_mmap_unlock_irqrestore(mapping, flags);
}
void flush_cache_all(void)
diff --git a/arch/parisc/include/asm/cacheflush.h b/arch/parisc/include/asm/cacheflush.h
index 0bdee6724132..c8b6928cee1e 100644
--- a/arch/parisc/include/asm/cacheflush.h
+++ b/arch/parisc/include/asm/cacheflush.h
@@ -48,6 +48,10 @@ void flush_dcache_page(struct page *page);
#define flush_dcache_mmap_lock(mapping) xa_lock_irq(&mapping->i_pages)
#define flush_dcache_mmap_unlock(mapping) xa_unlock_irq(&mapping->i_pages)
+#define flush_dcache_mmap_lock_irqsave(mapping, flags) \
+ xa_lock_irqsave(&mapping->i_pages, flags)
+#define flush_dcache_mmap_unlock_irqrestore(mapping, flags) \
+ xa_unlock_irqrestore(&mapping->i_pages, flags)
#define flush_icache_page(vma,page) do { \
flush_kernel_dcache_page_addr(page_address(page)); \
diff --git a/arch/parisc/kernel/cache.c b/arch/parisc/kernel/cache.c
index 1d3b8bc8a623..ca4a302d4365 100644
--- a/arch/parisc/kernel/cache.c
+++ b/arch/parisc/kernel/cache.c
@@ -399,6 +399,7 @@ void flush_dcache_page(struct page *page)
unsigned long offset;
unsigned long addr, old_addr = 0;
unsigned long count = 0;
+ unsigned long flags;
pgoff_t pgoff;
if (mapping && !mapping_mapped(mapping)) {
@@ -420,7 +421,7 @@ void flush_dcache_page(struct page *page)
* to flush one address here for them all to become coherent
* on machines that support equivalent aliasing
*/
- flush_dcache_mmap_lock(mapping);
+ flush_dcache_mmap_lock_irqsave(mapping, flags);
vma_interval_tree_foreach(mpnt, &mapping->i_mmap, pgoff, pgoff) {
offset = (pgoff - mpnt->vm_pgoff) << PAGE_SHIFT;
addr = mpnt->vm_start + offset;
@@ -460,7 +461,7 @@ void flush_dcache_page(struct page *page)
}
WARN_ON(++count == 4096);
}
- flush_dcache_mmap_unlock(mapping);
+ flush_dcache_mmap_unlock_irqrestore(mapping, flags);
}
EXPORT_SYMBOL(flush_dcache_page);
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: spinlock recursion in aio_complete()
2023-05-23 20:01 ` Helge Deller
@ 2023-05-23 20:06 ` Bart Van Assche
2023-05-23 20:12 ` Helge Deller
2023-05-23 20:24 ` Helge Deller
1 sibling, 1 reply; 11+ messages in thread
From: Bart Van Assche @ 2023-05-23 20:06 UTC (permalink / raw)
To: Helge Deller, Russell King (Oracle), Dinh Nguyen
Cc: Linux SCSI List, linux-aio, linux-parisc,
linux-arm-kernel@lists.infradead.org
On 5/23/23 13:01, Helge Deller wrote:
> Subject: [PATCH] Fix flush_dcache_page() for usage in irq context
>
> flush_dcache_page() can be called with IRQs disabled, e.g. from
> aio_complete().
>
> Fix flush_dcache_page() on the arm, parisc and nios2 architectures
> to not unintentionally re-enable IRQs by using xa_lock_irqsave() instead
> of xa_lock_irq() for the flush_dcache_mmap_*lock() functions.
Please consider adding a Fixes: tag such that this patch is picked up
automatically by the stable tree maintainers.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: spinlock recursion in aio_complete()
2023-05-23 20:06 ` Bart Van Assche
@ 2023-05-23 20:12 ` Helge Deller
0 siblings, 0 replies; 11+ messages in thread
From: Helge Deller @ 2023-05-23 20:12 UTC (permalink / raw)
To: Bart Van Assche, Russell King (Oracle), Dinh Nguyen
Cc: Linux SCSI List, linux-aio, linux-parisc,
linux-arm-kernel@lists.infradead.org
On 5/23/23 22:06, Bart Van Assche wrote:
> On 5/23/23 13:01, Helge Deller wrote:
>> Subject: [PATCH] Fix flush_dcache_page() for usage in irq context
>>
>> flush_dcache_page() can be called with IRQs disabled, e.g. from
>> aio_complete().
>>
>> Fix flush_dcache_page() on the arm, parisc and nios2 architectures
>> to not unintentionally re-enable IRQs by using xa_lock_irqsave() instead
>> of xa_lock_irq() for the flush_dcache_mmap_*lock() functions.
>
> Please consider adding a Fixes: tag such that this patch is picked up
> automatically by the stable tree maintainers.
Sure. I'll probably split it up as per-arch patch as well. Just wanted
to get some feedback first.
Helge
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: spinlock recursion in aio_complete()
2023-05-23 20:01 ` Helge Deller
2023-05-23 20:06 ` Bart Van Assche
@ 2023-05-23 20:24 ` Helge Deller
1 sibling, 0 replies; 11+ messages in thread
From: Helge Deller @ 2023-05-23 20:24 UTC (permalink / raw)
To: Helge Deller
Cc: Russell King (Oracle), Dinh Nguyen, Bart Van Assche,
Linux SCSI List, linux-aio, linux-parisc,
linux-arm-kernel@lists.infradead.org
* Helge Deller <deller@gmx.de>:
> * Russell King (Oracle) <linux@armlinux.org.uk>:
> > On Tue, May 23, 2023 at 12:24:04PM +0200, Helge Deller wrote:
> > > On 5/22/23 23:22, Helge Deller wrote:
> > > > > > It hangs in fs/aio.c:1128, function aio_complete(), in this call:
> > > > > > spin_lock_irqsave(&ctx->completion_lock, flags);
> > > > >
> > > > > All code that I found and that obtains ctx->completion_lock disables IRQs.
> > > > > It is not clear to me how this spinlock can be locked recursively? Is it
> > > > > sure that the "spinlock recursion" report is correct?
> > > >
> > > > Yes, it seems correct.
> > > > [...]
> > >
> > > Bart, thanks to your suggestions I was able to narrow down the problem!
> > >
> > > I got LOCKDEP working on parisc, which then reports:
> > > raw_local_irq_restore() called with IRQs enabled
> > > for the spin_unlock_irqrestore() in function aio_complete(), which shouldn't happen.
> > >
> > > Finally, I found that parisc's flush_dcache_page() re-enables the IRQs
> > > which leads to the spinlock hang in aio_complete().
> > >
> > > So, this is NOT a bug in aio or scsci, but we need fix in the the arch code.
> >
> > You can find some of the background to this at:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=16ceff2d5dc9f0347ab5a08abff3f4647c2fee04
> >
> > which introduced flush_dcache_mmap_lock(). It looks like Hugh had
> > questions over whether this should be _irqsave() rather than _irq()
> > but I guess at the time all callers had interrupts enabled, and
> > it's only recently that someone came up with the idea of calling
> > flush_dcache_page() with interrupts disabled.
> >
> > Adding another arg to flush_dcache_mmap_lock() to save the flags
> > may be doable, but requires a patch that touches not only architectures
> > that have a private implementation, but also various code in mm/.
>
> I've tested the attached patch on parisc, and it solves the issue.
> I've not compile-tested it on arm and nios2, both seem to be
> the only other affected platforms.
For your convenience, here is the hunk I used to trigger the bug.
It triggers immediately at bootup when starting userspace.
Helge
diff --git a/fs/aio.c b/fs/aio.c
index b0b17bd098bb..6076b0ab5580 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1127,6 +1127,7 @@ static void aio_complete(struct aio_kiocb *iocb)
*/
spin_lock_irqsave(&ctx->completion_lock, flags);
+ BUG_ON(!arch_irqs_disabled());
tail = ctx->tail;
pos = tail + AIO_EVENTS_OFFSET;
@@ -1139,7 +1140,10 @@ static void aio_complete(struct aio_kiocb *iocb)
*event = iocb->ki_res;
kunmap_atomic(ev_page);
+ BUG_ON(!arch_irqs_disabled());
+ /* the next flush_dcache_page() should keep IRQs disabled */
flush_dcache_page(ctx->ring_pages[pos / AIO_EVENTS_PER_PAGE]);
+ BUG_ON(!arch_irqs_disabled());
pr_debug("%p[%u]: %p: %p %Lx %Lx %Lx\n", ctx, tail, iocb,
(void __user *)(unsigned long)iocb->ki_res.obj,
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-05-23 20:24 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-21 5:43 spinlock recursion in aio_complete() Helge Deller
2023-05-22 19:28 ` Bart Van Assche
2023-05-22 20:51 ` Helge Deller
2023-05-22 20:58 ` Bart Van Assche
2023-05-22 21:22 ` Helge Deller
2023-05-23 10:24 ` Helge Deller
2023-05-23 10:51 ` Russell King (Oracle)
2023-05-23 20:01 ` Helge Deller
2023-05-23 20:06 ` Bart Van Assche
2023-05-23 20:12 ` Helge Deller
2023-05-23 20:24 ` Helge Deller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).