2.6.24-git: kmap_atomic() WARN_ON()

2.6.24-git: kmap_atomic() WARN_ON()

[Thomas Gleixner]

current mainline triggers:

WARNING: at /home/tglx/work/kernel/x86/linux-2.6/arch/x86/mm/highmem_32.c:52 kmap_atomic_prot+0xe5/0x19b()
Modules linked in: ahci(+) sata_sil libata sd_mod scsi_mod raid1 ext3 jbd ehci_hcd ohci_hcd uhci_hcd
Pid: 0, comm: swapper Not tainted 2.6.24 #173
 [<c0126b60>] warn_on_slowpath+0x41/0x51
 [<c011c5eb>] ? __enqueue_entity+0x9c/0xa4
 [<c011c717>] ? enqueue_entity+0x124/0x13b
 [<c011cedb>] ? enqueue_task_fair+0x41/0x4c
 [<c032ce88>] ? _spin_lock_irqsave+0x14/0x2e
 [<c012e885>] ? lock_timer_base+0x1f/0x3e
 [<c011ad6d>] kmap_atomic_prot+0xe5/0x19b
 [<c011ae37>] kmap_atomic+0x14/0x16
 [<f88ea218>] ata_scsi_rbuf_get+0x1e/0x2c [libata]
 [<f88ea821>] atapi_qc_complete+0x23f/0x289 [libata]
 [<f88e3d00>] __ata_qc_complete+0x8e/0x93 [libata]
 [<f88e3fc7>] ata_qc_complete+0x115/0x128 [libata]
 [<f88e8d47>] ata_qc_complete_multiple+0x86/0xa0 [libata]
 [<f8841a5d>] ahci_interrupt+0x370/0x40d [ahci]
 [<c01512c6>] handle_IRQ_event+0x21/0x48
 [<c01521ca>] handle_edge_irq+0xc9/0x10a
 [<c0152101>] ? handle_edge_irq+0x0/0x10a
 [<c0107518>] do_IRQ+0x8b/0xb7
 [<c01055db>] common_interrupt+0x23/0x28
 [<c032007b>] ? init_chipset_cmd64x+0xb/0x93
 [<c010316e>] ? mwait_idle_with_hints+0x39/0x3d
 [<c0103172>] ? mwait_idle+0x0/0xf
 [<c010317f>] mwait_idle+0xd/0xf
 [<c0103761>] cpu_idle+0xb0/0xe4
 [<c031b509>] rest_init+0x5d/0x5f

This is not a new problem. It was pointed out some time ago already,
but now the WARN_ON() finally made it into mainline :)

The fix is not obvious, as this code seems to be called from various
call sites.

Thanks,
	tglx

Re: 2.6.24-git: kmap_atomic() WARN_ON()

[Rafael J. Wysocki]

Hi Thomas,

On Thursday, 7 of February 2008, Thomas Gleixner wrote:
> current mainline triggers:

Has the issue been fixed in the meantime?

> WARNING: at /home/tglx/work/kernel/x86/linux-2.6/arch/x86/mm/highmem_32.c:52 kmap_atomic_prot+0xe5/0x19b()
> Modules linked in: ahci(+) sata_sil libata sd_mod scsi_mod raid1 ext3 jbd ehci_hcd ohci_hcd uhci_hcd
> Pid: 0, comm: swapper Not tainted 2.6.24 #173
>  [<c0126b60>] warn_on_slowpath+0x41/0x51
>  [<c011c5eb>] ? __enqueue_entity+0x9c/0xa4
>  [<c011c717>] ? enqueue_entity+0x124/0x13b
>  [<c011cedb>] ? enqueue_task_fair+0x41/0x4c
>  [<c032ce88>] ? _spin_lock_irqsave+0x14/0x2e
>  [<c012e885>] ? lock_timer_base+0x1f/0x3e
>  [<c011ad6d>] kmap_atomic_prot+0xe5/0x19b
>  [<c011ae37>] kmap_atomic+0x14/0x16
>  [<f88ea218>] ata_scsi_rbuf_get+0x1e/0x2c [libata]
>  [<f88ea821>] atapi_qc_complete+0x23f/0x289 [libata]
>  [<f88e3d00>] __ata_qc_complete+0x8e/0x93 [libata]
>  [<f88e3fc7>] ata_qc_complete+0x115/0x128 [libata]
>  [<f88e8d47>] ata_qc_complete_multiple+0x86/0xa0 [libata]
>  [<f8841a5d>] ahci_interrupt+0x370/0x40d [ahci]
>  [<c01512c6>] handle_IRQ_event+0x21/0x48
>  [<c01521ca>] handle_edge_irq+0xc9/0x10a
>  [<c0152101>] ? handle_edge_irq+0x0/0x10a
>  [<c0107518>] do_IRQ+0x8b/0xb7
>  [<c01055db>] common_interrupt+0x23/0x28
>  [<c032007b>] ? init_chipset_cmd64x+0xb/0x93
>  [<c010316e>] ? mwait_idle_with_hints+0x39/0x3d
>  [<c0103172>] ? mwait_idle+0x0/0xf
>  [<c010317f>] mwait_idle+0xd/0xf
>  [<c0103761>] cpu_idle+0xb0/0xe4
>  [<c031b509>] rest_init+0x5d/0x5f
> 
> This is not a new problem. It was pointed out some time ago already,
> but now the WARN_ON() finally made it into mainline :)
> 
> The fix is not obvious, as this code seems to be called from various
> call sites.
> 
> Thanks,
> 	tglx

Re: 2.6.24-git: kmap_atomic() WARN_ON()

[Thomas Gleixner]

On Wed, 13 Feb 2008, Rafael J. Wysocki wrote:

> Hi Thomas,
> 
> On Thursday, 7 of February 2008, Thomas Gleixner wrote:
> > current mainline triggers:
> 
> Has the issue been fixed in the meantime?

Nope. 

Just for reference: http://lkml.org/lkml/2007/1/14/38 looks
frighteningly similar.

Thanks,

	tglx

Re: 2.6.24-git: kmap_atomic() WARN_ON()

[Björn Steinbrink]

On 2008.02.07 00:58:42 +0100, Thomas Gleixner wrote:
> current mainline triggers:
> 
> WARNING: at /home/tglx/work/kernel/x86/linux-2.6/arch/x86/mm/highmem_32.c:52 kmap_atomic_prot+0xe5/0x19b()
> Modules linked in: ahci(+) sata_sil libata sd_mod scsi_mod raid1 ext3 jbd ehci_hcd ohci_hcd uhci_hcd
> Pid: 0, comm: swapper Not tainted 2.6.24 #173
>  [<c0126b60>] warn_on_slowpath+0x41/0x51
>  [<c011c5eb>] ? __enqueue_entity+0x9c/0xa4
>  [<c011c717>] ? enqueue_entity+0x124/0x13b
>  [<c011cedb>] ? enqueue_task_fair+0x41/0x4c
>  [<c032ce88>] ? _spin_lock_irqsave+0x14/0x2e
>  [<c012e885>] ? lock_timer_base+0x1f/0x3e
>  [<c011ad6d>] kmap_atomic_prot+0xe5/0x19b
>  [<c011ae37>] kmap_atomic+0x14/0x16
>  [<f88ea218>] ata_scsi_rbuf_get+0x1e/0x2c [libata]
>  [<f88ea821>] atapi_qc_complete+0x23f/0x289 [libata]
>  [<f88e3d00>] __ata_qc_complete+0x8e/0x93 [libata]
>  [<f88e3fc7>] ata_qc_complete+0x115/0x128 [libata]
>  [<f88e8d47>] ata_qc_complete_multiple+0x86/0xa0 [libata]
>  [<f8841a5d>] ahci_interrupt+0x370/0x40d [ahci]
>  [<c01512c6>] handle_IRQ_event+0x21/0x48
>  [<c01521ca>] handle_edge_irq+0xc9/0x10a
>  [<c0152101>] ? handle_edge_irq+0x0/0x10a
>  [<c0107518>] do_IRQ+0x8b/0xb7
>  [<c01055db>] common_interrupt+0x23/0x28
>  [<c032007b>] ? init_chipset_cmd64x+0xb/0x93
>  [<c010316e>] ? mwait_idle_with_hints+0x39/0x3d
>  [<c0103172>] ? mwait_idle+0x0/0xf
>  [<c010317f>] mwait_idle+0xd/0xf
>  [<c0103761>] cpu_idle+0xb0/0xe4
>  [<c031b509>] rest_init+0x5d/0x5f
> 
> This is not a new problem. It was pointed out some time ago already,
> but now the WARN_ON() finally made it into mainline :)
> 
> The fix is not obvious, as this code seems to be called from various
> call sites.

Hm, do you have lockdep enabled? If not, does lockdep make this go away?
Because lockdep will set IRQF_DISABLED for all interrupt handlers, and
unless that flag is set, handle_IRQ_event will reenable interrupts while
the handler is running. And ahci_interrupt only uses a plain spin_lock,
so interrupts keep being enabled. The patch below should help with that.

Hmhm, maybe that also solves the deadlock you saw? Dunno...

I can't come up with an useful commit message right now, but I'll resend
in suitable form for submission if that thing actually works.

Björn


diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 1db93b6..ae3dbc8 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1739,6 +1739,7 @@ static irqreturn_t ahci_interrupt(int irq, void *dev_instance)
 	unsigned int i, handled = 0;
 	void __iomem *mmio;
 	u32 irq_stat, irq_ack = 0;
+	unsigned long flags;
 
 	VPRINTK("ENTER\n");
 
@@ -1751,7 +1752,7 @@ static irqreturn_t ahci_interrupt(int irq, void *dev_instance)
 	if (!irq_stat)
 		return IRQ_NONE;
 
-	spin_lock(&host->lock);
+	spin_lock_irqsave(&host->lock, flags);
 
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap;
@@ -1778,7 +1779,7 @@ static irqreturn_t ahci_interrupt(int irq, void *dev_instance)
 		handled = 1;
 	}
 
-	spin_unlock(&host->lock);
+	spin_unlock_irqrestore(&host->lock, flags);
 
 	VPRINTK("EXIT\n");
 

Re: 2.6.24-git: kmap_atomic() WARN_ON()

[Jeff Garzik]

Björn Steinbrink wrote:
> On 2008.02.07 00:58:42 +0100, Thomas Gleixner wrote:
>> current mainline triggers:
>>
>> WARNING: at /home/tglx/work/kernel/x86/linux-2.6/arch/x86/mm/highmem_32.c:52 kmap_atomic_prot+0xe5/0x19b()
>> Modules linked in: ahci(+) sata_sil libata sd_mod scsi_mod raid1 ext3 jbd ehci_hcd ohci_hcd uhci_hcd
>> Pid: 0, comm: swapper Not tainted 2.6.24 #173
>>  [<c0126b60>] warn_on_slowpath+0x41/0x51
>>  [<c011c5eb>] ? __enqueue_entity+0x9c/0xa4
>>  [<c011c717>] ? enqueue_entity+0x124/0x13b
>>  [<c011cedb>] ? enqueue_task_fair+0x41/0x4c
>>  [<c032ce88>] ? _spin_lock_irqsave+0x14/0x2e
>>  [<c012e885>] ? lock_timer_base+0x1f/0x3e
>>  [<c011ad6d>] kmap_atomic_prot+0xe5/0x19b
>>  [<c011ae37>] kmap_atomic+0x14/0x16
>>  [<f88ea218>] ata_scsi_rbuf_get+0x1e/0x2c [libata]
>>  [<f88ea821>] atapi_qc_complete+0x23f/0x289 [libata]
>>  [<f88e3d00>] __ata_qc_complete+0x8e/0x93 [libata]
>>  [<f88e3fc7>] ata_qc_complete+0x115/0x128 [libata]
>>  [<f88e8d47>] ata_qc_complete_multiple+0x86/0xa0 [libata]
>>  [<f8841a5d>] ahci_interrupt+0x370/0x40d [ahci]
>>  [<c01512c6>] handle_IRQ_event+0x21/0x48
>>  [<c01521ca>] handle_edge_irq+0xc9/0x10a
>>  [<c0152101>] ? handle_edge_irq+0x0/0x10a
>>  [<c0107518>] do_IRQ+0x8b/0xb7
>>  [<c01055db>] common_interrupt+0x23/0x28
>>  [<c032007b>] ? init_chipset_cmd64x+0xb/0x93
>>  [<c010316e>] ? mwait_idle_with_hints+0x39/0x3d
>>  [<c0103172>] ? mwait_idle+0x0/0xf
>>  [<c010317f>] mwait_idle+0xd/0xf
>>  [<c0103761>] cpu_idle+0xb0/0xe4
>>  [<c031b509>] rest_init+0x5d/0x5f
>>
>> This is not a new problem. It was pointed out some time ago already,
>> but now the WARN_ON() finally made it into mainline :)
>>
>> The fix is not obvious, as this code seems to be called from various
>> call sites.
> 
> Hm, do you have lockdep enabled? If not, does lockdep make this go away?
> Because lockdep will set IRQF_DISABLED for all interrupt handlers, and
> unless that flag is set, handle_IRQ_event will reenable interrupts while
> the handler is running. And ahci_interrupt only uses a plain spin_lock,
> so interrupts keep being enabled. The patch below should help with that.
> 
> Hmhm, maybe that also solves the deadlock you saw? Dunno...
> 
> I can't come up with an useful commit message right now, but I'll resend
> in suitable form for submission if that thing actually works.
> 
> Björn
> 
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 1db93b6..ae3dbc8 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1739,6 +1739,7 @@ static irqreturn_t ahci_interrupt(int irq, void *dev_instance)
>  	unsigned int i, handled = 0;
>  	void __iomem *mmio;
>  	u32 irq_stat, irq_ack = 0;
> +	unsigned long flags;
>  
>  	VPRINTK("ENTER\n");
>  
> @@ -1751,7 +1752,7 @@ static irqreturn_t ahci_interrupt(int irq, void *dev_instance)
>  	if (!irq_stat)
>  		return IRQ_NONE;
>  
> -	spin_lock(&host->lock);
> +	spin_lock_irqsave(&host->lock, flags);
>  
>  	for (i = 0; i < host->n_ports; i++) {
>  		struct ata_port *ap;
> @@ -1778,7 +1779,7 @@ static irqreturn_t ahci_interrupt(int irq, void *dev_instance)
>  		handled = 1;
>  	}
>  
> -	spin_unlock(&host->lock);
> +	spin_unlock_irqrestore(&host->lock, flags);

If this truly fixes the problem, then lockdep is definitely the problem 
source.

There are plenty of drivers that do the same thing that ahci does, in 
terms of interrupt handler locking...  and I will definitely push back 
on efforts to convert otherwise-100%-safe spin_lock() into 
spin_lock_irqsave() just to quiet lockdep.

Very interesting email, thanks...

	Jeff

Re: 2.6.24-git: kmap_atomic() WARN_ON()

[Björn Steinbrink]

On 2008.02.25 15:08:35 -0500, Jeff Garzik wrote:
> Björn Steinbrink wrote:
>> Hm, do you have lockdep enabled? If not, does lockdep make this go away?
>> Because lockdep will set IRQF_DISABLED for all interrupt handlers, and
>> unless that flag is set, handle_IRQ_event will reenable interrupts while
>> the handler is running. And ahci_interrupt only uses a plain spin_lock,
>> so interrupts keep being enabled. The patch below should help with that.
>>
>> Hmhm, maybe that also solves the deadlock you saw? Dunno...
>>
>> I can't come up with an useful commit message right now, but I'll resend
>> in suitable form for submission if that thing actually works.
>>
>> Björn
>>
>>
>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>> index 1db93b6..ae3dbc8 100644
>> --- a/drivers/ata/ahci.c
>> +++ b/drivers/ata/ahci.c
>> @@ -1739,6 +1739,7 @@ static irqreturn_t ahci_interrupt(int irq, void *dev_instance)
>>  	unsigned int i, handled = 0;
>>  	void __iomem *mmio;
>>  	u32 irq_stat, irq_ack = 0;
>> +	unsigned long flags;
>>   	VPRINTK("ENTER\n");
>>  @@ -1751,7 +1752,7 @@ static irqreturn_t ahci_interrupt(int irq, void 
>> *dev_instance)
>>  	if (!irq_stat)
>>  		return IRQ_NONE;
>>  -	spin_lock(&host->lock);
>> +	spin_lock_irqsave(&host->lock, flags);
>>   	for (i = 0; i < host->n_ports; i++) {
>>  		struct ata_port *ap;
>> @@ -1778,7 +1779,7 @@ static irqreturn_t ahci_interrupt(int irq, void *dev_instance)
>>  		handled = 1;
>>  	}
>>  -	spin_unlock(&host->lock);
>> +	spin_unlock_irqrestore(&host->lock, flags);
>
> If this truly fixes the problem, then lockdep is definitely the problem  
> source.

Hm, lockdep keeps the interrupts _disabled_. The code that reenables the
interrupts was already in the first revision of Linus' git tree. So
lockdep would actually just hide the problem, not cause it.

Björn

Re: 2.6.24-git: kmap_atomic() WARN_ON()

[Andrew Morton]

On Mon, 25 Feb 2008 15:08:35 -0500 Jeff Garzik <jgarzik@pobox.com> wrote:

> Bj__rn Steinbrink wrote:
> > On 2008.02.07 00:58:42 +0100, Thomas Gleixner wrote:
> >> current mainline triggers:
> >>
> >> WARNING: at /home/tglx/work/kernel/x86/linux-2.6/arch/x86/mm/highmem_32.c:52 kmap_atomic_prot+0xe5/0x19b()
> >> Modules linked in: ahci(+) sata_sil libata sd_mod scsi_mod raid1 ext3 jbd ehci_hcd ohci_hcd uhci_hcd
> >> Pid: 0, comm: swapper Not tainted 2.6.24 #173
> >>  [<c0126b60>] warn_on_slowpath+0x41/0x51
> >>  [<c011c5eb>] ? __enqueue_entity+0x9c/0xa4
> >>  [<c011c717>] ? enqueue_entity+0x124/0x13b
> >>  [<c011cedb>] ? enqueue_task_fair+0x41/0x4c
> >>  [<c032ce88>] ? _spin_lock_irqsave+0x14/0x2e
> >>  [<c012e885>] ? lock_timer_base+0x1f/0x3e
> >>  [<c011ad6d>] kmap_atomic_prot+0xe5/0x19b
> >>  [<c011ae37>] kmap_atomic+0x14/0x16
> >>  [<f88ea218>] ata_scsi_rbuf_get+0x1e/0x2c [libata]
> >>  [<f88ea821>] atapi_qc_complete+0x23f/0x289 [libata]
> >>  [<f88e3d00>] __ata_qc_complete+0x8e/0x93 [libata]
> >>  [<f88e3fc7>] ata_qc_complete+0x115/0x128 [libata]
> >>  [<f88e8d47>] ata_qc_complete_multiple+0x86/0xa0 [libata]
> >>  [<f8841a5d>] ahci_interrupt+0x370/0x40d [ahci]
> >>  [<c01512c6>] handle_IRQ_event+0x21/0x48
> >>  [<c01521ca>] handle_edge_irq+0xc9/0x10a
> >>  [<c0152101>] ? handle_edge_irq+0x0/0x10a
> >>  [<c0107518>] do_IRQ+0x8b/0xb7
> >>  [<c01055db>] common_interrupt+0x23/0x28
> >>  [<c032007b>] ? init_chipset_cmd64x+0xb/0x93
> >>  [<c010316e>] ? mwait_idle_with_hints+0x39/0x3d
> >>  [<c0103172>] ? mwait_idle+0x0/0xf
> >>  [<c010317f>] mwait_idle+0xd/0xf
> >>  [<c0103761>] cpu_idle+0xb0/0xe4
> >>  [<c031b509>] rest_init+0x5d/0x5f
> >>
> >> This is not a new problem. It was pointed out some time ago already,
> >> but now the WARN_ON() finally made it into mainline :)
> >>
> >> The fix is not obvious, as this code seems to be called from various
> >> call sites.
> > 
> > Hm, do you have lockdep enabled? If not, does lockdep make this go away?
> > Because lockdep will set IRQF_DISABLED for all interrupt handlers, and
> > unless that flag is set, handle_IRQ_event will reenable interrupts while
> > the handler is running. And ahci_interrupt only uses a plain spin_lock,
> > so interrupts keep being enabled. The patch below should help with that.
> > 
> > Hmhm, maybe that also solves the deadlock you saw? Dunno...
> > 
> > I can't come up with an useful commit message right now, but I'll resend
> > in suitable form for submission if that thing actually works.
> > 
> > Bj__rn
> > 
> > 
> > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> > index 1db93b6..ae3dbc8 100644
> > --- a/drivers/ata/ahci.c
> > +++ b/drivers/ata/ahci.c
> > @@ -1739,6 +1739,7 @@ static irqreturn_t ahci_interrupt(int irq, void *dev_instance)
> >  	unsigned int i, handled = 0;
> >  	void __iomem *mmio;
> >  	u32 irq_stat, irq_ack = 0;
> > +	unsigned long flags;
> >  
> >  	VPRINTK("ENTER\n");
> >  
> > @@ -1751,7 +1752,7 @@ static irqreturn_t ahci_interrupt(int irq, void *dev_instance)
> >  	if (!irq_stat)
> >  		return IRQ_NONE;
> >  
> > -	spin_lock(&host->lock);
> > +	spin_lock_irqsave(&host->lock, flags);
> >  
> >  	for (i = 0; i < host->n_ports; i++) {
> >  		struct ata_port *ap;
> > @@ -1778,7 +1779,7 @@ static irqreturn_t ahci_interrupt(int irq, void *dev_instance)
> >  		handled = 1;
> >  	}
> >  
> > -	spin_unlock(&host->lock);
> > +	spin_unlock_irqrestore(&host->lock, flags);
> 
> If this truly fixes the problem, then lockdep is definitely the problem 
> source.
> 
> There are plenty of drivers that do the same thing that ahci does, in 
> terms of interrupt handler locking...  and I will definitely push back 
> on efforts to convert otherwise-100%-safe spin_lock() into 
> spin_lock_irqsave() just to quiet lockdep.
> 
> Very interesting email, thanks...
> 

I suspect this is a bug in my old kmap_atomic debugging patch.  It doesn't
know about the implicit irq-disablememnt which interrupt handlers enjoy.  I
don't think...

Re: 2.6.24-git: kmap_atomic() WARN_ON()

[Thomas Gleixner]

On Mon, 25 Feb 2008, Andrew Morton wrote:
> On Mon, 25 Feb 2008 15:08:35 -0500 Jeff Garzik <jgarzik@pobox.com> wrote:
> > There are plenty of drivers that do the same thing that ahci does, in 
> > terms of interrupt handler locking...  and I will definitely push back 
> > on efforts to convert otherwise-100%-safe spin_lock() into 
> > spin_lock_irqsave() just to quiet lockdep.
> > 
> > Very interesting email, thanks...
> > 
> 
> I suspect this is a bug in my old kmap_atomic debugging patch.  It doesn't
> know about the implicit irq-disablememnt which interrupt handlers enjoy.  I
> don't think...

I suspect here is confusion. The implicit irq-disablement of lockdep
is actually hiding the warning.

The code which emits the warning is:

        if (type == KM_IRQ0 || type == KM_IRQ1 || type == KM_BOUNCE_READ ||
                        type == KM_BIO_SRC_IRQ || type == KM_BIO_DST_IRQ) {
                if (!irqs_disabled()) {
                        WARN_ON(1);
                        warn_count--;
                }

It checks for _NOT_ irqs_disabled. The calling code is
ata_scsi_rbuf_get() which calls with:

     buf = kmap_atomic(sg_page(sg), KM_IRQ0) + sg->offset;

This happens with interrupts enabled. So the warning is according to
the well documented km_type enum and the equally well documented
highmem debug code correct.

Bjoern decoded it very well, just Jeff jumped to very interesting
conclusions.

Thanks,

	tglx

Re: 2.6.24-git: kmap_atomic() WARN_ON()

[Andrew Morton]

On Mon, 25 Feb 2008 23:01:59 +0100 (CET) Thomas Gleixner <tglx@linutronix.de> wrote:

> On Mon, 25 Feb 2008, Andrew Morton wrote:
> > On Mon, 25 Feb 2008 15:08:35 -0500 Jeff Garzik <jgarzik@pobox.com> wrote:
> > > There are plenty of drivers that do the same thing that ahci does, in 
> > > terms of interrupt handler locking...  and I will definitely push back 
> > > on efforts to convert otherwise-100%-safe spin_lock() into 
> > > spin_lock_irqsave() just to quiet lockdep.
> > > 
> > > Very interesting email, thanks...
> > > 
> > 
> > I suspect this is a bug in my old kmap_atomic debugging patch.  It doesn't
> > know about the implicit irq-disablememnt which interrupt handlers enjoy.  I
> > don't think...
> 
> I suspect here is confusion. The implicit irq-disablement of lockdep
> is actually hiding the warning.
> 
> The code which emits the warning is:
> 
>         if (type == KM_IRQ0 || type == KM_IRQ1 || type == KM_BOUNCE_READ ||
>                         type == KM_BIO_SRC_IRQ || type == KM_BIO_DST_IRQ) {
>                 if (!irqs_disabled()) {
>                         WARN_ON(1);
>                         warn_count--;
>                 }
> 
> It checks for _NOT_ irqs_disabled. The calling code is
> ata_scsi_rbuf_get() which calls with:
> 
>      buf = kmap_atomic(sg_page(sg), KM_IRQ0) + sg->offset;
> 
> This happens with interrupts enabled. So the warning is according to
> the well documented km_type enum and the equally well documented
> highmem debug code correct.
> 
> Bjoern decoded it very well, just Jeff jumped to very interesting
> conclusions.
> 

Ah, OK, yes.  ata is wrong.  It must disable interrupts here.  Otherwise
this CPU could get interrupted by some other device whose handler also uses
KM_IRQ0, resulting in data corruption.

Re: 2.6.24-git: kmap_atomic() WARN_ON()

[Jeff Garzik]

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

Welcome to test this... (attached, not tested nor even compiled, really)

	Jeff




[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1078 bytes --]

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 0562b0a..7b1f1ee 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1694,12 +1694,17 @@ void ata_scsi_rbuf_fill(struct ata_scsi_args *args,
 	u8 *rbuf;
 	unsigned int buflen, rc;
 	struct scsi_cmnd *cmd = args->cmd;
+	unsigned long flags;
+
+	local_irq_save(flags);
 
 	buflen = ata_scsi_rbuf_get(cmd, &rbuf);
 	memset(rbuf, 0, buflen);
 	rc = actor(args, rbuf, buflen);
 	ata_scsi_rbuf_put(cmd, rbuf);
 
+	local_irq_restore(flags);
+
 	if (rc == 0)
 		cmd->result = SAM_STAT_GOOD;
 	args->done(cmd);
@@ -2473,6 +2478,9 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
 		if ((scsicmd[0] == INQUIRY) && ((scsicmd[1] & 0x03) == 0)) {
 			u8 *buf = NULL;
 			unsigned int buflen;
+			unsigned long flags;
+
+			local_irq_save(flags);
 
 			buflen = ata_scsi_rbuf_get(cmd, &buf);
 
@@ -2490,6 +2498,8 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
 			}
 
 			ata_scsi_rbuf_put(cmd, buf);
+
+			local_irq_restore(flags);
 		}
 
 		cmd->result = SAM_STAT_GOOD;

Re: 2.6.24-git: kmap_atomic() WARN_ON()

[Ingo Molnar]

* Jeff Garzik <jgarzik@pobox.com> wrote:

> +	unsigned long flags;
> +
> +	local_irq_save(flags);

hm, couldnt we attach the irq disabling to some spinlock, in a natural 
way? Explicit flags fiddling is a PITA once we do things like threaded 
irq handlers, -rt, etc.

	Ingo

Re: 2.6.24-git: kmap_atomic() WARN_ON()

[Jeff Garzik]

Ingo Molnar wrote:
> * Jeff Garzik <jgarzik@pobox.com> wrote:
> 
>> +	unsigned long flags;
>> +
>> +	local_irq_save(flags);
> 
> hm, couldnt we attach the irq disabling to some spinlock, in a natural 
> way? Explicit flags fiddling is a PITA once we do things like threaded 
> irq handlers, -rt, etc.

Attaching the irq disabling to some spinlock is what would be 
artificial...  See the ahci.c patch earlier in this thread.  It is taken 
without spin_lock_irqsave() in the interrupt handler, and there is no 
reason to disable interrupts for the entirety of the interrupt handler 
run -- only the part where we call kmap.

This is only being done to satisfy kmap_atomic's requirements, not libata's.

I could add a "kmap lock" but that just seems silly.

	Jeff

Re: 2.6.24-git: kmap_atomic() WARN_ON()

[Andrew Morton]

On Tue, 26 Feb 2008 11:32:24 -0500 Jeff Garzik <jgarzik@pobox.com> wrote:

> Ingo Molnar wrote:
> > * Jeff Garzik <jgarzik@pobox.com> wrote:
> > 
> >> +	unsigned long flags;
> >> +
> >> +	local_irq_save(flags);
> > 
> > hm, couldnt we attach the irq disabling to some spinlock, in a natural 
> > way? Explicit flags fiddling is a PITA once we do things like threaded 
> > irq handlers, -rt, etc.
> 
> Attaching the irq disabling to some spinlock is what would be 
> artificial...  See the ahci.c patch earlier in this thread.  It is taken 
> without spin_lock_irqsave() in the interrupt handler, and there is no 
> reason to disable interrupts for the entirety of the interrupt handler 
> run -- only the part where we call kmap.
> 
> This is only being done to satisfy kmap_atomic's requirements, not libata's.
> 
> I could add a "kmap lock" but that just seems silly.
> 

It's a bit sad to disable interupts across a memset (how big is it?) just
for the small proportion of cases which are accessing a highmem page.

What you could do is to add an `unsigned long *flags' arg to
ata_scsi_rbuf_get() and ata_scsi_rbuf_put(), and then, in
ata_scsi_rbuf_get() do

	if (PageHighmem(page))
		local_irq_disable(*flags);

Re: 2.6.24-git: kmap_atomic() WARN_ON()

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> > This is only being done to satisfy kmap_atomic's requirements, not 
> > libata's.
> > 
> > I could add a "kmap lock" but that just seems silly.
> > 
> 
> It's a bit sad to disable interupts across a memset (how big is it?) 
> just for the small proportion of cases which are accessing a highmem 
> page.
> 
> What you could do is to add an `unsigned long *flags' arg to 
> ata_scsi_rbuf_get() and ata_scsi_rbuf_put(), and then, in 
> ata_scsi_rbuf_get() do
> 
> 	if (PageHighmem(page))
> 		local_irq_disable(*flags);

it would be much nicer to attach the irq disabling to the object, not to 
some arbitrary place in the code.

i.e. to introduce a kmap_atomic_irqsave(...,flags) and 
kunmap_atomic_irqrestore() API variant. _That_ then could be mapped by 
-rt to a non-irq disabling thing.

	Ingo

Re: 2.6.24-git: kmap_atomic() WARN_ON()

[Andrew Morton]

On Tue, 26 Feb 2008 21:49:43 +0100 Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > > This is only being done to satisfy kmap_atomic's requirements, not 
> > > libata's.
> > > 
> > > I could add a "kmap lock" but that just seems silly.
> > > 
> > 
> > It's a bit sad to disable interupts across a memset (how big is it?) 
> > just for the small proportion of cases which are accessing a highmem 
> > page.
> > 
> > What you could do is to add an `unsigned long *flags' arg to 
> > ata_scsi_rbuf_get() and ata_scsi_rbuf_put(), and then, in 
> > ata_scsi_rbuf_get() do
> > 
> > 	if (PageHighmem(page))
> > 		local_irq_disable(*flags);
> 
> it would be much nicer to attach the irq disabling to the object, not to 
> some arbitrary place in the code.
> 
> i.e. to introduce a kmap_atomic_irqsave(...,flags) and 
> kunmap_atomic_irqrestore() API variant. _That_ then could be mapped by 
> -rt to a non-irq disabling thing.
> 

Sure.  But iirc we haven't had a need for this before.  Which is a bit odd.
 

Re: 2.6.24-git: kmap_atomic() WARN_ON()

[Jeff Garzik]

Andrew Morton wrote:
> On Tue, 26 Feb 2008 21:49:43 +0100 Ingo Molnar <mingo@elte.hu> wrote:
>> i.e. to introduce a kmap_atomic_irqsave(...,flags) and 
>> kunmap_atomic_irqrestore() API variant. _That_ then could be mapped by 
>> -rt to a non-irq disabling thing.

> Sure.  But iirc we haven't had a need for this before.  Which is a bit odd.

We have such a need -- we just always paper over it with 
spin_lock_irqsave() or local_irq_save() because those are "the rules." 
grep around :)

See ata_pio_sector() in libata-core.c, where we do:

>         if (PageHighMem(page)) {
>                 unsigned long flags;
> 
>                 /* FIXME: use a bounce buffer */
>                 local_irq_save(flags);
>                 buf = kmap_atomic(page, KM_IRQ0);
> 
>                 /* do the actual data transfer */
>                 ap->ops->data_xfer(qc->dev, buf + offset, qc->sect_size, do_write);
> 
>                 kunmap_atomic(buf, KM_IRQ0);
>                 local_irq_restore(flags);
>         } else {
>                 buf = page_address(page);
>                 ap->ops->data_xfer(qc->dev, buf + offset, qc->sect_size, do_write);
>         }

This is a core non-DMA data transfer routine.  Given the high cost of 
disabling interrupts during the relatively-slow PIO data transfer, we 
added extra logic to only disable interrupts for the highmem case.

The code in libata-scsi generally only deals with a single buffer, on 
average less than 128 bytes in length.  So the memcpy() isn't really 
that expensive in the case being discussed -- unlike the 
ata_pio_sector() case, where the cost is very, very high.

Aside: The "FIXME: use bounce buffer" comment above indicates the more 
optimal PIO data xfer approach of

	local_irq_save()
	kmap_atomic()
	memcpy into bounce buffer
	kunmap_atomic()
	local_irq_restore()

	/* do slow PIO bitbanging data transfer */
	ap->ops->data_xfer(...)

Regards,

	Jeff

Re: 2.6.24-git: kmap_atomic() WARN_ON()

[Alan Cox]

> Aside: The "FIXME: use bounce buffer" comment above indicates the more 
> optimal PIO data xfer approach of
> 
> 	local_irq_save()
> 	kmap_atomic()
> 	memcpy into bounce buffer
> 	kunmap_atomic()
> 	local_irq_restore()
> 
> 	/* do slow PIO bitbanging data transfer */
> 	ap->ops->data_xfer(...)

Definitely - older PATA controllers are unbuffered. A PIO_0 transfer is
running at ISA speed with IRQs off. Guaranteed to give Ingo's RT a blip.

Alan

Re: 2.6.24-git: kmap_atomic() WARN_ON()

[Nick Piggin]

On Wednesday 27 February 2008 07:49, Ingo Molnar wrote:
> * Andrew Morton <akpm@linux-foundation.org> wrote:
> > > This is only being done to satisfy kmap_atomic's requirements, not
> > > libata's.
> > >
> > > I could add a "kmap lock" but that just seems silly.
> >
> > It's a bit sad to disable interupts across a memset (how big is it?)
> > just for the small proportion of cases which are accessing a highmem
> > page.
> >
> > What you could do is to add an `unsigned long *flags' arg to
> > ata_scsi_rbuf_get() and ata_scsi_rbuf_put(), and then, in
> > ata_scsi_rbuf_get() do
> >
> > 	if (PageHighmem(page))
> > 		local_irq_disable(*flags);
>
> it would be much nicer to attach the irq disabling to the object, not to
> some arbitrary place in the code.
>
> i.e. to introduce a kmap_atomic_irqsave(...,flags) and
> kunmap_atomic_irqrestore() API variant. _That_ then could be mapped by
> -rt to a non-irq disabling thing.

Yeah that is the right way to fix it, but that name implies that
interrupts are disabled for non-highmem pages too, which we don't
want. Can you think of a better one?

kmap_atomic_irq_safe maybe?

Re: 2.6.24-git: kmap_atomic() WARN_ON()

[Thomas Gleixner]

On Mon, 25 Feb 2008, Jeff Garzik wrote:

> Welcome to test this... (attached, not tested nor even compiled, really)

Works, but I agree with Ingo vs. the stand alone irq_en/disable.

Thanks,
	tglx